Skip to content
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.

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

overlay: do not clone source recursively #1309

Merged

Conversation

giuseppe
Copy link
Member

do not clone the source directory in recursive mode (the equivalent of
MS_BIND|MS_RECURSIVE) but use only a regular bind mount. If
not recursive bind mount is used then the existing overlay mounts are
not replicated. In this way a new idmapped mount won't need to map
the overlay mount as well, causing the mount_settattr(2) syscall to
fail with EINVAL since it is not possible to idmap an overlay mount
yet.

Closes: #1308

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

do not clone the source directory in recursive mode (the equivalent of
MS_BIND|MS_RECURSIVE) but use only a regular bind mount.  If
not recursive bind mount is used then the existing overlay mounts are
not replicated.  In this way a new idmapped mount won't need to map
the overlay mount as well, causing the mount_settattr(2) syscall to
fail with EINVAL since it is not possible to idmap an overlay mount
yet.

Closes: containers#1308

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@rhatdan
Copy link
Member

rhatdan commented Aug 16, 2022

@rhatdan
Copy link
Member

rhatdan commented Aug 16, 2022

@markhuang1212 PTAL and see if this solves your issue.

@markhuang1212
Copy link

@markhuang1212 PTAL and see if this solves your issue.

It solved my issue.

@nalind
Copy link
Member

nalind commented Aug 16, 2022

Sounds reasonable. Is there a test that exercises this scenario?
The constants defined in idmapped_utils.go are now present in golang.org/x/sys/unix, so we might want to switch to using those definitions.

Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

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

LGTM

@flouthoc
Copy link
Collaborator

Sounds reasonable. Is there a test that exercises this scenario?
The constants defined in idmapped_utils.go are now present in golang.org/x/sys/unix, so we might want to switch to using those definitions.

Just waiting for this ^

remove some constants that are also defined in the unix package.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@giuseppe
Copy link
Member Author

Sounds reasonable. Is there a test that exercises this scenario?
The constants defined in idmapped_utils.go are now present in golang.org/x/sys/unix, so we might want to switch to using those definitions.

I've added another commit that drops these constants.

I am not sure about a test, it is not a new feature just a different way to mount the layers when there are mappings so we are taking advantage of the existing ones. We have not caught the issue in the CI because the new feature requires Linux 5.19 that is currently only available on Rawhide

@rhatdan
Copy link
Member

rhatdan commented Aug 17, 2022

LGTM

@rhatdan rhatdan merged commit d3f1078 into containers:main Aug 17, 2022
@thorstenrie
Copy link

Thanks for the fix to #1308. Referring to the comment, and as I understood it, and I hope I didn't get it wrong, it resolves for Arch Linux users with a new release of containers/storage, afterwards including it in podman with a new podman release (for reference, see FS#75605). I would appreciate considering a new containers/storage release to allow it to proceed down to the downstream. Thanks for your consideration and taking the time. Hope you are all safe & best regards

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User id mapping seems not working properly in kernel 5.19.1 on Fedora
6 participants