Skip to content

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

Merged
mtrmac merged 2 commits intocontainers:mainfrom
giuseppe:overlay-use-openat2
May 5, 2026
Merged

storage, overlay: use openat2 instead of using procfs#820
mtrmac merged 2 commits intocontainers:mainfrom
giuseppe:overlay-use-openat2

Conversation

@giuseppe
Copy link
Copy Markdown
Member

@giuseppe giuseppe commented May 5, 2026

No description provided.

Flags: unix.O_RDONLY | unix.O_CLOEXEC | unix.O_PATH,
Resolve: unix.RESOLVE_NO_SYMLINKS | unix.RESOLVE_BENEATH,
})
if err != nil {
Copy link
Copy Markdown
Member

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

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

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

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

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 .

@jankaluza
Copy link
Copy Markdown
Member

jankaluza commented May 5, 2026

LGTM once you address the issue with ignoring the openat2 errors (either by fixing or acknowledging in the comment).

@giuseppe giuseppe force-pushed the overlay-use-openat2 branch from 17aecd6 to 75ed086 Compare May 5, 2026 09:36
Copy link
Copy Markdown
Contributor

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Thanks!

Comment thread storage/drivers/overlay/overlay.go Outdated
Comment thread storage/drivers/overlay/overlay.go
giuseppe added 2 commits May 5, 2026 14:37
the kernel accepts only few flags with O_PATH, these do not include
O_RDONLY, it works only because `O_RDONLY == 0`.

followup for containers#820 (comment)

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@giuseppe giuseppe force-pushed the overlay-use-openat2 branch from 75ed086 to 363943a Compare May 5, 2026 12:37
@github-actions github-actions Bot added the common Related to "common" package label May 5, 2026
@mtrmac mtrmac merged commit dcfbb7d into containers:main May 5, 2026
37 checks passed
@Luap99
Copy link
Copy Markdown
Member

Luap99 commented May 6, 2026

I assume this code path was not tested, my simple vendor PR is failing:

# podman [options] --pull-option=enable_partial_images=true --pull-option=convert_images=true save -o /var/tmp/quay.io-libpod-cirros-latest.tar quay.io/libpod/cirros:latest
Copying blob sha256:b993cfcfd8fdc2731b308222fce8b5ce7a681780ab276b6a23e1b0fe3ce06bbb
Copying blob sha256:aa107a40759291c596cd432ca14cf15e781b91cf82121dc5a335eb734cafe1c4
Copying blob sha256:858d98ac489379d0c34c20811558091180c724d7d185693c3a601ba8c0a1cedf
Error: reading blob sha256:b993cfcfd8fdc2731b308222fce8b5ce7a681780ab276b6a23e1b0fe3ce06bbb: fgetxattr bin/busybox: bad file descriptor

https://api.cirrus-ci.com/v1/artifact/task/5161488211509248/html/int-podman-rawhide-root-host.log.html

Is it expected that we do not have any code coverage in the storage tests? should there be a overlay + composefs test here as part of the CI matrix?


The O_PATH docs are clear that this wont work.

The file itself is not opened, and other file operations (e.g., read(2), write(2), fchmod(2), fchown(2), fgetxattr(2), ioctl(2), mmap(2)) fail with the error EBADF.

I opened a PR #824

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

common Related to "common" package storage Related to "storage" package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants