From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:38892 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1727091AbeLLM4U (ORCPT ); Wed, 12 Dec 2018 07:56:20 -0500 Date: Wed, 12 Dec 2018 13:56:18 +0100 From: Jan Kara To: Al Viro Cc: Jan Kara , linux-fsdevel@vger.kernel.org Subject: Re: [PATCH 2/2] proc: Protect readers of /proc/mounts from remount Message-ID: <20181212125618.GD10902@quack2.suse.cz> References: <20181211172423.7709-1-jack@suse.cz> <20181211172423.7709-3-jack@suse.cz> <20181211185831.GH2217@ZenIV.linux.org.uk> <20181211191451.GJ2217@ZenIV.linux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181211191451.GJ2217@ZenIV.linux.org.uk> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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 SUSE Labs, CR