Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1448685 > unrolled thread

[PATCH 3/7] nfsd: remove redundant i_lookup check

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3/7] nfsd: remove redundant i_lookup check "J. Bruce Fields" <bfields@redhat.com> - 2016-07-22 19:50 +0200
    Re: [PATCH 3/7] nfsd: remove redundant i_lookup check Al Viro <viro@ZenIV.linux.org.uk> - 2016-07-24 02:30 +0200
      Re: [PATCH 3/7] nfsd: remove redundant i_lookup check "J. Bruce Fields" <bfields@fieldses.org> - 2016-07-24 14:20 +0200
        Re: [PATCH 3/7] nfsd: remove redundant i_lookup check Al Viro <viro@ZenIV.linux.org.uk> - 2016-07-24 16:30 +0200
          Re: [PATCH 3/7] nfsd: remove redundant i_lookup check "J. Bruce Fields" <bfields@fieldses.org> - 2016-07-24 22:30 +0200

#1448685 — [PATCH 3/7] nfsd: remove redundant i_lookup check

From: "J. Bruce Fields" <bfields@redhat.com>

I'm not sure why this was added.  It doesn't seem necessary, and no
other caller does this.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/vfs.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index fba8e7e521e0..7ae3b5a72a4d 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1169,9 +1169,6 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	dentry = fhp->fh_dentry;
 	dirp = d_inode(dentry);
 
-	err = nfserr_notdir;
-	if (!dirp->i_op->lookup)
-		goto out;
 	/*
 	 * Check whether the response file handle has been verified yet.
 	 * If it has, the parent directory should already be locked.
-- 
2.7.4

[toc] | [next] | [standalone]

#1449023

On Fri, Jul 22, 2016 at 01:48:52PM -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> I'm not sure why this was added.  It doesn't seem necessary, and no
> other caller does this.

lookup_one_len() will explode if you call it for non-directory (==
!d_can_lookup(), i.e. something without ->lookup()).  So unless the callers
do guarantee that check being true, it *is* needed.

[toc] | [prev] | [next] | [standalone]

#1449127

On Sun, Jul 24, 2016 at 01:22:06AM +0100, Al Viro wrote:
> On Fri, Jul 22, 2016 at 01:48:52PM -0400, J. Bruce Fields wrote:
> > From: "J. Bruce Fields" <bfields@redhat.com>
> > 
> > I'm not sure why this was added.  It doesn't seem necessary, and no
> > other caller does this.
> 
> lookup_one_len() will explode if you call it for non-directory (==
> !d_can_lookup(), i.e. something without ->lookup()).  So unless the callers
> do guarantee that check being true, it *is* needed.

Both callers call fh_verify(.,.,S_IFDIR,.), so at this point we know
that i_mode & S_IFMT == S_IFDIR.  Is there some odd case where that's
insufficient?  If so, I think there may be bugs elsewhere in nfsd.  If
not, I'll add a note to the changelog.

Thanks for reminding me to check this, I hadn't thought of that as an
"is this a directory" check, it makes more sense now.

--b.

[toc] | [prev] | [next] | [standalone]

#1449148

On Sun, Jul 24, 2016 at 08:10:14AM -0400, J. Bruce Fields wrote:
> On Sun, Jul 24, 2016 at 01:22:06AM +0100, Al Viro wrote:
> > On Fri, Jul 22, 2016 at 01:48:52PM -0400, J. Bruce Fields wrote:
> > > From: "J. Bruce Fields" <bfields@redhat.com>
> > > 
> > > I'm not sure why this was added.  It doesn't seem necessary, and no
> > > other caller does this.
> > 
> > lookup_one_len() will explode if you call it for non-directory (==
> > !d_can_lookup(), i.e. something without ->lookup()).  So unless the callers
> > do guarantee that check being true, it *is* needed.
> 
> Both callers call fh_verify(.,.,S_IFDIR,.), so at this point we know
> that i_mode & S_IFMT == S_IFDIR.  Is there some odd case where that's
> insufficient?  If so, I think there may be bugs elsewhere in nfsd.  If
> not, I'll add a note to the changelog.

First of all, such objects do exist; they probably won't be encountered by
nfsd and all instances I can think of are not writable, but...

> Thanks for reminding me to check this, I hadn't thought of that as an
> "is this a directory" check, it makes more sense now.

I'd have turned that into d_can_lookup(fhp->fh_dentry), actually.

[toc] | [prev] | [next] | [standalone]

#1449199

On Sun, Jul 24, 2016 at 03:23:07PM +0100, Al Viro wrote:
> On Sun, Jul 24, 2016 at 08:10:14AM -0400, J. Bruce Fields wrote:
> > On Sun, Jul 24, 2016 at 01:22:06AM +0100, Al Viro wrote:
> > > On Fri, Jul 22, 2016 at 01:48:52PM -0400, J. Bruce Fields wrote:
> > > > From: "J. Bruce Fields" <bfields@redhat.com>
> > > > 
> > > > I'm not sure why this was added.  It doesn't seem necessary, and no
> > > > other caller does this.
> > > 
> > > lookup_one_len() will explode if you call it for non-directory (==
> > > !d_can_lookup(), i.e. something without ->lookup()).  So unless the callers
> > > do guarantee that check being true, it *is* needed.
> > 
> > Both callers call fh_verify(.,.,S_IFDIR,.), so at this point we know
> > that i_mode & S_IFMT == S_IFDIR.  Is there some odd case where that's
> > insufficient?  If so, I think there may be bugs elsewhere in nfsd.  If
> > not, I'll add a note to the changelog.
> 
> First of all, such objects do exist; they probably won't be encountered by
> nfsd and all instances I can think of are not writable, but...
> 
> > Thanks for reminding me to check this, I hadn't thought of that as an
> > "is this a directory" check, it makes more sense now.
> 
> I'd have turned that into d_can_lookup(fhp->fh_dentry), actually.

So would such a check mainly just protect developers from themselves if
they try to make a weird filesystems exportable?

If we need to catch this I'd rather do it in fh_verify, which would
cover some other operations, too.  Maybe like the below.  We could be
nicer and WARN()/error out instead of BUG.  But it's unclear to me
whether this case is worth checking for at all.

--b.

diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index 27250e279c37..372747a00214 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -59,14 +59,17 @@ static int nfsd_acceptable(void *expv, struct dentry *dentry)
  * the write call).
  */
 static inline __be32
-nfsd_mode_check(struct svc_rqst *rqstp, umode_t mode, umode_t requested)
+nfsd_mode_check(struct svc_rqst *rqstp, struct dentry *dentry,
+		umode_t requested)
 {
-	mode &= S_IFMT;
+	umode_t mode = d_inode(dentry)->i_mode & S_IFMT;
 
 	if (requested == 0) /* the caller doesn't care */
 		return nfs_ok;
-	if (mode == requested)
+	if (mode == requested) {
+		BUG_ON(mode == S_IFDIR && !d_can_lookup(dentry));
 		return nfs_ok;
+	}
 	/*
 	 * v4 has an error more specific than err_notdir which we should
 	 * return in preference to err_notdir:
@@ -340,7 +343,7 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, int access)
 	if (error)
 		goto out;
 
-	error = nfsd_mode_check(rqstp, d_inode(dentry)->i_mode, type);
+	error = nfsd_mode_check(rqstp, dentry, type);
 	if (error)
 		goto out;
 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web