Re: [PATCH] Fix a race in put_mountpoint.

[ebiederm@xmission.com (Eric W. Biederman)]

Al Viro <viro@ZenIV.linux.org.uk> writes:

> On Tue, Jan 03, 2017 at 04:17:14PM +1300, Eric W. Biederman wrote:
>> Al Viro <viro@ZenIV.linux.org.uk> writes:
>> 
>> > On Tue, Jan 03, 2017 at 01:51:36PM +1300, Eric W. Biederman wrote:
>> >
>> >> The only significant thing I see is that you have not taken the
>> >> mount_lock on the path where new_mountpoint adds the new struct
>> >> mountpoint into the mountpoint hash table.
>> >
>> > Umm...  Point, but I really don't like that bouncing mount_lock up
>> > and down there.  It's not going to cause any serious overhead,
>> > but it just looks ugly... ;-/
>> >
>> > Let me think for a while...
>> 
>> The other possibility is to grab namespace_sem in mntput_no_expire
>> around the call of umount_mnt.  That is the only path where
>> put_mountpoint can be called where we are not holding namespace_sem.
>> That works in the small but I haven't traced the callers of mntput and
>> mntput_no_expire yet to see if it works in practice.
>
> No, that's a really bad idea.  Final mntput should _not_ happen under
> namespace_lock, but I don't want grabbing it in that place.

Agreed.  That just makes the code harder to maintain later on.

> How about this instead:

I really don't like the logic inlined as my patch to kill shadow mounts
needs to call acquire a mountpoint which may not already have been
allocated as well.

Beyond that we can make the logic simpler by causing d_set_mounted to
fail if the flag is already set and syncrhonize on that.  Which means
we don't have to verify the ordering between mount_lock
and rename_lock (from d_set_mounted) is not a problem, which makes
backports easier to verify.

Patch follows.

Eric