nfsd bugfixes

nfsd bugfixes

[J. Bruce Fields]

The following two patches are nfsd bugfixes that I believe are
appropriate for 2.6.24 and 2.6.23.y.

--b.

[PATCH] knfsd: fix spurious EINVAL errors on first access of new filesystem

[J. Bruce Fields]

The v2/v3 acl code in nfsd is translating any return from fh_verify() to
nfserr_inval.  This is particularly unfortunate in the case of an
nfserr_dropit return, which is an internal error meant to indicate to
callers that this request has been deferred and should just be dropped
pending the results of an upcall to mountd.

Thanks to Roland <devzero@web.de> for bug report and data collection.

Cc: Roland <devzero@web.de>
Acked-by: Andreas Gruenbacher <agruen@suse.de>
Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 fs/nfsd/nfs2acl.c |    2 +-
 fs/nfsd/nfs3acl.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs2acl.c b/fs/nfsd/nfs2acl.c
index b617428..0e5fa11 100644
--- a/fs/nfsd/nfs2acl.c
+++ b/fs/nfsd/nfs2acl.c
@@ -41,7 +41,7 @@ static __be32 nfsacld_proc_getacl(struct svc_rqst * rqstp,
 
 	fh = fh_copy(&resp->fh, &argp->fh);
 	if ((nfserr = fh_verify(rqstp, &resp->fh, 0, MAY_NOP)))
-		RETURN_STATUS(nfserr_inval);
+		RETURN_STATUS(nfserr);
 
 	if (argp->mask & ~(NFS_ACL|NFS_ACLCNT|NFS_DFACL|NFS_DFACLCNT))
 		RETURN_STATUS(nfserr_inval);
diff --git a/fs/nfsd/nfs3acl.c b/fs/nfsd/nfs3acl.c
index 3e3f2de..b647f2f 100644
--- a/fs/nfsd/nfs3acl.c
+++ b/fs/nfsd/nfs3acl.c
@@ -37,7 +37,7 @@ static __be32 nfsd3_proc_getacl(struct svc_rqst * rqstp,
 
 	fh = fh_copy(&resp->fh, &argp->fh);
 	if ((nfserr = fh_verify(rqstp, &resp->fh, 0, MAY_NOP)))
-		RETURN_STATUS(nfserr_inval);
+		RETURN_STATUS(nfserr);
 
 	if (argp->mask & ~(NFS_ACL|NFS_ACLCNT|NFS_DFACL|NFS_DFACLCNT))
 		RETURN_STATUS(nfserr_inval);
-- 
1.5.3.5.561.g140d

[PATCH] nfsd4: recheck for secure ports in fh_verify

[J. Bruce Fields]

As with

	7fc90ec93a5eb71f4b08... "call nfsd_setuser() on fh_compose()..."

this is a case where we need to redo a security check in fh_verify()
even though the filehandle already has an associated dentry--if the
filehandle was created by fh_compose() in an earlier operation of the
nfsv4 compound, then we may not have done these checks yet.

Without this fix it is possible, for example, to traverse from an export
without the secure ports requirement to one with it in a single
compound, and bypass the secure port check on the new export.

While we're here, fix up some minor style problems and change a printk()
to a dprintk(), to make it harder for random unprivileged users to spam
the logs.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 fs/nfsd/nfsfh.c |   43 ++++++++++++++++++++++++++-----------------
 1 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index 4f712e9..468f17a 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -95,6 +95,22 @@ nfsd_mode_check(struct svc_rqst *rqstp, umode_t mode, int type)
 	return 0;
 }
 
+static __be32 nfsd_setuser_and_check_port(struct svc_rqst *rqstp,
+					  struct svc_export *exp)
+{
+	/* Check if the request originated from a secure port. */
+	if (!rqstp->rq_secure && EX_SECURE(exp)) {
+		char buf[RPC_MAX_ADDRBUFLEN];
+		dprintk(KERN_WARNING
+		       "nfsd: request from insecure port %s!\n",
+		       svc_print_addr(rqstp, buf, sizeof(buf)));
+		return nfserr_perm;
+	}
+
+	/* Set user creds for this exportpoint */
+	return nfserrno(nfsd_setuser(rqstp, exp));
+}
+
 /*
  * Perform sanity checks on the dentry in a client's file handle.
  *
@@ -167,18 +183,7 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, int access)
 			goto out;
 		}
 
-		/* Check if the request originated from a secure port. */
-		error = nfserr_perm;
-		if (!rqstp->rq_secure && EX_SECURE(exp)) {
-			char buf[RPC_MAX_ADDRBUFLEN];
-			printk(KERN_WARNING
-			       "nfsd: request from insecure port %s!\n",
-			       svc_print_addr(rqstp, buf, sizeof(buf)));
-			goto out;
-		}
-
-		/* Set user creds for this exportpoint */
-		error = nfserrno(nfsd_setuser(rqstp, exp));
+		error = nfsd_setuser_and_check_port(rqstp, exp);
 		if (error)
 			goto out;
 
@@ -227,18 +232,22 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, int access)
 		fhp->fh_export = exp;
 		nfsd_nr_verified++;
 	} else {
-		/* just rechecking permissions
-		 * (e.g. nfsproc_create calls fh_verify, then nfsd_create does as well)
+		/*
+		 * just rechecking permissions
+		 * (e.g. nfsproc_create calls fh_verify, then nfsd_create
+		 * does as well)
 		 */
 		dprintk("nfsd: fh_verify - just checking\n");
 		dentry = fhp->fh_dentry;
 		exp = fhp->fh_export;
-		/* Set user creds for this exportpoint; necessary even
+		/*
+		 * Set user creds for this exportpoint; necessary even
 		 * in the "just checking" case because this may be a
 		 * filehandle that was created by fh_compose, and that
 		 * is about to be used in another nfsv4 compound
-		 * operation */
-		error = nfserrno(nfsd_setuser(rqstp, exp));
+		 * operation.
+		 */
+		error = nfsd_setuser_and_check_port(rqstp, exp);
 		if (error)
 			goto out;
 	}
-- 
1.5.3.5.561.g140d

Re: nfsd bugfixes

[Neil Brown]

On Monday November 12, bfields@citi.umich.edu wrote:
> The following two patches are nfsd bugfixes that I believe are
> appropriate for 2.6.24 and 2.6.23.y.
> 
> --b.
> 

Both
  Reviewed-By: NeilBrown <neilb@suse.de>

Calling nfsd_setuser an extra time does open us up for a very tiny
possibility of an ENOMEM at an awkward time.
We could remove that entirely for NFSEXP_ALLSQUASH by allocating an
empty group_info at export time and just using a reference to that.
It would be more awkward to pre-allocate for the NFSEXP_ROOTSQUASH
case as the group_info has to be at least as big as the one in the RPC
request, and would could need a different one of each concurrent
request.... not sure if that is worth "fixing".

Thanks,
NeilBrown

Re: nfsd bugfixes

[J. Bruce Fields]

On Tue, Nov 13, 2007 at 09:08:42AM +1100, Neil Brown wrote:
> On Monday November 12, bfields@citi.umich.edu wrote:
> > The following two patches are nfsd bugfixes that I believe are
> > appropriate for 2.6.24 and 2.6.23.y.
> > 
> > --b.
> > 
> 
> Both
>   Reviewed-By: NeilBrown <neilb@suse.de>

Thanks, Neil.

> Calling nfsd_setuser an extra time does open us up for a very tiny
> possibility of an ENOMEM at an awkward time.

Hm.  Could you give an example of possible consequences?

(Though note this is somewhat of a separate discussion, since this
particular patch doesn't add a call to nfsd_setuser().)

> We could remove that entirely for NFSEXP_ALLSQUASH by allocating an
> empty group_info at export time and just using a reference to that.
> It would be more awkward to pre-allocate for the NFSEXP_ROOTSQUASH
> case as the group_info has to be at least as big as the one in the RPC
> request, and would could need a different one of each concurrent
> request.... not sure if that is worth "fixing".

--b.

Re: nfsd bugfixes

[Neil Brown]

(CC: trimmed - as Bruce says: separate discussion)

On Monday November 12, bfields@fieldses.org wrote:
> On Tue, Nov 13, 2007 at 09:08:42AM +1100, Neil Brown wrote:
> > Calling nfsd_setuser an extra time does open us up for a very tiny
> > possibility of an ENOMEM at an awkward time.
> 
> Hm.  Could you give an example of possible consequences?

Just that you could get an ENOMEM in the middle of a NFSv4 COMPOUND.
I guess that should result in NFSERR_RESOURCE and we just hope the
client is able to cope and resend the remainder of the compound.
Though looking at the code, ENOMEM becomes nfserr_dropit... does that
mean the we would drop the whole request and the client would need to
resend, possibly duplicating non-idempotent portions?

Mainly, it just feels unclean.

> 
> (Though note this is somewhat of a separate discussion, since this
> particular patch doesn't add a call to nfsd_setuser().)

Hmm, you are right, we already call nfsd_setuser in both paths, you we
just adding the check for privileged port - doh ;-)

NeilBrown

Re: nfsd bugfixes

[J. Bruce Fields]

On Tue, Nov 13, 2007 at 09:35:01AM +1100, Neil Brown wrote:
> 
> (CC: trimmed - as Bruce says: separate discussion)
> 
> On Monday November 12, bfields@fieldses.org wrote:
> > On Tue, Nov 13, 2007 at 09:08:42AM +1100, Neil Brown wrote:
> > > Calling nfsd_setuser an extra time does open us up for a very tiny
> > > possibility of an ENOMEM at an awkward time.
> > 
> > Hm.  Could you give an example of possible consequences?
> 
> Just that you could get an ENOMEM in the middle of a NFSv4 COMPOUND.
> I guess that should result in NFSERR_RESOURCE and we just hope the
> client is able to cope and resend the remainder of the compound.
> Though looking at the code, ENOMEM becomes nfserr_dropit... does that
> mean the we would drop the whole request and the client would need to
> resend, possibly duplicating non-idempotent portions?

Yeah, OK, that's another one for my list of compound processing problems
to worry about.  (Others:

	- a deferral for an export upcall can similarly lead to
	  incorrect behavior on nonidempotent operations; if we could
	  fix this we could also eliminate the nfs4idmap.c special case,
	  which would probably enable some other cleanup.  I've made a
	  couple half-hearted attempts to fix this but don't have
	  anything finished yet.

	- We've still got the reply cache turned off for nfsv4!  I think
	  it was originally turned off just because whoever did it
	  didn't want to figure out which compounds to cache.  That's
	  probably not hard to fix, but last I checked I didn't think
	  our reply cache actually helped much in the tcp case (the only
	  case we care about).  So that needs to be fixed first.  I've
	  done no work on this yet.

)--b.