Miklos Szeredi <miklos@xxxxxxxxxx> writes:
> On Thu, Jun 13, 2019 at 8:35 PM Eric W. Biederman <ebiederm@xxxxxxxxxxxx> wrote:
>>
>> Christian Brauner <christian@xxxxxxxxxx> writes:
>>
>> > On Wed, Jun 12, 2019 at 06:00:39PM -1000, Linus Torvalds wrote:
>> >> On Wed, Jun 12, 2019 at 12:54 PM Christian Brauner <christian@xxxxxxxxxx> wrote:
>> >> >
>> >> > The commit changes the internal logic to lock mounts when propagating
>> >> > mounts (user+)mount namespaces and - I believe - causes do_mount_move()
>> >> > to fail at:
>> >>
>> >> You mean 'do_move_mount()'.
>> >>
>> >> > if (old->mnt.mnt_flags & MNT_LOCKED)
>> >> > goto out;
>> >> >
>> >> > If that's indeed the case we should either revert this commit (reverts
>> >> > cleanly, just tested it) or find a fix.
>> >>
>> >> Hmm.. I'm not entirely sure of the logic here, and just looking at
>> >> that commit 3bd045cc9c4b ("separate copying and locking mount tree on
>> >> cross-userns copies") doesn't make me go "Ahh" either.
>> >>
>> >> Al? My gut feel is that we need to just revert, since this was in 5.1
>> >> and it's getting reasonably late in 5.2 too. But maybe you go "guys,
>> >> don't be silly, this is easily fixed with this one-liner".
>> >
>> > David and I have been staring at that code today for a while together.
>> > I think I made some sense of it.
>> > One thing we weren't absolutely sure is if the old MS_MOVE behavior was
>> > intentional or a bug. If it is a bug we have a problem since we quite
>> > heavily rely on this...
>>
>> It was intentional.
>>
>> The only mounts that are locked in propagation are the mounts that
>> propagate together. If you see the mounts come in as individuals you
>> can always see/manipulate/work with the underlying mount.
>>
>> I can think of only a few ways for MNT_LOCKED to become set:
>> a) unshare(CLONE_NEWNS)
>> b) mount --rclone /path/to/mnt/tree /path/to/propagation/point
>> c) mount --move /path/to/mnt/tree /path/to/propgation/point
>>
>> Nothing in the target namespace should be locked on the propgation point
>> but all of the new mounts that came across as a unit should be locked
>> together.
>
> Locked together means the root of the new mount tree doesn't have
> MNT_LOCKED set, but all mounts below do have MNT_LOCKED, right?
>
> Isn't the bug here that the root mount gets MNT_LOCKED as well?
Yes, and the code to remove MNT_LOCKED is still sitting there in
propogate_one right after it calls copy_tree. It should be a trivial
matter of moving that change to after the lock_mnt_tree call.
Now that I have been elightened about anonymous mount namespaces
I am suspecting that we want to take the user_namespace of the anonymous
namespace into account when deciding to lock the mounts.
>> Then it breaking is definitely a regression that needs to be fixed.
>>
>> I believe the problematic change as made because the new mount
>> api allows attaching floating mounts. Or that was the plan last I
>> looked. Those floating mounts don't have a mnt_ns so will result
>> in a NULL pointer dereference when they are attached.
>
> Well, it's called anonymous namespace. So there *is* an mnt_ns, and
> its lifetime is bound to the file returned by fsmount().
Interesting. That has changed since I last saw the patches.
Below is what will probably be a straight forward fix for the regression.
Eric
diff --git a/fs/namespace.c b/fs/namespace.c
index ffb13f0562b0..a39edeecbc46 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2105,6 +2105,7 @@ static int attach_recursive_mnt(struct mount *source_mnt,
/* Notice when we are propagating across user namespaces */
if (child->mnt_parent->mnt_ns->user_ns != user_ns)
lock_mnt_tree(child);
+ child->mnt.mnt_flags &= ~MNT_LOCKED;
commit_tree(child);
}
put_mountpoint(smp);
diff --git a/fs/pnode.c b/fs/pnode.c
index 7ea6cfb65077..012be405fec0 100644
--- a/fs/pnode.c
+++ b/fs/pnode.c
@@ -262,7 +262,6 @@ static int propagate_one(struct mount *m)
child = copy_tree(last_source, last_source->mnt.mnt_root, type);
if (IS_ERR(child))
return PTR_ERR(child);
- child->mnt.mnt_flags &= ~MNT_LOCKED;
mnt_set_mountpoint(m, mp, child);
last_dest = m;
last_source = child;
