Re: [PATCH] mnt: Protect the mountpoint hashtable with mount_lock

[Krister Johansen <kjlx@templeofstupid.com>]

On Wed, Jan 04, 2017 at 04:53:59PM +1300, Eric W. Biederman wrote:
> 
> Protecting the mountpoint hashtable with namespace_sem was sufficient
> until a call to umount_mnt was added to mntput_no_expire.  At which
> point it became possible for multiple calls of put_mountpoint on
> the same hash chain to happen on the same time.
> 
> Kristen Johansen <kjlx@templeofstupid.com> reported:
> > This can cause a panic when simultaneous callers of put_mountpoint
> > attempt to free the same mountpoint.  This occurs because some callers
> > hold the mount_hash_lock, while others hold the namespace lock.  Some
> > even hold both.
> >
> > In this submitter's case, the panic manifested itself as a GP fault in
> > put_mountpoint() when it called hlist_del() and attempted to dereference
> > a m_hash.pprev that had been poisioned by another thread.
> 
> Al Viro observed that the simple fix is to switch from using the namespace_sem
> to the mount_lock to protect the mountpoint hash table.
> 
> I have taken Al's suggested patch moved put_mountpoint in pivot_root
> (instead of taking mount_lock an additional time), and have replaced
> new_mountpoint with get_mountpoint a function that does the hash table
> lookup and addition under the mount_lock.   The introduction of get_mounptoint
> ensures that only the mount_lock is needed to manipulate the mountpoint
> hashtable.
> 
> d_set_mounted is modified to only set DCACHE_MOUNTED if it is not
> already set.  This allows get_mountpoint to use the setting of
> DCACHE_MOUNTED to ensure adding a struct mountpoint for a dentry
> happens exactly once.
> 
> Cc: stable@vger.kernel.org
> Fixes: ce07d891a089 ("mnt: Honor MNT_LOCKED when detaching mounts")
> Reported-by: Krister Johansen <kjlx@templeofstupid.com>
> Suggested-by: Al Viro <viro@ZenIV.linux.org.uk>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---

Sorry for the slow reply.

This looks right to me.  I just pulled in the patch and went through all
of the code paths in cscope.  Everything is now under the mount_lock,
which solves the problem from my perspective.  Feel free to put me down
as a reviewed-by if my vote counts.`

There's another issue with MNT_LOCKED and detached mounts that I've been
investigating.  I'd be curious to get your opinion before I write any
code.  I'll send that out in a separate e-mail, though.

-K