Make parent mount private before bind mounting rootfs #1148
Conversation
This reverts part of the commit eb0a144 That commit introduced two issues. - We need to make parent mount of rootfs private before bind mounting rootfs. Otherwise bind mounting root can propagate in other mount namespaces. (If parent mount is shared). - It broke test TestRootfsPropagationSharedMount() on Fedora. On fedora /tmp is a mount point with "shared" propagation. I think you should be able to reproduce it on other distributions as well as long as you mount tmpfs on /tmp and make it "shared" propagation. Reason for failure is that pivot_root() fails. And it fails because kernel does following check. IS_MNT_SHARED(new_mnt->mnt_parent) Say /tmp/foo is new rootfs, we have bind mounted rootfs, so new_mnt is /tmp/foo, and new_mnt->mnt_parent is /tmp which is "shared" on fedora and above check fails. So this change broke few things, it is a good idea to revert part of it. Signed-off-by: Vivek Goyal <[email protected]>
|
Following commit introduced regression. commit eb0a144 |
|
Ok, looks like following is the PR for above mentioned commit. |
|
LGTM |
|
Another way to reproduce problem manually is to do following.
This fails for me with following error message. container_linux.go:247: starting container process caused "process_linux.go:359: container init caused "rootfs_linux.go:89: jailing process inside rootfs caused \"pivot_root invalid argument\""" |
|
@rhvgoyal This PR doesn't fix this problem for me: It also doesn't work if you do it with Rejected. |
|
@cyphar It is working for me. I tried the steps you mentioned. And works with Does it work without my patch?" Are you using any specialized config.json? I am using one generated by runc spec and just added What distro are you using. Trying to come close to your configuration so that I can try to reproduce the issue and debug. |
|
Here's my |
|
@cyphar tried your config.json and that works for me to (on top of fedora 24). I guess I will try to install openSUSE tumbleweed. |
|
@cyphar I just tested with openSuSE tumbleweed and it works for me.
One thing which surprised me though the path to runc binary. I did But path was still resolving to Are you sure you are using new runc binary. |
|
I must've forgotten to recompile |
|
Okay, I think I'm starting to go insane. I've managed to trigger a third failure mode, where |
|
I followed the step without this PR, and I got panic: And my host mount is messed up, all container mount info propagated to host. :( I'm on ubuntu 14.04 with the latest runc master code. |
|
@hqhq That's the same panic I get, but the panic only comes after you do a |
|
@hqhq I am getting that backtrace without this PR. runc hangs and when I do Ctrl-C, I get that backtrace. But with this PR, I don't see the hang and backtrace. |
|
@hqhq I reverted the pivot_root(".",".") patch and now runc hang goes away and I see the error message instead, as expected. (This is without my PR). $ runc run foo |
|
Eugh, that's not good. Now there's an issue with the I'll take a look at this next week (this week is pretty packed). |
|
@cyphar I can reproduce the problem without my PR. But with my PR I don't see the issue you mentioned. Please make sure you are testing with right binary. Looks like when pivot_root(".", ".") fails, it makes some changes which can make the calling process behave badly and that's what we seem to be facing. My PR just avoids the failure, so I don't see the issue. So, IMO, my PR is a safe change to do, irrespective of pivot_root(".", "."). It is fixing a regression introduced fixed few months back. We should continue to debug though that why pivot_root(".",".") failure leaves runc in a hung state. |
|
I think problem might be that we do Fchdir(newroot) but in case of failure don't restore the orginal state back. Should we store cwd of calling process and restore it if pivot_root(".", ".") fails. |
|
Okay, it looks like my previous issues were me not rebuilding the binary properly (not sure). I just tried it again and it works. I'm going to lgtm this, and we can investigate the Up to you though, it's probably too special-purpose to justify. LGTM. |
|
@cyphar I think previous issue is that Changing it to Given my PR got rid of this code entirely, it automatically fixes the other issue too. |
This reverts part of the commit eb0a144
That commit introduced two issues.
We need to make parent mount of rootfs private before bind mounting
rootfs. Otherwise bind mounting root can propagate in other mount
namespaces. (If parent mount is shared).
It broke test TestRootfsPropagationSharedMount() on Fedora.
On fedora /tmp is a mount point with "shared" propagation. I think
you should be able to reproduce it on other distributions as well
as long as you mount tmpfs on /tmp and make it "shared" propagation.
Reason for failure is that pivot_root() fails. And it fails because
kernel does following check.
IS_MNT_SHARED(new_mnt->mnt_parent)
Say /tmp/foo is new rootfs, we have bind mounted rootfs, so new_mnt
is /tmp/foo, and new_mnt->mnt_parent is /tmp which is "shared" on
fedora and above check fails.
So this change broke few things, it is a good idea to revert part of it.
Signed-off-by: Vivek Goyal [email protected]