Re: [RFC][CFT][PATCH] Rewrite of propagate_umount() (was Re: [BUG] propagate_umount() breakage)

["Eric W. Biederman" <ebiederm@xmission.com>]

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

> On Mon, May 19, 2025 at 11:11:10AM -0700, Linus Torvalds wrote:
>> Another thing that is either purely syntactic, or shows that I
>> *really* don't understand your patch. Why do you do this odd thing:
>> 
>>         // reduce the set until it's non-shifting
>>         for (m = first_candidate(); m; m = trim_one(m))
>>                 ;
>> 
>> which seems to just walk the candidates list in a very non-obvious
>> manner (this is one of those "I had to go back and forth to see what
>> first_candidate() did and what lists it used" cases).
>> 
>> It *seems* to be the same as
>> 
>>         list_for_each_entry_safe(m, tmp, &candidates, mnt_umounting)
>>                 trim_one(m);
>> 
>> because if I read that code right, 'trim_one()' will just always
>> return the next entry in that candidate list.
>
>
> 	Another variant would be to steal one more bit from mnt_flags,
> set it for all candidates when collecting them, have is_candidate() check
> that instead of list_empty(&m->mnt_umounting) and clean it where this
> variant removes from the list; trim_one() would immediately return if
> bit is not set.  Then we could really do list_for_each_entry_safe(),
> with another loop doing list removals afterwards.  Extra work that way,
> though, and I still think it's more confusing...

I have only skimmed this so far, and I am a bit confused what we
are using MNT_MARK for.   I would think we should be able to use
MNT_MARK instead of stealing another bit.

Regardless I believe you said the goal is to make the code as readable
as possible, so next time it needs to be audited a decade from now
it won't be hard to figure out what is going on.

To that end I think leaving everything on the candidate list, and
flipping a bit when we decide that that the mount should be kept
will be easier to understand.

That way we can have all of the mostly naive algorithms examine
a mount and see what we should do with it, in all of the various
cases, and we don't have to be clever.

The only way I can see to avoid difficult audits is to remove as
much cleverness from the code as the problem domain allows.

Eric