knfsd: Fix stale file handle problem with subtree_checking.

The following patch fixes a bug that was introduced since 2.6.17, and should go in 2.6.18.

### Comments for Changeset

A recent patch:

h=7fc90ec93a5eb71f4b08403baf5ba7176b3ec6b1

moved the call to nfsd_setuser out of the 'find a dentry for a filehandle' branch of fh_verify so that it would always be called.

This had the unfortunately side-effect of moving *after* the call to decode_fh, so the prober fsuid was not set when nfsd_acceptable was called, the 'permission' check did the wrong thing.

This patch moves the nfsd_setuser call back where it was, and add as call in the other branch of the if.

### Diffstat output

 ./fs/nfsd/nfsfh.c |   20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff .prev/fs/nfsd/nfsfh.c ./fs/nfsd/nfsfh.c
@@ -187,6 +187,11 @@ fh_verify(struct svc_rqst *rqstp, struct
 			goto out;
 		}
 
+		/* Set user creds for this exportpoint */
+		error = nfserrno(nfsd_setuser(rqstp, exp));
+		if (error)
+			goto out;
+
 		/*
 		 * Look up the dentry using the NFS file handle.
 		 */

@@ -241,16 +246,17 @@ fh_verify(struct svc_rqst *rqstp, struct
 		dprintk("nfsd: fh_verify - just checking\n");
 		dentry = fhp->fh_dentry;
 		exp = fhp->fh_export;
+		/* 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));
+		if (error)
+			goto out;
 	}
 	cache_get(&exp->h);
 
-	/* 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));
-	if (error)
-		goto out;
 
 	error = nfsd_mode_check(rqstp, dentry->d_inode->i_mode, type);
 	if (error)

Argh, sorry, thanks for the fix.

It'd be great if we could deprecate subtree checking some day in the distant future....

Would it be feasible to add filesystem support for some sort of subvolume-like thing that acted like a mountpoint (in the sense that it restricted hardlinks and renames) but that didn't require setting aside a separate partition? I imagine that'd probably do what most people exporting subtrees want without forcing us to do dubious tricks with filehandles.

--b.

The first step would be to stop it from being the default (as Trond has suggested a number of times :-)

How about this.
I release a 1.0.10 shortly which addresses some 'portlist' related breakage and prints a nasty warning if you have neither subtree_check or no_subtree_check, but still defaults to subtree_check.

Then the next release will be 1.1.0 which prints the same warning, but defaults the other way - and probably removed the warning if you include neither sync not async.

That should at least get subtree_check to be used less.

I think it is a great idea for a 'filesystem' to support multiple independent file-trees within the one storage set, which is roughly what you are saying I think (though probably not quite).

However I suspect that most people don't actually want subtrees. They just get it as the default. It isn't something that I would have implemented if I hadn't inherited the requirement, and no other OS that I know of provides that particular semantic.

Maybe we should make it non-default, and then in one year, subtly break it (maybe reintroduce this bug) and see if anyone notices :-) (No, that's unethical, I wouldn't do that - really ....)

NeilBrown

Sounds good to me. (Though for these kinds of changes I suppose it's the time elasped that matters more than the number of released versions--people probably upgrade every x months/years/whatever rather than every x versions. By that criteria I think we might be making the subtree_check change a little fast, while the warning period for the sync change may already be overkill....)

Could be.

--b.