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;
