Re: [PATCH 2/2] proc: Protect readers of /proc/mounts from remount

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

On Tue, Dec 11, 2018 at 06:58:31PM +0000, Al Viro wrote:

> > +static bool mounts_trylock_super(struct proc_mounts *p, struct super_block *sb)
> > +{
> > +	if (p->locked_sb == sb)
> > +		return true;
> > +	if (p->locked_sb) {
> > +		drop_super(p->locked_sb);
> > +		p->locked_sb = NULL;
> > +	}
> > +	if (down_read_trylock(&sb->s_umount)) {
> > +		hold_sb(sb);
> > +		p->locked_sb = sb;
> > +		return true;
> > +	}
> > +	return false;
> > +}
> 
> Bad calling conventions, and you are paying for those with making
> hold_sb() non-static (and having it, in the first place).
> 
> > +	if (mounts_trylock_super(p, sb))
> > +		return p->cached_mount;
> > +	/*
> > +	 * Trylock failed. Since namepace_sem ranks below s_umount (through
> > +	 * sb->s_umount > dir->i_rwsem > namespace_sem in the mount path), we
> > +	 * have to drop it, wait for s_umount and then try again to guarantee
> > +	 * forward progress.
> > +	 */
> > +	hold_sb(sb);
> 
> That.  Just hoist that hold_sb() into your trylock (and put it before the
> down_read_trylock() there, while we are at it).  And turn the other caller
> into
> 	if (unlikely(!.....))
> 		ret = -EAGAIN;
> 	else
> 		ret = p->show(m, &r->mnt);
> followed by unconditional drop_super().  And I would probably go for
> 	mount_trylock_super(&p->locked_super, sb)
> while we are at it, so that it's isolated from proc_mounts and can
> be moved to fs/super.c

Looking at it some more...  I still hate it ;-/  Take a look at traverse()
in fs/seq_file.c and think what kind of clusterfuck will it cause...