rootfs: switch ms_private remount of oldroot to ms_slave

[cyphar]

Using MS_PRIVATE meant that there was a race between the mount(2) and
the umount2(2) calls where runc inadvertently has a live reference to a
mountpoint that existed on the host (which the host cannot kill
implicitly through an unmount and peer sharing).

In particular, this means that if we have a devicemapper mountpoint and
the host is trying to delete the underlying device, the delete will fail
because it is "in use" during the race. While the race is very small
(and libdm actually retries to avoid these sorts of cases) this appears
to manifest in various cases.

Signed-off-by: Aleksa Sarai [email protected]

[cyphar]

This is a proposed fix for moby/moby#33846, I'm just posting it here because I'm not actually sure why we're using MS_PRIVATE.

[crosbymichael]

@rhvgoyal can you take a look at this?

[rhvgoyal]

This patch makes sense to me. Making it slave will make sure if host unmounts it, it goes away from here.

What I am not sure about is if mount propagation is synchronous. IOW, by the time umount() in host is finished, is it guaranteed that mount point as gone away in other slave mount namespace or not. Or it is asynchronous and mount will soon be propagated.

In case of later, a small race window will still exist and will require retrying device removal in a loop. Which we already do. So it probably is fine.

So as long as testing of this patch passes, it looks fine to me.

LGTM

[cyphar]

@rhvgoyal

What I am not sure about is if mount propagation is synchronous.

I will ask some of our kernel folks, as the VFS code has always been particularly hairy to read through (for me at least). I would hope it is synchronous, but I imagine there might be some races that force it to be asynchronous.

[cyphar]

I have a sneaking suspicion that this patch will actually also help fix the reproducer for moby/moby#34542. Note that it doesn't fix the /underlying/ problem.

[crosbymichael]

LGTM

oh devmapper

[rhvgoyal]

@cyphar If it just a small race, then it should not matter as devicemapper graph driver tries device deactivation in a loop. If we can't deactivate device in first try, we will do it in second try? So this is not a must, AFAIU.

[cyphar]

@rhvgoyal We've been hitting kernel oopses with devmapper recently because it looks like the namespace in which the actual kernel-side cleanup happens might be more important than it first seems. It's not clear whether this patch fixes the issue (we still can't reproduce it effectively) but it seems like namespace leaking can cause more than just annoyances, it can actually trigger panics.

[rhatdan]

@cyphar You probably know this but we are using oci-umount to clean up these leaks as an OCI Hook

[rhvgoyal]

@cyphar Can you provide backtrace of that oops. Looking at it might give some clues. If devicemapper oopses, then we should fix devicemapper as that's the root cause of the issue. Mounting being present in more than one mount namespace with private propagation should be just fine.

[cyphar]

@rhatdan Yeah, I'm aware of that.

@rhvgoyal It appears to be an XFS bug, there are a couple of SUSE kernel engineers already working on it. While all of that is well and good, the current scheme still has the race condition which may confuse any number of runc users (such cri-o and so on). I'll paste the backtrace in a minute after I download the coredump we got sent.

[rhvgoyal]

@cyphar so has it been verified that mount propagation is synchronous? If not, then this patch does not solve the race condition.

[euank]

I'm not an approver here, but this LGTM.

I think this also fixes a theoretical race under docker+overlay (per my comment/explanation here).

[hqhq]

LGTM

[mrunalp]

@cyphar Should we merge this or you still need to verify?

[cyphar]

We can merge this IMO.

[thaJeztah]

/cc @cpuguy83 @kolyshkin

[cpuguy83]

Makes sense, I think we have a similar issue in our chrootarchive package.