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:24:23PM +0100, Jan Kara wrote:
> Readers of /proc/mounts (and similar files) are currently unprotected
> from concurrently running remount on the filesystem they are reporting.
> This can not only result in bogus reported information but also in
> confusion in filesystems ->show_options callbacks resulting in
> use-after-free issues or similar (for example ext4 handling of quota
> file names is prone to such races).
> 
> Fix the problem by protecting showing of mount information with
> sb->s_umount semaphore.

> +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


> +	up_read(&namespace_sem);
> +	down_read(&sb->s_umount);
> +	/*
> +	 * Sb may be dead by now but that just means we won't find it in any
> +	 * mount and drop lock & reference anyway.
> +	 */
> +	p->locked_sb = sb;
> +	goto restart;

No.
	if (likely(sb->s_root))
		p->locked_sb = sb;
	else
		drop_super(sb);
	goto restart;