diff --git a/storage/drivers/overlay/composefs.go b/storage/drivers/overlay/composefs.go index e325e5a5f4..613b30ffd7 100644 --- a/storage/drivers/overlay/composefs.go +++ b/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,11 +59,36 @@ 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() @@ -70,28 +96,7 @@ func generateComposeFsBlob(verityDigests map[string]string, toc any, composefsDi } 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) diff --git a/storage/pkg/chunked/filesystem_linux.go b/storage/pkg/chunked/filesystem_linux.go index 152ffee392..7b7e58482a 100644 --- a/storage/pkg/chunked/filesystem_linux.go +++ b/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) { diff --git a/storage/pkg/chunked/filesystem_linux_test.go b/storage/pkg/chunked/filesystem_linux_test.go index f4877aa77b..0191262ef0 100644 --- a/storage/pkg/chunked/filesystem_linux_test.go +++ b/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) diff --git a/storage/pkg/chunked/storage_linux.go b/storage/pkg/chunked/storage_linux.go index 24f26c2102..a220e8e2bd 100644 --- a/storage/pkg/chunked/storage_linux.go +++ b/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 }