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

[Jan Kara <jack@suse.cz>]

On Tue 11-12-18 19:14:52, Al Viro wrote:
> 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...

I guess you mean that in case we fail to lock s_umount semaphore, we'll
return -EAGAIN and traverse() will abort? That is true but since we return
-EAGAIN, callers will call into traverse() again - both do:

while ((err = traverse(m, *ppos)) == -EAGAIN) ;

and then in m_start() we will do the blocking lock on s_umount. I agree it
is ugly and twisted but it should be rare...

Now looking at the code, maybe we could avoid this weird retry dance with
traverse(). Something like following in m_show():

        sb = mnt->mnt_sb;
	if (mount_trylock_super())
		show and done
	get passive sb reference
	namespace_unlock();
	down_read(&sb->s_umount);
	namespace_lock();
	new_mnt = seq_list_start();
	if (new_mnt != mnt)
		retry
	show and done

This could be handled completely inside m_show() so no strange retry dance
with traverse(). Do you find this better?

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR