Re: [RFC v3 1/1] fs/namespace: remove RCU sync for MNT_DETACH umount

[Ian Kent <ikent@redhat.com>]

On 2/7/24 12:58, Christian Brauner wrote:
> On Fri, Jun 28, 2024 at 01:13:45PM GMT, Jan Kara wrote:
>> On Fri 28-06-24 10:58:54, Ian Kent wrote:
>>> On 27/6/24 19:54, Jan Kara wrote:
>>>> On Thu 27-06-24 09:11:14, Ian Kent wrote:
>>>>> On 27/6/24 04:47, Matthew Wilcox wrote:
>>>>>> On Wed, Jun 26, 2024 at 04:07:49PM -0400, Lucas Karpinski wrote:
>>>>>>> +++ b/fs/namespace.c
>>>>>>> @@ -78,6 +78,7 @@ static struct kmem_cache *mnt_cache __ro_after_init;
>>>>>>>     static DECLARE_RWSEM(namespace_sem);
>>>>>>>     static HLIST_HEAD(unmounted);	/* protected by namespace_sem */
>>>>>>>     static LIST_HEAD(ex_mountpoints); /* protected by namespace_sem */
>>>>>>> +static bool lazy_unlock = false; /* protected by namespace_sem */
>>>>>> That's a pretty ugly way of doing it.  How about this?
>>>>> Ha!
>>>>>
>>>>> That was my original thought but I also didn't much like changing all the
>>>>> callers.
>>>>>
>>>>> I don't really like the proliferation of these small helper functions either
>>>>> but if everyone
>>>>>
>>>>> is happy to do this I think it's a great idea.
>>>> So I know you've suggested removing synchronize_rcu_expedited() call in
>>>> your comment to v2. But I wonder why is it safe? I *thought*
>>>> synchronize_rcu_expedited() is there to synchronize the dropping of the
>>>> last mnt reference (and maybe something else) - see the comment at the
>>>> beginning of mntput_no_expire() - and this change would break that?
>>> Interesting, because of the definition of lazy umount I didn't look closely
>>> enough at that.
>>>
>>> But I wonder, how exactly would that race occur, is holding the rcu read
>>> lock sufficient since the rcu'd mount free won't be done until it's
>>> released (at least I think that's how rcu works).
>> I'm concerned about a race like:
>>
>> [path lookup]				[umount -l]
>> ...
>> path_put()
>>    mntput(mnt)
>>      mntput_no_expire(m)
>>        rcu_read_lock();
>>        if (likely(READ_ONCE(mnt->mnt_ns))) {
>> 					do_umount()
>> 					  umount_tree()
>> 					    ...
>> 					    mnt->mnt_ns = NULL;
>> 					    ...
>> 					  namespace_unlock()
>> 					    mntput(&m->mnt)
>> 					      mntput_no_expire(mnt)
>> 				              smp_mb();
>> 					      mnt_add_count(mnt, -1);
>> 					      count = mnt_get_count(mnt);
>> 					      if (count != 0) {
>> 						...
>> 						return;
>>          mnt_add_count(mnt, -1);
>>          rcu_read_unlock();
>>          return;
>> -> KABOOM, mnt->mnt_count dropped to 0 but nobody cleaned up the mount!
>>        }
> Yeah, I think that's a valid concern. mntput_no_expire() requires that
> the last reference is dropped after an rcu grace period and that can
> only be done by synchronize_rcu_*() (It could be reworked but that would
> be quite ugly.). See also mnt_make_shortterm() caller's for kernel
> initiated unmounts.

I've thought about this a couple of times now.


Isn't it the case here that the path lookup thread will have taken a 
reference

(because it's calling path_put()) and the umount will have taken a 
reference on

system call entry.


So for the mount being umounted the starting count will be at lest three 
then if

the umount mntput() is called from namespace_unlock() it will correctly see

count != 0 and the path lookup mntput() to release it's reference 
finally leaving

the mntput() of the path_put() from the top level system call function 
to release

the last reference.


Once again I find myself thinking this should be independent of the rcu 
wait because

only path walks done before the mount being detached can be happening 
and the lockless

walks are done holding the rcu read lock and how likely is it a ref-walk 
path lookup

(that should follow a failed rcu-walk in this case) has been able to 
grab a reference

anyway?


I think the only reason the wait could be significant is to prevent 
changes to the

structures concerned causing problems because they happen earlier than 
can be

tolerated. That I can understand.


Mmm ... I feel like I'm starting to sound like a broken record ... oops!

Ian