Re: [PATCH RFC] NFSD: fix cannot umounting mount points under pseudo root

From: "J. Bruce Fields" <bfields@fieldses.org>
To: NeilBrown <neilb@suse.de>
Cc: Kinglong Mee <kinglongmee@gmail.com>,
	"linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH RFC] NFSD: fix cannot umounting mount points under pseudo root
Date: Fri, 8 May 2015 12:15:58 -0400	[thread overview]
Message-ID: <20150508161558.GA18851@fieldses.org> (raw)
In-Reply-To: <20150506082628.0dd049c7@notabene.brown>

On Wed, May 06, 2015 at 08:26:28AM +1000, NeilBrown wrote:
> On Tue, 5 May 2015 11:52:03 -0400 "J. Bruce Fields" <bfields@fieldses.org>
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 4a8d998b7274..0f554bf41dea 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -2139,6 +2139,8 @@ static struct dentry *lookup_hash(struct nameidata *nd)
> >   *
> >   * Note that this routine is purely a helper for filesystem usage and should
> >   * not be called by generic code.
> > + *
> > + * Must be called with the parented i_mutex held.
> 
> "parented"?
> 
>     * base->i_mutex must be held by caller.
> 
> ??

Thanks.

> > +struct dentry *lookup_one_len_unlocked(const char *name,
...
> > +	ret = __d_lookup(base, &this);
> > +	if (ret)
> > +		return ret;
> 
> __d_lookup() is a little too low level, as it doesn't call d_revalidate() and
> it doesn't retry if the rename_lock seqlock says it should.
> 
> The latter doesn't really matter as we would fall through to the safe
> mutex-protected version.
> The former isn't much of an issue for most filesystems under nfsd, but still
> needs to be handled to some extent.
> Also, the comment for __d_lookup says
> 
>  * __d_lookup callers must be commented.
> 
> The simplest resolution would be:
> 
> 	/* __d_lookup() is used to try to get a quick answer and avoid the
> 	 * mutex.  A false-negative does no harm.
> 	 */
> 	ret = __d_lookup(base, &this);
> 	if (ret && ret->d_flags & DCACHE_OP_REVALIDATE) {
> 		dput(ret);
> 		ret = NULL;
> 	}
> 	if (ret)
> 		return ret;

That looks right to me....  I'll probably take the simple option.

> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index a30e79900086..4accdb00976b 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -217,6 +217,13 @@ nfsd_lookup_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp,
> >  		host_err = PTR_ERR(dentry);
> >  		if (IS_ERR(dentry))
> >  			goto out_nfserr;
> > +		if (!(d_inode(dentry) && S_ISREG(d_inode(dentry)->i_mode))) {
> > +			/*
> > +			 * Never mind, we're not going to open this
> > +			 * anyway.  And we don't want to hold it over
> > +			 * the userspace upcalls in nfsd_mountpoint. */
> > +			fh_unlock(fhp);
> > +		}
> 
> You could avoid the test by just putting the fh_unlock(fhp) inside the 
> 
> 		if (nfsd_mountpoint(dentry, exp)) {
> 
> block.  It might also be appropriate for nfsd_mountpoint to fail on
> non-directories.

If check_export()'s to be believed, our support for regular-file
exports is intentional. So even if nfsd_mountpoint() is true, it's
possible we might still be about to open the result.

But I think we're OK:

The only reason for keeping the i_mutex here in the open case is to
avoid the situation where a client does OPEN(dirfh, "foo") and gets a
reply that grants a delegation even though the file has actually been
renamed to "bar".  (Thus violating the protocol, since the delegation's
supposed to guarantee you'll be notified before the file's renamed.)

(So the i_mutex is actually overkill, it would be enough to detect a
rename and then refuse the delegation (which we're always allowed to
do).)

But actually, if this is a mountpoint then I suppose we're safe from a
rename.  So, right, we can just move that unlock into the mountpoint
case.

--b.