Re: [PATCH] mnt: Fix a memory stomp in umount

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

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

> On Thu, Dec 18, 2014 at 01:24:26PM -0600, Eric W. Biederman wrote:
>> Linus Torvalds <torvalds@linux-foundation.org> writes:
>> 
>> > On Thu, Dec 18, 2014 at 9:07 AM, Linus Torvalds
>> > <torvalds@linux-foundation.org> wrote:
>> >> Why is this piece of code using its own made up and buggy list handling in
>> >> the first place? We have list functions for these things, exactly so that
>> >> people shouldn't write buggy stuff by hand.
>> >
>> > Oh. Ok, I see what's going on. We have "list_splice()", but we don't
>> > have the equivalent "hlist_splice()". So it's doing that by hand, and
>> > did it badly.
>> >
>> > Al, this is your bug. I guess I can take the "manual hlist_splice" fix
>> > from Eric, but I'm not really happy with it. There's a few other
>> > places in that same commit where the list splice operation has been
>> > open-coded.
>> >
>> > Mind taking a look?
>> 
>> It looks like we can pretty easily use mnt_list instead of mnt_hash,
>> see below (note: the code is only compile tested).
>> 
>> While converting this to ordinary list helpers I found something
>> strange.
>> 
>> In __propagate_umount we currently add the child to be unmounted in a
>> different location in the list then we did before the conversion of
>> mnt_hash to a hlist for rcu's accesses benefit.
>> 
>> Now maybe propagate_next handles this (I still need to read and
>> understand that code) if not it looks like I may have found another bug,
>> as it looks like today we can add a node to our list without propogating
>> the unmount from the node.
>
> Er...  Why would we want to reprocess it?  The loop goes through all nodes
> getting events from ours; everything that gets events from _them_ included
> at the same time they are.

We might have wanted to because before your change to an hlist that is
what the code did.

Having read through the code of propagate_next it does look like it
iterates through the entire propagation hierarchy so there shouldn't be
a need to visit mounts that we have placed on the propagation list.

Any thoughts on using mnt_list instead of mnt_hash to allow the use of
list_splice and list_move?

Eric