[PATCH] knfsd: Fix stale file handle problem with subtree_checking.

[PATCH] knfsd: Fix stale file handle problem with subtree_checking.

[NeilBrown]

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.

Cc: "J. Bruce Fields" <bfields@fieldses.org>
Signed-off-by: Neil Brown <neilb@suse.de>

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

diff .prev/fs/nfsd/nfsfh.c ./fs/nfsd/nfsfh.c
--- .prev/fs/nfsd/nfsfh.c	2006-07-28 19:14:17.000000000 +1000
+++ ./fs/nfsd/nfsfh.c	2006-07-28 19:14:36.000000000 +1000
@@ -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)

Re: [PATCH] knfsd: Fix stale file handle problem with subtree_checking.

[J. Bruce Fields]

On Fri, Jul 28, 2006 at 07:42:55PM +1000, NeilBrown wrote:
> 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.

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.

Re: [PATCH] knfsd: Fix stale file handle problem with subtree_checking.

[Neil Brown]

On Friday July 28, bfields@fieldses.org wrote:
> 
> It'd be great if we could deprecate subtree checking some day in the
> distant future....

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.

> 
> 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.

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

Re: [PATCH] knfsd: Fix stale file handle problem with subtree_checking.

[J. Bruce Fields]

On Sat, Jul 29, 2006 at 03:29:33PM +1000, Neil Brown wrote:
> 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.

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....)

> 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.

Could be.

--b.