Skip to content

storage, overlay: use openat2 instead of using procfs #820

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mtrmac merged 2 commits into from
May 5, 2026
+17 −7
Merged

storage, overlay: use openat2 instead of using procfs #820

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d65601f
common, storage: drop O_RDONLY with O_PATH
giuseppe May 5, 2026
363943a
storage, overlay: use openat2 instead of using procfs
giuseppe Feb 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion common/pkg/timezone/timezone.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,5 +106,5 @@ func copyTimezoneFile(containerRunDir, zonePath string) (string, error) {
}

func openDirectory(path string) (fd int, err error) {
return unix.Open(path, unix.O_RDONLY|O_PATH|unix.O_CLOEXEC, 0)
return unix.Open(path, O_PATH|unix.O_CLOEXEC, 0)
}
18 changes: 14 additions & 4 deletions storage/drivers/overlay/overlay.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2057,17 +2057,27 @@ func (g *overlayFileGetter) Get(path string) (io.ReadCloser, error) {
buf := make([]byte, unix.PathMax)
for _, d := range g.diffDirs {
if f, found := g.composefsMounts[d]; found {
// there is no *at equivalent for getxattr, but it can be emulated by opening the file under /proc/self/fd/$FD/$PATH
len, err := unix.Getxattr(fmt.Sprintf("/proc/self/fd/%d/%s", int(f.Fd()), path), "trusted.overlay.redirect", buf)
cfd, err := unix.Openat2(int(f.Fd()), path, &unix.OpenHow{
Flags: unix.O_CLOEXEC | unix.O_PATH,
Resolve: unix.RESOLVE_NO_SYMLINKS | unix.RESOLVE_BENEATH,
})
if err != nil {
Copy link
Copy Markdown
Member

@jankaluza jankaluza May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking if we should be more specific here and only continue on certain errors. Previous code, using Getxattr for example returned an error in case the path did not exist or couldn't be opened, while the code introduced in this PR ignores these issues.

I don't have a strong opinion, just something to think about.

Copy link
Copy Markdown
Member Author

@giuseppe giuseppe May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right, we should ignore only ENOENT. I'll fix it

Copy link
Copy Markdown
Contributor

@kolyshkin kolyshkin May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this was previously noted by @cgwalters somewhere in #651 (although I can't find it now; github is terrible in that regard).

Copy link
Copy Markdown
Contributor

@kolyshkin kolyshkin May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! There was also a comment from @cgwalters about RESOLVE_NO_SYMLINKS -- this might be a change in behavior here -- intermediate symlinks were followed before and are not followed now.

Or are we sure we do not use symlinks here at all?

Copy link
Copy Markdown
Contributor

@mtrmac mtrmac May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a reasonable reading of https://github.com/opencontainers/image-spec/blob/main/layer.md implies that layers never refer to files by paths that indirect through symlinks (and certainly that’s the only reasonable way to implement creating layers from diffs), but the spec doesn’t quite say so explicitly.

I’m personally very comfortable with enforcing that assumption .

if errors.Is(err, unix.ENOENT) {
continue
}
return nil, &fs.PathError{Op: "openat2", Path: path, Err: err}
Comment thread
mtrmac marked this conversation as resolved.
}
n, err := unix.Fgetxattr(cfd, "trusted.overlay.redirect", buf)
unix.Close(cfd)
if err != nil {
if errors.Is(err, unix.ENODATA) {
continue
}
return nil, &fs.PathError{Op: "getxattr", Path: path, Err: err}
return nil, &fs.PathError{Op: "fgetxattr", Path: path, Err: err}
}

// the xattr value is the path to the file in the composefs layer diff directory
return os.Open(filepath.Join(d, string(buf[:len])))
return os.Open(filepath.Join(d, string(buf[:n])))
}

f, err := os.Open(filepath.Join(d, path))
Expand Down
2 changes: 1 addition & 1 deletion storage/pkg/chunked/filesystem_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,7 +510,7 @@ func safeMkdir(dirfd int, mode os.FileMode, name string, metadata *fileMetadata,
}

func safeLink(dirfd int, mode os.FileMode, metadata *fileMetadata, options *archive.TarOptions) error {
sourceFile, err := openFileUnderRoot(dirfd, metadata.Linkname, unix.O_PATH|unix.O_RDONLY|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0)
sourceFile, err := openFileUnderRoot(dirfd, metadata.Linkname, unix.O_PATH|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0)
if err != nil {
return err
}
Expand Down
2 changes: 1 addition & 1 deletion storage/pkg/chunked/storage_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1549,7 +1549,7 @@ func (c *chunkedDiffer) ApplyDiff(dest string, options *archive.TarOptions, diff
}
}

dirfd, err := unix.Open(dest, unix.O_RDONLY|unix.O_PATH|unix.O_CLOEXEC, 0)
dirfd, err := unix.Open(dest, unix.O_PATH|unix.O_CLOEXEC, 0)
if err != nil {
return output, &fs.PathError{Op: "open", Path: dest, Err: err}
}
Expand Down
Loading