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

[Al Viro <viro@ZenIV.linux.org.uk>]

On Thu, Dec 18, 2014 at 10:57:19AM -0600, Eric W. Biederman wrote:
> 
> While reviewing the code of umount_tree I realized that when we append
> to a preexisting unmounted list we do not change pprev of the former
> first item in the list.
> 
> Which means later in namespace_unlock hlist_del_init(&mnt->mnt_hash) on
> the former first item of the list will stomp unmounted.first leaving
> it set to some random mount point which we are likely to free soon.
> 
> This isn't likely to hit, but if it does I don't know how anyone could
> track it down.

All you need for that kind of loop to work is correct -next on all elements.
Seriously.  Even correct first->pprev is not needed.  Look:
        struct hlist_head head = unmounted;

        if (likely(hlist_empty(&head))) // no dereferences of ppre *or* next
		sod off

        head.first->pprev = &head.first; // pprev of the first is valid now
        INIT_HLIST_HEAD(&unmounted);

        hlist_for_each_entry(mnt, &head, mnt_hash)
                if (mnt->mnt_ex_mountpoint.mnt)
                        mntget(mnt->mnt_ex_mountpoint.mnt);
	// only needed ->next for that loop
	...
        while (!hlist_empty(&head)) {
                mnt = hlist_entry(head.first, struct mount, mnt_hash);
                hlist_del_init(&mnt->mnt_hash);
		...
	}

Now, we certainly need pprev of the first to be correct for hlist_del_init()
to work.  What we do not need is correctness of pprev of the second as we
call hlist_del_init().  And _after_ hlist_del_init() pprev of what used to
be the second (the first, now) is healed.

So we actually are OK here.