-
Notifications
You must be signed in to change notification settings - Fork 359
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
Disable private mounts in chroot'ed operation in the unshare plugin #3228
Conversation
I'm trying to understand the purpose of the |
That's a good question. I basically copy-pasted it from #2617, assuming this is just part of the magic incantation to make any mount changes in the scriptlet private to the scriptlet. But, isn't that what CLONE_NEWNS is about? Looking at mount(2):
@jsegitz, do you remember what the deal with the / mount was? |
Yep, my thoughts as well, we are unsharing a namespace already, and it's a mount namespace because of the CLONE_NEWNS flag (as described in the man page). I guess the mount call must have had a different purpose then. Looking at the flags used in the call:
So my suspicion is that this has to do with preventing (some kind of) mount propagation but I'm yet to understand this topic better. LWN to the rescue: https://lwn.net/Articles/690679/ |
One potentially relevant difference between the original PR and the plugin is that the plugin actions take place in the already forked child process, whereas the original worked in the main rpm process. At any rate, this PR looks more and more like the wrong thing to do. |
Indeed, although just disabling the plugin in chroots (i.e. what this patch effectively does) sounds like a good-enough workaround until we have a proper solution/understanding. |
da59ef2
to
6abb38c
Compare
6abb38c
to
98f563a
Compare
My takeaway from some further poking + studying is that it's more complicated than we want to handle just now. Made the disabled in chroots -behavior explicit both in the docs and a warning message, I think that's the best we can do for 4.20. But instead of closing the ticket, we'll just move it back to the backlog with a bunch of added info. |
Sorry for the delay, I was away on a longer holiday (well, not sorry actually ;)). The reason is the one mentioned by @dmnks. I was also surprised by this, because my assumption was that a mount namespace is self contained. But withou this explicit mount call I saw side effects outside of the namespace. I'm not going to pretend that I fully understand this ... |
Right, I had similar experiences with this, without the explicit private mount the namespace doesn't seem to mean much because the mounts still propagate outside somehow. It's a weird thing. I think I saw something in the lines of "developers soon found the private mount namespace is in fact too private and had to change it" when looking for info on this. |
OK, I think I have a better understanding of this mount propagation thingy now (and still improving, as the history of edits in this comment would suggest 😄). First of all, the "remount" of What the call does here is that it recursively changes the propagation type of Much like the In fact, I've tried this with the unshare plugin by buildling a simple package with a So that explains the "mysterious" Now, the reason it fails in a chroot is simple, it tries to change the propagation type on the chrooted directory which (likely) is not a mount point. This is basically what @pmatilai realized in the original ticket #3187 already. The core issue here is that unshare & chroot are two competing operations, in a sense. I think, in long term, it would be better to make the unshare plugin somehow "replace" the chroot functionality altogether, instead of trying to work around it. That, or have a plugin hook in RPM that runs before chroot, which the unshare plugin would then use instead and thus run the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The essence of this PR is fine, we simply want to bail out in case we're running in chroot, at least for now. I only have some minor nitpicks, see inline.
mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL) inside a chroot fails with EINVAL if the "/" inside the chroot is not an actual mount point on the system - as it often isn't. For now, just disable that functionality on chroot operation. Related: rpm-software-management#3187
98f563a
to
6f51881
Compare
Besides the independent notes above, changed the mount propagation call source to NULL and changed the error message more specific for that case. Also, rebased. |
And thanks for the in-depth research, I ran out of patience around 1/4 of the way 😅 |
Thanks, LGTM now, merging. |
Disable private mounts in chroot'ed operation in the unshare plugin
mount("/", "/", NULL, MS_REC | MS_PRIVATE, NULL) inside a chroot fails with EINVAL. Apparently this is because "/" inside the chroot is not (necessarily) an actual mount point and ... then it starts getting more complicated. It should be possible to handle but not something we want to attempt just before a release candidate.
Related: #3187