chunked: fix ETXTBSY race in FS_IOC_ENABLE_VERITY with ForkLock

storage/drivers/overlay/composefs.go

@@ -13,6 +13,7 @@ import (
 	"strings"
 	"sync"
 	"sync/atomic"
+	"syscall"
 
 	"github.com/sirupsen/logrus"
 	"go.podman.io/storage/pkg/chunked/dump"
@@ -58,40 +59,44 @@ func generateComposeFsBlob(verityDigests map[string]string, toc any, composefsDi
 		return fmt.Errorf("failed to find mkcomposefs: %w", err)
 	}
 
-	outFile, err := os.OpenFile(destFile, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o644)
+	errBuf := &bytes.Buffer{}
+	outBuf := &bytes.Buffer{}
+	cmd := exec.Command(writerJSON, "--from-file", "-", "-")
+	cmd.Stderr = errBuf
+	cmd.Stdin = dumpReader
+	cmd.Stdout = outBuf
+	if err := cmd.Run(); err != nil {
+		rErr := fmt.Errorf("failed to convert json to erofs: %w", err)
+		exitErr := &exec.ExitError{}
+		if errors.As(err, &exitErr) {
+			return fmt.Errorf("%w: %s", rErr, strings.TrimSpace(errBuf.String()))
+		}
+		return rErr
+	}
+
+	// Hold ForkLock to prevent concurrent fork(2) from duplicating
+	// the writable fd, which causes ETXTBSY from FS_IOC_ENABLE_VERITY.
+	syscall.ForkLock.RLock()
+	defer syscall.ForkLock.RUnlock()
+
+	outFile, err := os.OpenFile(destFile, os.O_WRONLY|os.O_CREATE|os.O_EXCL|syscall.O_CLOEXEC, 0o644)
 	if err != nil {
 		return err
 	}
 
+	if _, err := outFile.Write(outBuf.Bytes()); err != nil {
+		outFile.Close()
+		return fmt.Errorf("failed to write composefs blob %s: %w", destFile, err)
+	}
+
 	roFile, err := os.Open(fmt.Sprintf("/proc/self/fd/%d", outFile.Fd()))
 	if err != nil {
 		outFile.Close()
 		return fmt.Errorf("failed to reopen %s as read-only: %w", destFile, err)
 	}
 	defer roFile.Close()
 
-	err = func() error {
-		// a scope to close outFile before setting fsverity on the read-only fd.
-		defer outFile.Close()
-
-		errBuf := &bytes.Buffer{}
-		cmd := exec.Command(writerJSON, "--from-file", "-", "-")
-		cmd.Stderr = errBuf
-		cmd.Stdin = dumpReader
-		cmd.Stdout = outFile
-		if err := cmd.Run(); err != nil {
-			rErr := fmt.Errorf("failed to convert json to erofs: %w", err)
-			exitErr := &exec.ExitError{}
-			if errors.As(err, &exitErr) {
-				return fmt.Errorf("%w: %s", rErr, strings.TrimSpace(errBuf.String()))
-			}
-			return rErr
-		}
-		return nil
-	}()
-	if err != nil {
-		return err
-	}
+	outFile.Close()
 
 	if err := fsverity.EnableVerity("manifest file", int(roFile.Fd())); err != nil && !errors.Is(err, unix.ENOTSUP) && !errors.Is(err, unix.ENOTTY) {
 		logrus.Warningf("%s", err)

storage/pkg/chunked/filesystem_linux.go

@@ -112,7 +112,9 @@ func doHardLink(dirfd, srcFd int, destFile string) error {
 	return err
 }
 
-func copyFileContent(srcFd int, fileMetadata *fileMetadata, dirfd int, mode os.FileMode, useHardLinks bool) (*os.File, int64, error) {
+// copyFileContent copies the content of srcFd into a new file under dirfd.
+// The returned *os.File, when non-nil, is opened read-only.
+func copyFileContent(srcFd int, fileMetadata *fileMetadata, dirfd int, mode os.FileMode, useHardLinks bool, needsForkLock bool) (*os.File, int64, error) {
 	destFile := fileMetadata.Name
 	src := procPathForFd(srcFd)
 	st, err := os.Stat(src)
@@ -131,6 +133,13 @@ func copyFileContent(srcFd int, fileMetadata *fileMetadata, dirfd int, mode os.F
 		}
 	}
 
+	if needsForkLock {
+		// Prevent concurrent fork(2) from duplicating this writable fd.
+		// See openDestinationFile for the full explanation.
+		syscall.ForkLock.RLock()
+		defer syscall.ForkLock.RUnlock()
+	}
+
 	// If the destination file already exists, we shouldn't blow it away
 	dstFile, err := openFileUnderRoot(dirfd, destFile, newFileFlags, mode)
 	if err != nil {
@@ -142,7 +151,13 @@ func copyFileContent(srcFd int, fileMetadata *fileMetadata, dirfd int, mode os.F
 		dstFile.Close()
 		return nil, -1, fmt.Errorf("copy to file %q under rootfs: %w", destFile, err)
 	}
-	return dstFile, st.Size(), nil
+
+	roFile, err := reopenFileReadOnly(dstFile)
+	dstFile.Close()
+	if err != nil {
+		return nil, -1, fmt.Errorf("reopen %q as read-only: %w", destFile, err)
+	}
+	return roFile, st.Size(), nil
 }
 
 func timeToTimespec(time *time.Time) (ts unix.Timespec) {
@@ -521,7 +536,7 @@ func safeLink(dirfd int, mode os.FileMode, metadata *fileMetadata, options *arch
 		return err
 	}
 
-	newFile, err := openFileUnderRoot(dirfd, metadata.Name, unix.O_WRONLY|unix.O_NOFOLLOW, 0)
+	newFile, err := openFileUnderRoot(dirfd, metadata.Name, unix.O_WRONLY|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0)
 	if err != nil {
 		// If the target is a symlink, open the file with O_PATH.
 		if errors.Is(err, unix.ELOOP) {

storage/pkg/chunked/filesystem_linux_test.go

@@ -286,7 +286,7 @@ func TestCopyFileContent(t *testing.T) {
 		},
 	}
 
-	newFile, newSize, err := copyFileContent(int(file.Fd()), &metadata, rootFd, 0o755, false)
+	newFile, newSize, err := copyFileContent(int(file.Fd()), &metadata, rootFd, 0o755, false, false)
 	require.NoError(t, err)
 
 	assert.Equal(t, size, int(newSize))
@@ -310,7 +310,7 @@ func TestCopyFileContent(t *testing.T) {
 		},
 	}
 
-	newFile, newSize, err = copyFileContent(int(file.Fd()), &metadataCopyHardLinks, rootFd, 0o755, true)
+	newFile, newSize, err = copyFileContent(int(file.Fd()), &metadataCopyHardLinks, rootFd, 0o755, true, false)
 	require.NoError(t, err)
 	assert.Nil(t, newFile)
 

storage/pkg/chunked/storage_linux.go

@@ -46,7 +46,7 @@ import (
 const (
 	maxNumberMissingChunks  = 1024
 	autoMergePartsThreshold = 1024 // if the gap between two ranges is below this threshold, automatically merge them.
-	newFileFlags            = (unix.O_CREAT | unix.O_TRUNC | unix.O_EXCL | unix.O_WRONLY)
+	newFileFlags            = (unix.O_CREAT | unix.O_TRUNC | unix.O_EXCL | unix.O_WRONLY | unix.O_CLOEXEC)
 	bigDataKey              = "zstd-chunked-manifest"
 	chunkedData             = "zstd-chunked-data"
 	chunkedLayerDataKey     = "zstd-chunked-layer-data"
@@ -439,7 +439,7 @@ func makeCopyBuffer() []byte {
 // name is the path to the file to copy in source.
 // dirfd is an open file descriptor to the destination root directory.
 // useHardLinks defines whether the deduplication can be performed using hard links.
-func copyFileFromOtherLayer(file *fileMetadata, source string, name string, dirfd int, useHardLinks bool) (bool, *os.File, int64, error) {
+func copyFileFromOtherLayer(file *fileMetadata, source string, name string, dirfd int, useHardLinks bool, needsForkLock bool) (bool, *os.File, int64, error) {
 	srcDirfd, err := unix.Open(source, unix.O_RDONLY|unix.O_CLOEXEC, 0)
 	if err != nil {
 		if errors.Is(err, unix.ENOENT) {
@@ -460,7 +460,7 @@ func copyFileFromOtherLayer(file *fileMetadata, source string, name string, dirf
 	}
 	defer srcFile.Close()
 
-	dstFile, written, err := copyFileContent(int(srcFile.Fd()), file, dirfd, 0, useHardLinks)
+	dstFile, written, err := copyFileContent(int(srcFile.Fd()), file, dirfd, 0, useHardLinks, needsForkLock)
 	if err != nil {
 		return false, nil, 0, fmt.Errorf("copy content to %q: %w", file.Name, err)
 	}
@@ -529,7 +529,7 @@ func canDedupFileWithHardLink(file *fileMetadata, fd int, s os.FileInfo) bool {
 // ostreeRepos is a list of OSTree repos.
 // dirfd is an open fd to the destination checkout.
 // useHardLinks defines whether the deduplication can be performed using hard links.
-func findFileInOSTreeRepos(file *fileMetadata, ostreeRepos []string, dirfd int, useHardLinks bool) (bool, *os.File, int64, error) {
+func findFileInOSTreeRepos(file *fileMetadata, ostreeRepos []string, dirfd int, useHardLinks bool, needsForkLock bool) (bool, *os.File, int64, error) {
 	digest, err := digest.Parse(file.Digest)
 	if err != nil {
 		logrus.Debugf("could not parse digest: %v", err)
@@ -562,7 +562,7 @@ func findFileInOSTreeRepos(file *fileMetadata, ostreeRepos []string, dirfd int,
 			continue
 		}
 
-		dstFile, written, err := copyFileContent(fd, file, dirfd, 0, useHardLinks)
+		dstFile, written, err := copyFileContent(fd, file, dirfd, 0, useHardLinks, needsForkLock)
 		if err != nil {
 			logrus.Debugf("could not copyFileContent: %v", err)
 			return false, nil, 0, nil
@@ -571,7 +571,7 @@ func findFileInOSTreeRepos(file *fileMetadata, ostreeRepos []string, dirfd int,
 	}
 	// If hard links deduplication was used and it has failed, try again without hard links.
 	if useHardLinks {
-		return findFileInOSTreeRepos(file, ostreeRepos, dirfd, false)
+		return findFileInOSTreeRepos(file, ostreeRepos, dirfd, false, needsForkLock)
 	}
 
 	return false, nil, 0, nil
@@ -582,12 +582,12 @@ func findFileInOSTreeRepos(file *fileMetadata, ostreeRepos []string, dirfd int,
 // file is the file to look for.
 // dirfd is an open file descriptor to the checkout root directory.
 // useHardLinks defines whether the deduplication can be performed using hard links.
-func findFileInOtherLayers(cache *layersCache, file *fileMetadata, dirfd int, useHardLinks bool) (bool, *os.File, int64, error) {
+func findFileInOtherLayers(cache *layersCache, file *fileMetadata, dirfd int, useHardLinks bool, needsForkLock bool) (bool, *os.File, int64, error) {
 	target, name, err := cache.findFileInOtherLayers(file, useHardLinks)
 	if err != nil || name == "" {
 		return false, nil, 0, err
 	}
-	return copyFileFromOtherLayer(file, target, name, dirfd, useHardLinks)
+	return copyFileFromOtherLayer(file, target, name, dirfd, useHardLinks, needsForkLock)
 }
 
 func maybeDoIDRemap(manifest []fileMetadata, options *archive.TarOptions) error {
@@ -814,8 +814,16 @@ type destinationFile struct {
 }
 
 func openDestinationFile(dirfd int, metadata *fileMetadata, options *archive.TarOptions, skipValidation bool, recordFsVerity recordFsVerityFunc) (*destinationFile, error) {
+	// Hold ForkLock to prevent concurrent fork(2) from duplicating
+	// the writable fd, which causes ETXTBSY from FS_IOC_ENABLE_VERITY.
+	if recordFsVerity != nil {
+		syscall.ForkLock.RLock()
+	}
 	file, err := openFileUnderRoot(dirfd, metadata.Name, newFileFlags, 0)
 	if err != nil {
+		if recordFsVerity != nil {
+			syscall.ForkLock.RUnlock()
+		}
 		return nil, err
 	}
 
@@ -844,29 +852,18 @@ func openDestinationFile(dirfd int, metadata *fileMetadata, options *archive.Tar
 	}, nil
 }
 
-func (d *destinationFile) Close() (Err error) {
-	defer func() {
-		var roFile *os.File
-		var err error
-
-		if d.recordFsVerity != nil {
-			roFile, err = reopenFileReadOnly(d.file)
-			if err == nil {
-				defer roFile.Close()
-			} else if Err == nil {
-				Err = err
-			}
-		}
-
-		err = d.file.Close()
-		if Err == nil {
-			Err = err
-		}
-
-		if Err == nil && roFile != nil {
-			Err = d.recordFsVerity(d.metadata.Name, roFile)
-		}
-	}()
+func (d *destinationFile) Close() error {
+	if d.recordFsVerity != nil {
+		defer syscall.ForkLock.RUnlock()
+	}
+	// Reopen read-only while the writable fd is still valid, then
+	// close the writable fd to release ForkLock.
+	roFile, err := reopenFileReadOnly(d.file)
+	d.file.Close()
+	if err != nil {
+		return err
+	}
+	defer roFile.Close()
 
 	if !d.skipValidation {
 		manifestChecksum, err := digest.Parse(d.metadata.Digest)
@@ -883,7 +880,14 @@ func (d *destinationFile) Close() (Err error) {
 		mode = *d.options.ForceMask
 	}
 
-	return setFileAttrs(d.dirfd, d.file, mode, d.metadata, d.options, false)
+	if err := setFileAttrs(d.dirfd, roFile, mode, d.metadata, d.options, false); err != nil {
+		return err
+	}
+
+	if d.recordFsVerity != nil {
+		return d.recordFsVerity(d.metadata.Name, roFile)
+	}
+	return nil
 }
 
 func closeDestinationFiles(files chan *destinationFile, errors chan error) {
@@ -1219,32 +1223,23 @@ func reopenFileReadOnly(f *os.File) (*os.File, error) {
 }
 
 func (c *chunkedDiffer) findAndCopyFile(dirfd int, r *fileMetadata, copyOptions *findAndCopyFileOptions, mode os.FileMode) (bool, error) {
-	finalizeFile := func(dstFile *os.File) error {
-		if dstFile == nil {
+	finalizeFile := func(roFile *os.File) error {
+		if roFile == nil {
 			return nil
 		}
-		err := setFileAttrs(dirfd, dstFile, mode, r, copyOptions.options, false)
-		if err != nil {
-			dstFile.Close()
-			return err
-		}
-		var roFile *os.File
-		if c.useFsVerity != graphdriver.DifferFsVerityDisabled {
-			roFile, err = reopenFileReadOnly(dstFile)
-		}
-		dstFile.Close()
-		if err != nil {
+		defer roFile.Close()
+
+		if err := setFileAttrs(dirfd, roFile, mode, r, copyOptions.options, false); err != nil {
 			return err
 		}
-		if roFile == nil {
+		if c.useFsVerity == graphdriver.DifferFsVerityDisabled {
 			return nil
 		}
-
-		defer roFile.Close()
 		return c.recordFsVerity(r.Name, roFile)
 	}
 
-	found, dstFile, _, err := findFileInOtherLayers(c.layersCache, r, dirfd, copyOptions.useHardLinks)
+	needsForkLock := c.useFsVerity != graphdriver.DifferFsVerityDisabled
+	found, dstFile, _, err := findFileInOtherLayers(c.layersCache, r, dirfd, copyOptions.useHardLinks, needsForkLock)
 	if err != nil {
 		return false, err
 	}
@@ -1255,7 +1250,7 @@ func (c *chunkedDiffer) findAndCopyFile(dirfd int, r *fileMetadata, copyOptions
 		return true, nil
 	}
 
-	found, dstFile, _, err = findFileInOSTreeRepos(r, copyOptions.ostreeRepos, dirfd, copyOptions.useHardLinks)
+	found, dstFile, _, err = findFileInOSTreeRepos(r, copyOptions.ostreeRepos, dirfd, copyOptions.useHardLinks, needsForkLock)
 	if err != nil {
 		return false, err
 	}