reconnect_path() breaks NFS server causing occasional EACCES

reconnect_path() breaks NFS server causing occasional EACCES

[Frank van Maarseveen]

Occasionally we experience EACCES errors which are caused by the exportfs
reconnect_path() function called by the NFS server (NFSv3). The following
reproduces it reliably on the client.

Compile cd-droppriv.c and make it setuid root:
--------
#include <stdio.h>
#include <stdarg.h>
#include <unistd.h>
#include <string.h>
#include <stdlib.h>
#include <errno.h>
#include <dirent.h>

void die(const char *fmt, ...) __attribute__((format(printf, 1, 2), noreturn));
void die(const char *fmt, ...)
{
	va_list ap;

	va_start(ap, fmt);
	fprintf(stderr, "cd-droppriv: ");
	vfprintf(stderr, fmt, ap);
	va_end(ap);
	exit(1);
}

int main(int argc, char **argv)
{
	if (chdir(argv[1]) == -1)
		die("chdir: %s\n", strerror(errno));
	if (setuid(getuid()) == -1)
		die("setuid: %s\n", strerror(errno));
	printf("Restart the NFS server then press <enter>.\n");
	getchar();
	if (opendir(".") == NULL)
		die("opendir: %s\n", strerror(errno));
	return 0;
}
--------

As root, create a directory tree on the client. The export options on
the server for /mnt include no_root_squash and no_subtree_check:

	mkdir -p /mnt/a/b
	chmod 700 /mnt/a
	chmod 777 /mnt/a/b

Run the program as non-root on the client:

	cd-droppriv /mnt/a/b

and press <enter>. When the server is restarted before pressing <enter>
opendir() fails with EACCES:

	cd-droppriv: opendir: Permission denied

This happens too when dentries are dropped on the server due to memory
pressure. The following seems to fix the problem (2.6.24.4):

--- ./fs/exportfs/expfs.c.orig	2008-02-04 14:24:21.000000000 +0100
+++ ./fs/exportfs/expfs.c	2008-04-03 18:00:20.000000000 +0200
@@ -170,7 +170,7 @@ reconnect_path(struct vfsmount *mnt, str
 			}
 			dprintk("%s: found name: %s\n", __FUNCTION__, nbuf);
 			mutex_lock(&ppd->d_inode->i_mutex);
-			npd = lookup_one_len(nbuf, ppd, strlen(nbuf));
+			npd = lookup_one_noperm(nbuf, ppd);
 			mutex_unlock(&ppd->d_inode->i_mutex);
 			if (IS_ERR(npd)) {
 				err = PTR_ERR(npd);
@@ -447,8 +447,7 @@ struct dentry *exportfs_decode_fh(struct
 		err = exportfs_get_name(mnt, target_dir, nbuf, result);
 		if (!err) {
 			mutex_lock(&target_dir->d_inode->i_mutex);
-			nresult = lookup_one_len(nbuf, target_dir,
-						 strlen(nbuf));
+			nresult = lookup_one_noperm(nbuf, target_dir);
 			mutex_unlock(&target_dir->d_inode->i_mutex);
 			if (!IS_ERR(nresult)) {
 				if (nresult->d_inode) {

-- 
Frank

Re: reconnect_path() breaks NFS server causing occasional EACCES

[J. Bruce Fields]

Thanks for the patch and the nice test case....

On Fri, Apr 04, 2008 at 12:24:49PM +0200, Frank van Maarseveen wrote:
> Occasionally we experience EACCES errors which are caused by the exportfs
> reconnect_path() function called by the NFS server (NFSv3). The following
> reproduces it reliably on the client.

OK, so you're trying to use a directory that you previously descended
into, but that you no longer have the right to look up?

> 
> Compile cd-droppriv.c and make it setuid root:
> --------
> #include <stdio.h>
> #include <stdarg.h>
> #include <unistd.h>
> #include <string.h>
> #include <stdlib.h>
> #include <errno.h>
> #include <dirent.h>
> 
> void die(const char *fmt, ...) __attribute__((format(printf, 1, 2), noreturn));
> void die(const char *fmt, ...)
> {
> 	va_list ap;
> 
> 	va_start(ap, fmt);
> 	fprintf(stderr, "cd-droppriv: ");
> 	vfprintf(stderr, fmt, ap);
> 	va_end(ap);
> 	exit(1);
> }
> 
> int main(int argc, char **argv)
> {
> 	if (chdir(argv[1]) == -1)
> 		die("chdir: %s\n", strerror(errno));
> 	if (setuid(getuid()) == -1)
> 		die("setuid: %s\n", strerror(errno));
> 	printf("Restart the NFS server then press <enter>.\n");
> 	getchar();
> 	if (opendir(".") == NULL)
> 		die("opendir: %s\n", strerror(errno));
> 	return 0;
> }
> --------
> 
> As root, create a directory tree on the client. The export options on
> the server for /mnt include no_root_squash and no_subtree_check:
> 
> 	mkdir -p /mnt/a/b
> 	chmod 700 /mnt/a
> 	chmod 777 /mnt/a/b
> 
> Run the program as non-root on the client:
> 
> 	cd-droppriv /mnt/a/b
> 
> and press <enter>. When the server is restarted before pressing <enter>
> opendir() fails with EACCES:
> 
> 	cd-droppriv: opendir: Permission denied
> 
> This happens too when dentries are dropped on the server due to memory
> pressure. The following seems to fix the problem (2.6.24.4):
> 
> --- ./fs/exportfs/expfs.c.orig	2008-02-04 14:24:21.000000000 +0100
> +++ ./fs/exportfs/expfs.c	2008-04-03 18:00:20.000000000 +0200
> @@ -170,7 +170,7 @@ reconnect_path(struct vfsmount *mnt, str
>  			}
>  			dprintk("%s: found name: %s\n", __FUNCTION__, nbuf);
>  			mutex_lock(&ppd->d_inode->i_mutex);
> -			npd = lookup_one_len(nbuf, ppd, strlen(nbuf));
> +			npd = lookup_one_noperm(nbuf, ppd);

Anyone who depends on the "x" bit to control access to objects in an
nfs-exported filesystem is already in trouble.  We could do so for
directories (at the expense of non-posix-like behavior such as what
you've seen), but we probably can't for files.  So I'm inclined to think
this is the right thing to do.

The "DON'T USE THIS FUNCTION EVER, thanks." suggests we should at least
consult the person who added that comment (cc'd) before adding a call to
lookup_one_noperm().  (And if we decide to do this, we should make a
note of this in that comment.)

Also, I'm curious: could you explain how you hit this In Real Life,
before you made this test case?

--b.

>  			mutex_unlock(&ppd->d_inode->i_mutex);
>  			if (IS_ERR(npd)) {
>  				err = PTR_ERR(npd);
> @@ -447,8 +447,7 @@ struct dentry *exportfs_decode_fh(struct
>  		err = exportfs_get_name(mnt, target_dir, nbuf, result);
>  		if (!err) {
>  			mutex_lock(&target_dir->d_inode->i_mutex);
> -			nresult = lookup_one_len(nbuf, target_dir,
> -						 strlen(nbuf));
> +			nresult = lookup_one_noperm(nbuf, target_dir);
>  			mutex_unlock(&target_dir->d_inode->i_mutex);
>  			if (!IS_ERR(nresult)) {
>  				if (nresult->d_inode) {
> 
> -- 
> Frank
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: reconnect_path() breaks NFS server causing occasional EACCES

[Frank van Maarseveen]

On Mon, Apr 07, 2008 at 02:43:46PM -0400, J. Bruce Fields wrote:
> Thanks for the patch and the nice test case....
> 
> On Fri, Apr 04, 2008 at 12:24:49PM +0200, Frank van Maarseveen wrote:
> > Occasionally we experience EACCES errors which are caused by the exportfs
> > reconnect_path() function called by the NFS server (NFSv3). The following
> > reproduces it reliably on the client.
> 
> OK, so you're trying to use a directory that you previously descended
> into, but that you no longer have the right to look up?

Correct. Based on identity and permission bits the process should have
access but it no longer has.

> 
> > 
> > Compile cd-droppriv.c and make it setuid root:
> > --------
> > #include <stdio.h>
> > #include <stdarg.h>
> > #include <unistd.h>
> > #include <string.h>
> > #include <stdlib.h>
> > #include <errno.h>
> > #include <dirent.h>
> > 
> > void die(const char *fmt, ...) __attribute__((format(printf, 1, 2), noreturn));
> > void die(const char *fmt, ...)
> > {
> > 	va_list ap;
> > 
> > 	va_start(ap, fmt);
> > 	fprintf(stderr, "cd-droppriv: ");
> > 	vfprintf(stderr, fmt, ap);
> > 	va_end(ap);
> > 	exit(1);
> > }
> > 
> > int main(int argc, char **argv)
> > {
> > 	if (chdir(argv[1]) == -1)
> > 		die("chdir: %s\n", strerror(errno));
> > 	if (setuid(getuid()) == -1)
> > 		die("setuid: %s\n", strerror(errno));
> > 	printf("Restart the NFS server then press <enter>.\n");
> > 	getchar();
> > 	if (opendir(".") == NULL)
> > 		die("opendir: %s\n", strerror(errno));
> > 	return 0;
> > }
> > --------
> > 
> > As root, create a directory tree on the client. The export options on
> > the server for /mnt include no_root_squash and no_subtree_check:
> > 
> > 	mkdir -p /mnt/a/b
> > 	chmod 700 /mnt/a
> > 	chmod 777 /mnt/a/b
> > 
> > Run the program as non-root on the client:
> > 
> > 	cd-droppriv /mnt/a/b
> > 
> > and press <enter>. When the server is restarted before pressing <enter>
> > opendir() fails with EACCES:
> > 
> > 	cd-droppriv: opendir: Permission denied
> > 
> > This happens too when dentries are dropped on the server due to memory
> > pressure. The following seems to fix the problem (2.6.24.4):
> > 
> > --- ./fs/exportfs/expfs.c.orig	2008-02-04 14:24:21.000000000 +0100
> > +++ ./fs/exportfs/expfs.c	2008-04-03 18:00:20.000000000 +0200
> > @@ -170,7 +170,7 @@ reconnect_path(struct vfsmount *mnt, str
> >  			}
> >  			dprintk("%s: found name: %s\n", __FUNCTION__, nbuf);
> >  			mutex_lock(&ppd->d_inode->i_mutex);
> > -			npd = lookup_one_len(nbuf, ppd, strlen(nbuf));
> > +			npd = lookup_one_noperm(nbuf, ppd);
> 
> Anyone who depends on the "x" bit to control access to objects in an
> nfs-exported filesystem is already in trouble.  We could do so for
> directories (at the expense of non-posix-like behavior such as what
> you've seen), but we probably can't for files.  So I'm inclined to think
> this is the right thing to do.

Several file access issues have been fixed in the past. Redirecting
output of setuid-root programs (X server) when squash_root is in effect
and paging in execute-only files over NFS comes to my mind.

> 
> The "DON'T USE THIS FUNCTION EVER, thanks." suggests we should at least
> consult the person who added that comment (cc'd) before adding a call to
> lookup_one_noperm().  (And if we decide to do this, we should make a
> note of this in that comment.)

yes.

> 
> Also, I'm curious: could you explain how you hit this In Real Life,
> before you made this test case?

The NFS patch to deal with >16 groups on NFSv3 with AUTH_SYS I maintain
on www.frankvm.com/nfs-ngroups hit this problem first. Interactive shells
became unusable after a nightly run server mirroring script expelled
a lot of dentries from memory. The nfs-ngroups patch makes the client
picky about which groups to use in NFS requests in order to avoid the
16 groups limit.

> 
> --b.
> 
> >  			mutex_unlock(&ppd->d_inode->i_mutex);
> >  			if (IS_ERR(npd)) {
> >  				err = PTR_ERR(npd);
> > @@ -447,8 +447,7 @@ struct dentry *exportfs_decode_fh(struct
> >  		err = exportfs_get_name(mnt, target_dir, nbuf, result);
> >  		if (!err) {
> >  			mutex_lock(&target_dir->d_inode->i_mutex);
> > -			nresult = lookup_one_len(nbuf, target_dir,
> > -						 strlen(nbuf));
> > +			nresult = lookup_one_noperm(nbuf, target_dir);
> >  			mutex_unlock(&target_dir->d_inode->i_mutex);
> >  			if (!IS_ERR(nresult)) {
> >  				if (nresult->d_inode) {
> > 
> > -- 
> > Frank
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Frank

Re: reconnect_path() breaks NFS server causing occasional EACCES

[Christoph Hellwig]

On Mon, Apr 07, 2008 at 02:43:46PM -0400, J. Bruce Fields wrote:
> Anyone who depends on the "x" bit to control access to objects in an
> nfs-exported filesystem is already in trouble.  We could do so for
> directories (at the expense of non-posix-like behavior such as what
> you've seen), but we probably can't for files.  So I'm inclined to think
> this is the right thing to do.
> 
> The "DON'T USE THIS FUNCTION EVER, thanks." suggests we should at least
> consult the person who added that comment (cc'd) before adding a call to
> lookup_one_noperm().  (And if we decide to do this, we should make a
> note of this in that comment.)

That function really shouldn't be used and we should obey the x bit.
And yes, due to NFSs staleless file handles this will lead to non-posix
behaviour which is expected.  The same will happen with other nfs
servers aswell.

Re: reconnect_path() breaks NFS server causing occasional EACCES

[Frank van Maarseveen]

On Wed, Apr 09, 2008 at 03:36:39PM +0200, Christoph Hellwig wrote:
> On Mon, Apr 07, 2008 at 02:43:46PM -0400, J. Bruce Fields wrote:
> > Anyone who depends on the "x" bit to control access to objects in an
> > nfs-exported filesystem is already in trouble.  We could do so for
> > directories (at the expense of non-posix-like behavior such as what
> > you've seen), but we probably can't for files.  So I'm inclined to think
> > this is the right thing to do.
> > 
> > The "DON'T USE THIS FUNCTION EVER, thanks." suggests we should at least
> > consult the person who added that comment (cc'd) before adding a call to
> > lookup_one_noperm().  (And if we decide to do this, we should make a
> > note of this in that comment.)
> 
> That function really shouldn't be used and we should obey the x bit.
> And yes, due to NFSs staleless file handles this will lead to non-posix
> behaviour which is expected.  The same will happen with other nfs
> servers aswell.

The server exhibits non-posix behavior only occasionally and particularly
this "randomness" is bad. Random failure once every few days is hard to
figure out.

Whether we want to be posix compliant with directory handles is a separate
issue but I think we should. And we (mostly) are.

The proposed patch fixes the linux NFS server behavior to be consistent
over reboots and high memory pressure. It's not a security issue so I
don't see a downside.

-- 
Frank

Re: reconnect_path() breaks NFS server causing occasional EACCES

[J. Bruce Fields]

On Wed, Apr 09, 2008 at 03:36:39PM +0200, Christoph Hellwig wrote:
> On Mon, Apr 07, 2008 at 02:43:46PM -0400, J. Bruce Fields wrote:
> > Anyone who depends on the "x" bit to control access to objects in an
> > nfs-exported filesystem is already in trouble.  We could do so for
> > directories (at the expense of non-posix-like behavior such as what
> > you've seen), but we probably can't for files.  So I'm inclined to think
> > this is the right thing to do.
> > 
> > The "DON'T USE THIS FUNCTION EVER, thanks." suggests we should at least
> > consult the person who added that comment (cc'd) before adding a call to
> > lookup_one_noperm().  (And if we decide to do this, we should make a
> > note of this in that comment.)
> 
> That function really shouldn't be used and we should obey the x bit.
> And yes, due to NFSs staleless file handles this will lead to non-posix
> behaviour which is expected.  The same will happen with other nfs
> servers aswell.

Any references for that behavior?  Well, we can do some tests, I guess.

--b.

Re: reconnect_path() breaks NFS server causing occasional EACCES

[Neil Brown]

On Wednesday April 9, hch@lst.de wrote:
> On Mon, Apr 07, 2008 at 02:43:46PM -0400, J. Bruce Fields wrote:
> > Anyone who depends on the "x" bit to control access to objects in an
> > nfs-exported filesystem is already in trouble.  We could do so for
> > directories (at the expense of non-posix-like behavior such as what
> > you've seen), but we probably can't for files.  So I'm inclined to think
> > this is the right thing to do.
> > 
> > The "DON'T USE THIS FUNCTION EVER, thanks." suggests we should at least
> > consult the person who added that comment (cc'd) before adding a call to
> > lookup_one_noperm().  (And if we decide to do this, we should make a
> > note of this in that comment.)
> 
> That function really shouldn't be used and we should obey the x bit.
> And yes, due to NFSs staleless file handles this will lead to non-posix
> behaviour which is expected.  The same will happen with other nfs
> servers aswell.

For the record, I disagree.  I think it is perfectly appropriate to
use this function.  I think that obeying the 'x' bit is wrong.

Why?

What we are doing here is reconstructing the dcache to correctly
reflect the filesystem.  The reason that we need to do this (rather
than just leaving the dentry disconnected as we sometimes do with
files) is so that lock_rename can find valid d_parent pointers and can
guard against certain directory rename races that might create
disconnected loops.

i.e. the look_one_* is not being done on behalf of the owner of the
file, or of the group-owner of the file, or of anyone else.  It is
being done on behalf of the filesystem to ensure future filesystem
consistency.
So none of the 'x' bits (owner, group-owner, world) is appropriate to
validate this lookup.

NeilBrown

Re: reconnect_path() breaks NFS server causing occasional EACCES

[J. Bruce Fields]

On Tue, Apr 29, 2008 at 03:20:30PM +1000, Neil Brown wrote:
> On Wednesday April 9, hch@lst.de wrote:
> > On Mon, Apr 07, 2008 at 02:43:46PM -0400, J. Bruce Fields wrote:
> > > Anyone who depends on the "x" bit to control access to objects in an
> > > nfs-exported filesystem is already in trouble.  We could do so for
> > > directories (at the expense of non-posix-like behavior such as what
> > > you've seen), but we probably can't for files.  So I'm inclined to think
> > > this is the right thing to do.
> > > 
> > > The "DON'T USE THIS FUNCTION EVER, thanks." suggests we should at least
> > > consult the person who added that comment (cc'd) before adding a call to
> > > lookup_one_noperm().  (And if we decide to do this, we should make a
> > > note of this in that comment.)
> > 
> > That function really shouldn't be used and we should obey the x bit.
> > And yes, due to NFSs staleless file handles this will lead to non-posix
> > behaviour which is expected.  The same will happen with other nfs
> > servers aswell.
> 
> For the record, I disagree.  I think it is perfectly appropriate to
> use this function.  I think that obeying the 'x' bit is wrong.
> 
> Why?
> 
> What we are doing here is reconstructing the dcache to correctly
> reflect the filesystem.  The reason that we need to do this (rather
> than just leaving the dentry disconnected as we sometimes do with
> files) is so that lock_rename can find valid d_parent pointers and can
> guard against certain directory rename races that might create
> disconnected loops.
> 
> i.e. the look_one_* is not being done on behalf of the owner of the
> file, or of the group-owner of the file, or of anyone else.  It is
> being done on behalf of the filesystem to ensure future filesystem
> consistency.
> So none of the 'x' bits (owner, group-owner, world) is appropriate to
> validate this lookup.

Just to make sure I understand--you're not claiming that there's an
actual threat of corrupting the on-disk filesystem or in-core data
structures, right? 

I understand the "this isn't being done as any particular user"
argument.

It also seems to me that the actual security value of these checks is
very low, given that all they're likely to do is raise the cost of a
filehandle-guessing attack slightly, without really eliminating it.

And from the point of a user the current behavior seems likely to lead
to difficult-to-analyse behavior.

So I don't understand Christoph's objection yet either.

It might also help if we could confirm or deny Christoph's assertion
about the behavior of other nfs servers.  It shouldn't be hard to run
the original test case

	http://marc.info/?l=linux-nfs&m=120730475719642&w=2

against another server or two.

---b.

Re: reconnect_path() breaks NFS server causing occasional EACCES

[Frank van Maarseveen]

On Tue, Apr 29, 2008 at 12:35:54PM -0400, J. Bruce Fields wrote:
> On Tue, Apr 29, 2008 at 03:20:30PM +1000, Neil Brown wrote:
> > On Wednesday April 9, hch@lst.de wrote:
> > > On Mon, Apr 07, 2008 at 02:43:46PM -0400, J. Bruce Fields wrote:
> > > > Anyone who depends on the "x" bit to control access to objects in an
> > > > nfs-exported filesystem is already in trouble.  We could do so for
> > > > directories (at the expense of non-posix-like behavior such as what
> > > > you've seen), but we probably can't for files.  So I'm inclined to think
> > > > this is the right thing to do.
> > > > 
> > > > The "DON'T USE THIS FUNCTION EVER, thanks." suggests we should at least
> > > > consult the person who added that comment (cc'd) before adding a call to
> > > > lookup_one_noperm().  (And if we decide to do this, we should make a
> > > > note of this in that comment.)
> > > 
> > > That function really shouldn't be used and we should obey the x bit.
> > > And yes, due to NFSs staleless file handles this will lead to non-posix
> > > behaviour which is expected.  The same will happen with other nfs
> > > servers aswell.
> > 
> > For the record, I disagree.  I think it is perfectly appropriate to
> > use this function.  I think that obeying the 'x' bit is wrong.
> > 
> > Why?
> > 
> > What we are doing here is reconstructing the dcache to correctly
> > reflect the filesystem.  The reason that we need to do this (rather
> > than just leaving the dentry disconnected as we sometimes do with
> > files) is so that lock_rename can find valid d_parent pointers and can
> > guard against certain directory rename races that might create
> > disconnected loops.
> > 
> > i.e. the look_one_* is not being done on behalf of the owner of the
> > file, or of the group-owner of the file, or of anyone else.  It is
> > being done on behalf of the filesystem to ensure future filesystem
> > consistency.
> > So none of the 'x' bits (owner, group-owner, world) is appropriate to
> > validate this lookup.

Ok, so this is another reason why the x-bit check is incorrect. Sounds like
it could cause improper behavior in other cases.

> 
> Just to make sure I understand--you're not claiming that there's an
> actual threat of corrupting the on-disk filesystem or in-core data
> structures, right? 
> 
> I understand the "this isn't being done as any particular user"
> argument.
> 
> It also seems to me that the actual security value of these checks is
> very low, given that all they're likely to do is raise the cost of a
> filehandle-guessing attack slightly, without really eliminating it.
> 
> And from the point of a user the current behavior seems likely to lead
> to difficult-to-analyse behavior.
> 
> So I don't understand Christoph's objection yet either.
> 
> It might also help if we could confirm or deny Christoph's assertion
> about the behavior of other nfs servers.  It shouldn't be hard to run
> the original test case
> 
> 	http://marc.info/?l=linux-nfs&m=120730475719642&w=2
> 
> against another server or two.

I don't think it is that relevant for two reasons:

* Currently, the linux NFS server behaves inconsistent depending
  on memory pressure/reboot. That on its own looks like a bug to me.

* The NFS server should never re-check access to a [directory]
  path which has been used in the past to obtain a handle:
  Permission checks apply only once, at open time. This is how
  POSIX filesystems behave.

I also guess it will be hard to find another NFS server with exactly the
same behavior. Either the x-bit of the complete path to the file-handle
is checked upon every operation (sounds totally broken to me) or it is
never checked beyond open time. It looks like this bug has everything
to do with the internal housekeeping and is certainly not intentional.

-- 
Frank

Re: reconnect_path() breaks NFS server causing occasional EACCES

[J. Bruce Fields]

On Tue, Apr 29, 2008 at 07:40:04PM +0200, Frank van Maarseveen wrote:
> On Tue, Apr 29, 2008 at 12:35:54PM -0400, J. Bruce Fields wrote:
> > On Tue, Apr 29, 2008 at 03:20:30PM +1000, Neil Brown wrote:
> > > On Wednesday April 9, hch@lst.de wrote:
> > > > On Mon, Apr 07, 2008 at 02:43:46PM -0400, J. Bruce Fields wrote:
> > > > > Anyone who depends on the "x" bit to control access to objects in an
> > > > > nfs-exported filesystem is already in trouble.  We could do so for
> > > > > directories (at the expense of non-posix-like behavior such as what
> > > > > you've seen), but we probably can't for files.  So I'm inclined to think
> > > > > this is the right thing to do.
> > > > > 
> > > > > The "DON'T USE THIS FUNCTION EVER, thanks." suggests we should at least
> > > > > consult the person who added that comment (cc'd) before adding a call to
> > > > > lookup_one_noperm().  (And if we decide to do this, we should make a
> > > > > note of this in that comment.)
> > > > 
> > > > That function really shouldn't be used and we should obey the x bit.
> > > > And yes, due to NFSs staleless file handles this will lead to non-posix
> > > > behaviour which is expected.  The same will happen with other nfs
> > > > servers aswell.
> > > 
> > > For the record, I disagree.  I think it is perfectly appropriate to
> > > use this function.  I think that obeying the 'x' bit is wrong.
> > > 
> > > Why?
> > > 
> > > What we are doing here is reconstructing the dcache to correctly
> > > reflect the filesystem.  The reason that we need to do this (rather
> > > than just leaving the dentry disconnected as we sometimes do with
> > > files) is so that lock_rename can find valid d_parent pointers and can
> > > guard against certain directory rename races that might create
> > > disconnected loops.
> > > 
> > > i.e. the look_one_* is not being done on behalf of the owner of the
> > > file, or of the group-owner of the file, or of anyone else.  It is
> > > being done on behalf of the filesystem to ensure future filesystem
> > > consistency.
> > > So none of the 'x' bits (owner, group-owner, world) is appropriate to
> > > validate this lookup.
> 
> Ok, so this is another reason why the x-bit check is incorrect. Sounds like
> it could cause improper behavior in other cases.
> 
> > 
> > Just to make sure I understand--you're not claiming that there's an
> > actual threat of corrupting the on-disk filesystem or in-core data
> > structures, right? 
> > 
> > I understand the "this isn't being done as any particular user"
> > argument.
> > 
> > It also seems to me that the actual security value of these checks is
> > very low, given that all they're likely to do is raise the cost of a
> > filehandle-guessing attack slightly, without really eliminating it.
> > 
> > And from the point of a user the current behavior seems likely to lead
> > to difficult-to-analyse behavior.
> > 
> > So I don't understand Christoph's objection yet either.
> > 
> > It might also help if we could confirm or deny Christoph's assertion
> > about the behavior of other nfs servers.  It shouldn't be hard to run
> > the original test case
> > 
> > 	http://marc.info/?l=linux-nfs&m=120730475719642&w=2
> > 
> > against another server or two.
> 
> I don't think it is that relevant for two reasons:
> 
> * Currently, the linux NFS server behaves inconsistent depending
>   on memory pressure/reboot. That on its own looks like a bug to me.
> 
> * The NFS server should never re-check access to a [directory]
>   path which has been used in the past to obtain a handle:
>   Permission checks apply only once, at open time. This is how
>   POSIX filesystems behave.
> 
> I also guess it will be hard to find another NFS server with exactly the
> same behavior. Either the x-bit of the complete path to the file-handle
> is checked upon every operation (sounds totally broken to me)

I suppose it could conceivably do it on every operation that takes a
path component as an argument (lookup and open, but not read).

> or it is never checked beyond open time.

Well, clearly we have already found one such server; I'd be interested
to know if there are others.  NFS behavior has never been completely
posix, so the next best we can do is try to behave like other NFS
implementations.

If we can get confirmation that Solaris and Netapp (e.g.) allow this,
and if you wouldn't mind updating the lookup_one_noperm() comment as
well, then I'll queue the result up for 2.6.26.

--b.

> It looks like this bug has everything
> to do with the internal housekeeping and is certainly not intentional.
> 
> -- 
> Frank

Re: reconnect_path() breaks NFS server causing occasional EACCES

[Neil Brown]

On Tuesday April 29, bfields@fieldses.org wrote:
> 
> Just to make sure I understand--you're not claiming that there's an
> actual threat of corrupting the on-disk filesystem or in-core data
> structures, right? 

Correct.  I'm not claiming that.

NeilBrown

[PATCH] exportfs: fix incorrect EACCES in reconnect_path()

[Frank van Maarseveen]

A privileged process on an NFS client which drops privileges after using
them to change the current working directory, will experience incorrect
EACCES after an NFS server reboot. This problem can also occur after
memory pressure on the server, particularly when the client side is
quiet for some time.

This patch removes the x-bit check during dentry tree reconstruction at
the server by exportfs on behalf of nfsd.

Signed-off-by: Frank van Maarseveen <frankvm@frankvm.com>
---

Here's the patch including a comment change for lookup_one_noperm().

diff -urp a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
--- a/fs/exportfs/expfs.c	2008-02-04 14:24:21.000000000 +0100
+++ b/fs/exportfs/expfs.c	2008-05-02 14:40:13.000000000 +0200
@@ -170,7 +170,7 @@ reconnect_path(struct vfsmount *mnt, str
 			}
 			dprintk("%s: found name: %s\n", __FUNCTION__, nbuf);
 			mutex_lock(&ppd->d_inode->i_mutex);
-			npd = lookup_one_len(nbuf, ppd, strlen(nbuf));
+			npd = lookup_one_noperm(nbuf, ppd);
 			mutex_unlock(&ppd->d_inode->i_mutex);
 			if (IS_ERR(npd)) {
 				err = PTR_ERR(npd);
@@ -447,8 +447,7 @@ struct dentry *exportfs_decode_fh(struct
 		err = exportfs_get_name(mnt, target_dir, nbuf, result);
 		if (!err) {
 			mutex_lock(&target_dir->d_inode->i_mutex);
-			nresult = lookup_one_len(nbuf, target_dir,
-						 strlen(nbuf));
+			nresult = lookup_one_noperm(nbuf, target_dir);
 			mutex_unlock(&target_dir->d_inode->i_mutex);
 			if (!IS_ERR(nresult)) {
 				if (nresult->d_inode) {
diff -urp a/fs/namei.c b/fs/namei.c
--- a/fs/namei.c	2008-04-29 09:30:55.000000000 +0200
+++ b/fs/namei.c	2008-05-02 17:04:32.000000000 +0200
@@ -1391,15 +1391,15 @@ struct dentry *lookup_one_len(const char
 }
 
 /**
- * lookup_one_noperm - bad hack for sysfs
  * @name:	pathname component to lookup
  * @base:	base directory to lookup from
  *
  * This is a variant of lookup_one_len that doesn't perform any permission
- * checks.   It's a horrible hack to work around the braindead sysfs
- * architecture and should not be used anywhere else.
+ * checks. nfsd needs it via exportfs to reconstruct the dentry tree for
+ * directory handles (e.g. after a server reboot).
  *
- * DON'T USE THIS FUNCTION EVER, thanks.
+ * Originally this function was a horrible hack to work around the braindead
+ * sysfs architecture and it is still being used there.
  */
 struct dentry *lookup_one_noperm(const char *name, struct dentry *base)
 {

-- 
Frank

Re: [PATCH] exportfs: fix incorrect EACCES in reconnect_path()

[Christoph Hellwig]

On Fri, May 02, 2008 at 05:16:46PM +0200, Frank van Maarseveen wrote:
> A privileged process on an NFS client which drops privileges after using
> them to change the current working directory, will experience incorrect
> EACCES after an NFS server reboot. This problem can also occur after
> memory pressure on the server, particularly when the client side is
> quiet for some time.
> 
> This patch removes the x-bit check during dentry tree reconstruction at
> the server by exportfs on behalf of nfsd.

I'm still against adding this crap, and even when I get overruled that
doesn't make the comments on lookup_one_noperm any less true, not does
it give you a permit to break the kerneldoc generation.

Re: [PATCH] exportfs: fix incorrect EACCES in reconnect_path()

[J. Bruce Fields]

On Fri, May 02, 2008 at 11:34:39AM -0400, Christoph Hellwig wrote:
> On Fri, May 02, 2008 at 05:16:46PM +0200, Frank van Maarseveen wrote:
> > A privileged process on an NFS client which drops privileges after using
> > them to change the current working directory, will experience incorrect
> > EACCES after an NFS server reboot. This problem can also occur after
> > memory pressure on the server, particularly when the client side is
> > quiet for some time.
> > 
> > This patch removes the x-bit check during dentry tree reconstruction at
> > the server by exportfs on behalf of nfsd.
> 
> I'm still against adding this crap,

The only statements I've seen against the change so far have been of the
form "you should not do that", without explaining why not.

It's entirely possible that you're right, but I need some argument.

> and even when I get overruled that
> doesn't make the comments on lookup_one_noperm any less true,

We do need to at least update it to reflect the addition of a new
caller.

> not does it give you a permit to break the kerneldoc generation.

Oops; here's a version that should make kerneldoc happy.  It also adds a
little more explanation, and leaves alone the editorializing on sysfs
(on which I have no opinion).

--b.

commit ccdfe77dc49a07c298bb9e2107290267492f16b3
Author: Frank van Maarseveen <frankvm@frankvm.com>
Date:   Fri May 2 17:16:46 2008 +0200

    exportfs: fix incorrect EACCES in reconnect_path()
    
    A privileged process on an NFS client which drops privileges after using
    them to change the current working directory, will experience incorrect
    EACCES after an NFS server reboot. This problem can also occur after
    memory pressure on the server, particularly when the client side is
    quiet for some time.
    
    This patch removes the x-bit check during dentry tree reconstruction at
    the server by exportfs on behalf of nfsd.
    
    Signed-off-by: Frank van Maarseveen <frankvm@frankvm.com>
    Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>

diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
index 109ab5e..89dc7ae 100644
--- a/fs/exportfs/expfs.c
+++ b/fs/exportfs/expfs.c
@@ -170,7 +170,7 @@ reconnect_path(struct vfsmount *mnt, struct dentry *target_dir)
 			}
 			dprintk("%s: found name: %s\n", __FUNCTION__, nbuf);
 			mutex_lock(&ppd->d_inode->i_mutex);
-			npd = lookup_one_len(nbuf, ppd, strlen(nbuf));
+			npd = lookup_one_noperm(nbuf, ppd);
 			mutex_unlock(&ppd->d_inode->i_mutex);
 			if (IS_ERR(npd)) {
 				err = PTR_ERR(npd);
@@ -447,8 +447,7 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,
 		err = exportfs_get_name(mnt, target_dir, nbuf, result);
 		if (!err) {
 			mutex_lock(&target_dir->d_inode->i_mutex);
-			nresult = lookup_one_len(nbuf, target_dir,
-						 strlen(nbuf));
+			nresult = lookup_one_noperm(nbuf, target_dir);
 			mutex_unlock(&target_dir->d_inode->i_mutex);
 			if (!IS_ERR(nresult)) {
 				if (nresult->d_inode) {
diff --git a/fs/namei.c b/fs/namei.c
index e179f71..c00150c 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1391,7 +1391,7 @@ struct dentry *lookup_one_len(const char *name, struct dentry *base, int len)
 }
 
 /**
- * lookup_one_noperm - bad hack for sysfs
+ * lookup_one_noperm - bad hack for sysfs and nfsd
  * @name:	pathname component to lookup
  * @base:	base directory to lookup from
  *
@@ -1399,7 +1399,12 @@ struct dentry *lookup_one_len(const char *name, struct dentry *base, int len)
  * checks.   It's a horrible hack to work around the braindead sysfs
  * architecture and should not be used anywhere else.
  *
- * DON'T USE THIS FUNCTION EVER, thanks.
+ * It is also used by nfsd via exports to reconstruct the dentry tree
+ * for directory handles (e.g. when a client requests a directory by
+ * filehandle after a server reboot has cleared the dentry cache of that
+ * directory's parents).
+ *
+ * DON'T USE THIS FUNCTION ANYWHERE ELSE, thanks.
  */
 struct dentry *lookup_one_noperm(const char *name, struct dentry *base)
 {

Re: [PATCH] exportfs: fix incorrect EACCES in reconnect_path()

[Trond Myklebust]

On Fri, 2008-05-02 at 11:56 -0400, J. Bruce Fields wrote:
> On Fri, May 02, 2008 at 11:34:39AM -0400, Christoph Hellwig wrote:
> > On Fri, May 02, 2008 at 05:16:46PM +0200, Frank van Maarseveen wrote:
> > > A privileged process on an NFS client which drops privileges after using
> > > them to change the current working directory, will experience incorrect
> > > EACCES after an NFS server reboot. This problem can also occur after
> > > memory pressure on the server, particularly when the client side is
> > > quiet for some time.
> > > 
> > > This patch removes the x-bit check during dentry tree reconstruction at
> > > the server by exportfs on behalf of nfsd.
> > 
> > I'm still against adding this crap,
> 
> The only statements I've seen against the change so far have been of the
> form "you should not do that", without explaining why not.
> 
> It's entirely possible that you're right, but I need some argument.


AFAICS, the real problem here is that nfsd is dropping its privileged
mode too early. Why can't you call reconnect_path() using nfsd's root
permissions instead of dropping permissions checks altogether?

> > and even when I get overruled that
> > doesn't make the comments on lookup_one_noperm any less true,
> 
> We do need to at least update it to reflect the addition of a new
> caller.
> 
> > not does it give you a permit to break the kerneldoc generation.
> 
> Oops; here's a version that should make kerneldoc happy.  It also adds a
> little more explanation, and leaves alone the editorializing on sysfs
> (on which I have no opinion).
> 
> --b.
> 
> commit ccdfe77dc49a07c298bb9e2107290267492f16b3
> Author: Frank van Maarseveen <frankvm@frankvm.com>
> Date:   Fri May 2 17:16:46 2008 +0200
> 
>     exportfs: fix incorrect EACCES in reconnect_path()
>     
>     A privileged process on an NFS client which drops privileges after using
>     them to change the current working directory, will experience incorrect
>     EACCES after an NFS server reboot. This problem can also occur after
>     memory pressure on the server, particularly when the client side is
>     quiet for some time.
>     
>     This patch removes the x-bit check during dentry tree reconstruction at
>     the server by exportfs on behalf of nfsd.
>     
>     Signed-off-by: Frank van Maarseveen <frankvm@frankvm.com>
>     Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
> 
> diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
> index 109ab5e..89dc7ae 100644
> --- a/fs/exportfs/expfs.c
> +++ b/fs/exportfs/expfs.c
> @@ -170,7 +170,7 @@ reconnect_path(struct vfsmount *mnt, struct dentry *target_dir)
>  			}
>  			dprintk("%s: found name: %s\n", __FUNCTION__, nbuf);
>  			mutex_lock(&ppd->d_inode->i_mutex);
> -			npd = lookup_one_len(nbuf, ppd, strlen(nbuf));
> +			npd = lookup_one_noperm(nbuf, ppd);
>  			mutex_unlock(&ppd->d_inode->i_mutex);
>  			if (IS_ERR(npd)) {
>  				err = PTR_ERR(npd);
> @@ -447,8 +447,7 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,
>  		err = exportfs_get_name(mnt, target_dir, nbuf, result);
>  		if (!err) {
>  			mutex_lock(&target_dir->d_inode->i_mutex);
> -			nresult = lookup_one_len(nbuf, target_dir,
> -						 strlen(nbuf));
> +			nresult = lookup_one_noperm(nbuf, target_dir);
>  			mutex_unlock(&target_dir->d_inode->i_mutex);
>  			if (!IS_ERR(nresult)) {
>  				if (nresult->d_inode) {
> diff --git a/fs/namei.c b/fs/namei.c
> index e179f71..c00150c 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -1391,7 +1391,7 @@ struct dentry *lookup_one_len(const char *name, struct dentry *base, int len)
>  }
>  
>  /**
> - * lookup_one_noperm - bad hack for sysfs
> + * lookup_one_noperm - bad hack for sysfs and nfsd
>   * @name:	pathname component to lookup
>   * @base:	base directory to lookup from
>   *
> @@ -1399,7 +1399,12 @@ struct dentry *lookup_one_len(const char *name, struct dentry *base, int len)
>   * checks.   It's a horrible hack to work around the braindead sysfs
>   * architecture and should not be used anywhere else.
>   *
> - * DON'T USE THIS FUNCTION EVER, thanks.
> + * It is also used by nfsd via exports to reconstruct the dentry tree
> + * for directory handles (e.g. when a client requests a directory by
> + * filehandle after a server reboot has cleared the dentry cache of that
> + * directory's parents).
> + *
> + * DON'T USE THIS FUNCTION ANYWHERE ELSE, thanks.
>   */
>  struct dentry *lookup_one_noperm(const char *name, struct dentry *base)
>  {

How about if exportfs is compiled as a module?

> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] exportfs: fix incorrect EACCES in reconnect_path()

[J. Bruce Fields]

On Fri, May 02, 2008 at 12:04:53PM -0400, Trond Myklebust wrote:
> 
> On Fri, 2008-05-02 at 11:56 -0400, J. Bruce Fields wrote:
> > On Fri, May 02, 2008 at 11:34:39AM -0400, Christoph Hellwig wrote:
> > > On Fri, May 02, 2008 at 05:16:46PM +0200, Frank van Maarseveen wrote:
> > > > A privileged process on an NFS client which drops privileges after using
> > > > them to change the current working directory, will experience incorrect
> > > > EACCES after an NFS server reboot. This problem can also occur after
> > > > memory pressure on the server, particularly when the client side is
> > > > quiet for some time.
> > > > 
> > > > This patch removes the x-bit check during dentry tree reconstruction at
> > > > the server by exportfs on behalf of nfsd.
> > > 
> > > I'm still against adding this crap,
> > 
> > The only statements I've seen against the change so far have been of the
> > form "you should not do that", without explaining why not.
> > 
> > It's entirely possible that you're right, but I need some argument.
> 
> 
> AFAICS, the real problem here is that nfsd is dropping its privileged
> mode too early. Why can't you call reconnect_path() using nfsd's root
> permissions instead of dropping permissions checks altogether?

That's an interesting idea.

As I understand it, nfsd sets the current task's credentials only once,
in nfsd_setuser, called from fh_verify().  The change lingers around
until next time we do fh_verify().  So in addition to moving the
nfsd_setuser() call to after the lookup of the dentry (so after
exportfs_decode_fh(), we'd also need to add an explicit acquisition of
whatever permissions we need before we do that lookup.

--b.

> 
> > > and even when I get overruled that
> > > doesn't make the comments on lookup_one_noperm any less true,
> > 
> > We do need to at least update it to reflect the addition of a new
> > caller.
> > 
> > > not does it give you a permit to break the kerneldoc generation.
> > 
> > Oops; here's a version that should make kerneldoc happy.  It also adds a
> > little more explanation, and leaves alone the editorializing on sysfs
> > (on which I have no opinion).
> > 
> > --b.
> > 
> > commit ccdfe77dc49a07c298bb9e2107290267492f16b3
> > Author: Frank van Maarseveen <frankvm@frankvm.com>
> > Date:   Fri May 2 17:16:46 2008 +0200
> > 
> >     exportfs: fix incorrect EACCES in reconnect_path()
> >     
> >     A privileged process on an NFS client which drops privileges after using
> >     them to change the current working directory, will experience incorrect
> >     EACCES after an NFS server reboot. This problem can also occur after
> >     memory pressure on the server, particularly when the client side is
> >     quiet for some time.
> >     
> >     This patch removes the x-bit check during dentry tree reconstruction at
> >     the server by exportfs on behalf of nfsd.
> >     
> >     Signed-off-by: Frank van Maarseveen <frankvm@frankvm.com>
> >     Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
> > 
> > diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
> > index 109ab5e..89dc7ae 100644
> > --- a/fs/exportfs/expfs.c
> > +++ b/fs/exportfs/expfs.c
> > @@ -170,7 +170,7 @@ reconnect_path(struct vfsmount *mnt, struct dentry *target_dir)
> >  			}
> >  			dprintk("%s: found name: %s\n", __FUNCTION__, nbuf);
> >  			mutex_lock(&ppd->d_inode->i_mutex);
> > -			npd = lookup_one_len(nbuf, ppd, strlen(nbuf));
> > +			npd = lookup_one_noperm(nbuf, ppd);
> >  			mutex_unlock(&ppd->d_inode->i_mutex);
> >  			if (IS_ERR(npd)) {
> >  				err = PTR_ERR(npd);
> > @@ -447,8 +447,7 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,
> >  		err = exportfs_get_name(mnt, target_dir, nbuf, result);
> >  		if (!err) {
> >  			mutex_lock(&target_dir->d_inode->i_mutex);
> > -			nresult = lookup_one_len(nbuf, target_dir,
> > -						 strlen(nbuf));
> > +			nresult = lookup_one_noperm(nbuf, target_dir);
> >  			mutex_unlock(&target_dir->d_inode->i_mutex);
> >  			if (!IS_ERR(nresult)) {
> >  				if (nresult->d_inode) {
> > diff --git a/fs/namei.c b/fs/namei.c
> > index e179f71..c00150c 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -1391,7 +1391,7 @@ struct dentry *lookup_one_len(const char *name, struct dentry *base, int len)
> >  }
> >  
> >  /**
> > - * lookup_one_noperm - bad hack for sysfs
> > + * lookup_one_noperm - bad hack for sysfs and nfsd
> >   * @name:	pathname component to lookup
> >   * @base:	base directory to lookup from
> >   *
> > @@ -1399,7 +1399,12 @@ struct dentry *lookup_one_len(const char *name, struct dentry *base, int len)
> >   * checks.   It's a horrible hack to work around the braindead sysfs
> >   * architecture and should not be used anywhere else.
> >   *
> > - * DON'T USE THIS FUNCTION EVER, thanks.
> > + * It is also used by nfsd via exports to reconstruct the dentry tree
> > + * for directory handles (e.g. when a client requests a directory by
> > + * filehandle after a server reboot has cleared the dentry cache of that
> > + * directory's parents).
> > + *
> > + * DON'T USE THIS FUNCTION ANYWHERE ELSE, thanks.
> >   */
> >  struct dentry *lookup_one_noperm(const char *name, struct dentry *base)
> >  {
> 
> How about if exportfs is compiled as a module?
> 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] exportfs: fix incorrect EACCES in reconnect_path()

[Frank van Maarseveen]

On Fri, May 02, 2008 at 12:04:53PM -0400, Trond Myklebust wrote:
> 
> On Fri, 2008-05-02 at 11:56 -0400, J. Bruce Fields wrote:
> > On Fri, May 02, 2008 at 11:34:39AM -0400, Christoph Hellwig wrote:
> > > On Fri, May 02, 2008 at 05:16:46PM +0200, Frank van Maarseveen wrote:
> > > > A privileged process on an NFS client which drops privileges after using
> > > > them to change the current working directory, will experience incorrect
> > > > EACCES after an NFS server reboot. This problem can also occur after
> > > > memory pressure on the server, particularly when the client side is
> > > > quiet for some time.
> > > > 
> > > > This patch removes the x-bit check during dentry tree reconstruction at
> > > > the server by exportfs on behalf of nfsd.
> > > 
> > > I'm still against adding this crap,
> > 
> > The only statements I've seen against the change so far have been of the
> > form "you should not do that", without explaining why not.
> > 
> > It's entirely possible that you're right, but I need some argument.
> 
> 
> AFAICS, the real problem here is that nfsd is dropping its privileged
> mode too early. Why can't you call reconnect_path() using nfsd's root
> permissions instead of dropping permissions checks altogether?
> 
> > > and even when I get overruled that
> > > doesn't make the comments on lookup_one_noperm any less true,
> > 
> > We do need to at least update it to reflect the addition of a new
> > caller.
> > 
> > > not does it give you a permit to break the kerneldoc generation.
> > 
> > Oops; here's a version that should make kerneldoc happy.  It also adds a
> > little more explanation, and leaves alone the editorializing on sysfs
> > (on which I have no opinion).
> > 
> > --b.
> > 
> > commit ccdfe77dc49a07c298bb9e2107290267492f16b3
> > Author: Frank van Maarseveen <frankvm@frankvm.com>
> > Date:   Fri May 2 17:16:46 2008 +0200
> > 
> >     exportfs: fix incorrect EACCES in reconnect_path()
> >     
> >     A privileged process on an NFS client which drops privileges after using
> >     them to change the current working directory, will experience incorrect
> >     EACCES after an NFS server reboot. This problem can also occur after
> >     memory pressure on the server, particularly when the client side is
> >     quiet for some time.
> >     
> >     This patch removes the x-bit check during dentry tree reconstruction at
> >     the server by exportfs on behalf of nfsd.
> >     
> >     Signed-off-by: Frank van Maarseveen <frankvm@frankvm.com>
> >     Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
> > 
> > diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
> > index 109ab5e..89dc7ae 100644
> > --- a/fs/exportfs/expfs.c
> > +++ b/fs/exportfs/expfs.c
> > @@ -170,7 +170,7 @@ reconnect_path(struct vfsmount *mnt, struct dentry *target_dir)
> >  			}
> >  			dprintk("%s: found name: %s\n", __FUNCTION__, nbuf);
> >  			mutex_lock(&ppd->d_inode->i_mutex);
> > -			npd = lookup_one_len(nbuf, ppd, strlen(nbuf));
> > +			npd = lookup_one_noperm(nbuf, ppd);
> >  			mutex_unlock(&ppd->d_inode->i_mutex);
> >  			if (IS_ERR(npd)) {
> >  				err = PTR_ERR(npd);
> > @@ -447,8 +447,7 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,
> >  		err = exportfs_get_name(mnt, target_dir, nbuf, result);
> >  		if (!err) {
> >  			mutex_lock(&target_dir->d_inode->i_mutex);
> > -			nresult = lookup_one_len(nbuf, target_dir,
> > -						 strlen(nbuf));
> > +			nresult = lookup_one_noperm(nbuf, target_dir);
> >  			mutex_unlock(&target_dir->d_inode->i_mutex);
> >  			if (!IS_ERR(nresult)) {
> >  				if (nresult->d_inode) {
> > diff --git a/fs/namei.c b/fs/namei.c
> > index e179f71..c00150c 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -1391,7 +1391,7 @@ struct dentry *lookup_one_len(const char *name, struct dentry *base, int len)
> >  }
> >  
> >  /**
> > - * lookup_one_noperm - bad hack for sysfs
> > + * lookup_one_noperm - bad hack for sysfs and nfsd
> >   * @name:	pathname component to lookup
> >   * @base:	base directory to lookup from
> >   *
> > @@ -1399,7 +1399,12 @@ struct dentry *lookup_one_len(const char *name, struct dentry *base, int len)
> >   * checks.   It's a horrible hack to work around the braindead sysfs
> >   * architecture and should not be used anywhere else.
> >   *
> > - * DON'T USE THIS FUNCTION EVER, thanks.
> > + * It is also used by nfsd via exports to reconstruct the dentry tree
> > + * for directory handles (e.g. when a client requests a directory by
> > + * filehandle after a server reboot has cleared the dentry cache of that
> > + * directory's parents).
> > + *
> > + * DON'T USE THIS FUNCTION ANYWHERE ELSE, thanks.
> >   */
> >  struct dentry *lookup_one_noperm(const char *name, struct dentry *base)
> >  {
> 
> How about if exportfs is compiled as a module?

Good point. The patch should include a EXPORT_SYMBOL(lookup_one_noperm)
in fs/namei.c if we decide to fix it this way.

-- 
Frank

Re: [PATCH] exportfs: fix incorrect EACCES in reconnect_path()

[Neil Brown]

On Friday May 2, bfields@fieldses.org wrote:
> On Fri, May 02, 2008 at 12:04:53PM -0400, Trond Myklebust wrote:
> > 
> > AFAICS, the real problem here is that nfsd is dropping its privileged
> > mode too early. Why can't you call reconnect_path() using nfsd's root
> > permissions instead of dropping permissions checks altogether?
> 
> That's an interesting idea.
> 
> As I understand it, nfsd sets the current task's credentials only once,
> in nfsd_setuser, called from fh_verify().  The change lingers around
> until next time we do fh_verify().  So in addition to moving the
> nfsd_setuser() call to after the lookup of the dentry (so after
> exportfs_decode_fh(), we'd also need to add an explicit acquisition of
> whatever permissions we need before we do that lookup.

I have substantial sympathy for this approach.
The "explicit acquisition" would probably just be
		current->cap_effective =
			cap_raise_nfsd_set(current->cap_effective,
					   current->cap_permitted);
(from nfsd_setuser).  This should reclaim the RACOVERRIDE permission
which should be enough.

The one problem that I see is that it would invalidate the
		err = permission(parent->d_inode, MAY_EXEC, NULL);
check in nfsd_acceptable.

Currently if we have "subtree_check" set, not only must the file be in
the right subtree of the filesystem, but the user accessing the file
must have 'x' permission on every parent directory.  For that test to
work we must have already set the effective uid correctly.

Now it could be argued that this permission test is really a dumb idea
that buys nothing and costs much.  And if you were to queue a patch to
get rid of it, I doubt you would get any objections .... certainly not
from me :-)

NeilBrown

Re: [PATCH] exportfs: fix incorrect EACCES in reconnect_path()

[J. Bruce Fields]

On Mon, May 05, 2008 at 09:22:49AM +1000, Neil Brown wrote:
> On Friday May 2, bfields@fieldses.org wrote:
> > On Fri, May 02, 2008 at 12:04:53PM -0400, Trond Myklebust wrote:
> > > 
> > > AFAICS, the real problem here is that nfsd is dropping its privileged
> > > mode too early. Why can't you call reconnect_path() using nfsd's root
> > > permissions instead of dropping permissions checks altogether?
> > 
> > That's an interesting idea.
> > 
> > As I understand it, nfsd sets the current task's credentials only once,
> > in nfsd_setuser, called from fh_verify().  The change lingers around
> > until next time we do fh_verify().  So in addition to moving the
> > nfsd_setuser() call to after the lookup of the dentry (so after
> > exportfs_decode_fh(), we'd also need to add an explicit acquisition of
> > whatever permissions we need before we do that lookup.
> 
> I have substantial sympathy for this approach.
> The "explicit acquisition" would probably just be
> 		current->cap_effective =
> 			cap_raise_nfsd_set(current->cap_effective,
> 					   current->cap_permitted);
> (from nfsd_setuser).  This should reclaim the RACOVERRIDE permission
> which should be enough.
> 
> The one problem that I see is that it would invalidate the
> 		err = permission(parent->d_inode, MAY_EXEC, NULL);
> check in nfsd_acceptable.
> 
> Currently if we have "subtree_check" set, not only must the file be in
> the right subtree of the filesystem, but the user accessing the file
> must have 'x' permission on every parent directory.  For that test to
> work we must have already set the effective uid correctly.
> 
> Now it could be argued that this permission test is really a dumb idea
> that buys nothing and costs much.  And if you were to queue a patch to
> get rid of it, I doubt you would get any objections .... certainly not
> from me :-)

Dumb idea or not, it looks like it's explicitly documented in
exports(5):

	" subtree  checking  is  also  used to make sure that files
	inside directories to which only root has access can only be
	accessed if  the  filesystem is exported with no_root_squash
	(see below), even if the file itself allows more general
	access."

So as much as I'd like to I'm not comfortable silently turning off that
check.

I suppose we could choose to acquire those capabilities only in the
no_subtree_check case.

--b.

Re: [PATCH] exportfs: fix incorrect EACCES in reconnect_path()

[Neil Brown]

On Monday May 5, bfields@fieldses.org wrote:
> On Mon, May 05, 2008 at 09:22:49AM +1000, Neil Brown wrote:
> > 
> > Now it could be argued that this permission test is really a dumb idea
> > that buys nothing and costs much.  And if you were to queue a patch to
> > get rid of it, I doubt you would get any objections .... certainly not
> > from me :-)
> 
> Dumb idea or not, it looks like it's explicitly documented in
> exports(5):
> 
> 	" subtree  checking  is  also  used to make sure that files
> 	inside directories to which only root has access can only be
> 	accessed if  the  filesystem is exported with no_root_squash
> 	(see below), even if the file itself allows more general
> 	access."
> 
> So as much as I'd like to I'm not comfortable silently turning off that
> check.

Ack.

> 
> I suppose we could choose to acquire those capabilities only in the
> no_subtree_check case.

If only it were that easy ;-)

reconnect_path potentially requires both 'r' and 'x' permission on
parent directories.  'r' to be able to read the directory to find the
name of the object being reconnected, and 'x' to do the lookup which
effects the reconnect.

To fix the current bug properly, reconnect_path still needs to bypass
normal permission checks even when subtree_check is in effect, so it
can be sure of getting read permission on the parent directory.

There is another way .... but it would need careful consideration.

While the dentry returned by exportfs_decode_fh (for a directory) must
be connected in the dcache tree, it does *not* need to have a correct
name.  All that is needed is that d_parent is correct  (this is used,
as mentioned before, to correctly lock directory renames).

We can leave the dentry unhashed but with a correct d_parent pointer.
If the directory is ever access by name, d_slice_alias will be called
and this will update the name in the dentry to be correct.

We could then get rid of exportfs_get_name and the call to
lookup_one_len, and add some dcache magic after the ->get_parent call
to make 'pd' an anonymous child of 'ppd'.

Some matching changes to d_splice_alias should finish the task.

Does this seem sane to anyone else?  Is it worth a try?


NeilBrown

Re: [PATCH] exportfs: fix incorrect EACCES in reconnect_path()

[J. Bruce Fields]

On Tue, May 06, 2008 at 10:35:46AM +1000, Neil Brown wrote:
> On Monday May 5, bfields@fieldses.org wrote:
> > On Mon, May 05, 2008 at 09:22:49AM +1000, Neil Brown wrote:
> > > 
> > > Now it could be argued that this permission test is really a dumb idea
> > > that buys nothing and costs much.  And if you were to queue a patch to
> > > get rid of it, I doubt you would get any objections .... certainly not
> > > from me :-)
> > 
> > Dumb idea or not, it looks like it's explicitly documented in
> > exports(5):
> > 
> > 	" subtree  checking  is  also  used to make sure that files
> > 	inside directories to which only root has access can only be
> > 	accessed if  the  filesystem is exported with no_root_squash
> > 	(see below), even if the file itself allows more general
> > 	access."
> > 
> > So as much as I'd like to I'm not comfortable silently turning off that
> > check.
> 
> Ack.
> 
> > 
> > I suppose we could choose to acquire those capabilities only in the
> > no_subtree_check case.
> 
> If only it were that easy ;-)
> 
> reconnect_path potentially requires both 'r' and 'x' permission on
> parent directories.  'r' to be able to read the directory to find the
> name of the object being reconnected, and 'x' to do the lookup which
> effects the reconnect.
> 
> To fix the current bug properly, reconnect_path still needs to bypass
> normal permission checks even when subtree_check is in effect, so it
> can be sure of getting read permission on the parent directory.

OK, but why not just forget the subtree_check case?  It would be just
another item on the "reasons not to use subtree_check" list.

If a fix for the subtree checking case were easy (or if someone else had
the time to do a very careful job of it), then fine, but maybe we should
just fix the easy case and leave the subtree checking as is for now.

--b.

> 
> There is another way .... but it would need careful consideration.
> 
> While the dentry returned by exportfs_decode_fh (for a directory) must
> be connected in the dcache tree, it does *not* need to have a correct
> name.  All that is needed is that d_parent is correct  (this is used,
> as mentioned before, to correctly lock directory renames).
> 
> We can leave the dentry unhashed but with a correct d_parent pointer.
> If the directory is ever access by name, d_slice_alias will be called
> and this will update the name in the dentry to be correct.
> 
> We could then get rid of exportfs_get_name and the call to
> lookup_one_len, and add some dcache magic after the ->get_parent call
> to make 'pd' an anonymous child of 'ppd'.
> 
> Some matching changes to d_splice_alias should finish the task.
> 
> Does this seem sane to anyone else?  Is it worth a try?
> 
> 
> NeilBrown

Re: [PATCH] exportfs: fix incorrect EACCES in reconnect_path()

[Neil Brown]

On Tuesday May 6, bfields@fieldses.org wrote:
> On Tue, May 06, 2008 at 10:35:46AM +1000, Neil Brown wrote:
> > 
> > To fix the current bug properly, reconnect_path still needs to bypass
> > normal permission checks even when subtree_check is in effect, so it
> > can be sure of getting read permission on the parent directory.
> 
> OK, but why not just forget the subtree_check case?  It would be just
> another item on the "reasons not to use subtree_check" list.

I guess so.

> 
> If a fix for the subtree checking case were easy (or if someone else had
> the time to do a very careful job of it), then fine, but maybe we should
> just fix the easy case and leave the subtree checking as is for now.

So is this the proposed fix?  A bit ugly, but I guess it's OK.

NeilBrown

Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./fs/nfsd/nfsfh.c |   29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff .prev/fs/nfsd/nfsfh.c ./fs/nfsd/nfsfh.c
--- .prev/fs/nfsd/nfsfh.c	2008-05-06 10:06:59.000000000 +1000
+++ ./fs/nfsd/nfsfh.c	2008-05-08 13:01:06.000000000 +1000
@@ -176,9 +176,24 @@ static __be32 nfsd_set_fh_dentry(struct 
 	if (IS_ERR(exp))
 		return nfserrno(PTR_ERR(exp));
 
-	error = nfsd_setuser_and_check_port(rqstp, exp);
-	if (error)
-		goto out;
+	if (exp->ex_flags & NFSEXP_NOSUBTREECHECK) {
+		/* Elevate privileges so that the lack of 'r' or 'x'
+		 * permission on some parent directory will
+		 * not stop exportfs_decode_fh from being able
+		 * to reconnect a directory into the dentry cache.
+		 * The same problem can affect "SUBTREECHECK" exports,
+		 * but as nfsd_acceptable depends on correct
+		 * access control settings being in effect, we cannot
+		 * fix that case easily - so though.
+		 */
+		current->cap_effective =
+			cap_raise_nfsd_set(current->cap_effective,
+					   current->cap_permitted);
+	} else {
+		error = nfsd_setuser_and_check_port(rqstp, exp);
+		if (error)
+			goto out;
+	}
 
 	/*
 	 * Look up the dentry using the NFS file handle.
@@ -215,6 +230,14 @@ static __be32 nfsd_set_fh_dentry(struct 
 		goto out;
 	}
 
+	if (exp->ex_flags & NFSEXP_NOSUBTREECHECK) {
+		error = nfsd_setuser_and_check_port(rqstp, exp);
+		if (error) {
+			dput(dentry);
+			goto out;
+		}
+	}
+
 	if (S_ISDIR(dentry->d_inode->i_mode) &&
 			(dentry->d_flags & DCACHE_DISCONNECTED)) {
 		printk("nfsd: find_fh_dentry returned a DISCONNECTED directory: %s/%s\n",

Re: [PATCH] exportfs: fix incorrect EACCES in reconnect_path()

[J. Bruce Fields]

On Thu, May 08, 2008 at 01:03:09PM +1000, Neil Brown wrote:
> On Tuesday May 6, bfields@fieldses.org wrote:
> > On Tue, May 06, 2008 at 10:35:46AM +1000, Neil Brown wrote:
> > > 
> > > To fix the current bug properly, reconnect_path still needs to bypass
> > > normal permission checks even when subtree_check is in effect, so it
> > > can be sure of getting read permission on the parent directory.
> > 
> > OK, but why not just forget the subtree_check case?  It would be just
> > another item on the "reasons not to use subtree_check" list.
> 
> I guess so.
> 
> > 
> > If a fix for the subtree checking case were easy (or if someone else had
> > the time to do a very careful job of it), then fine, but maybe we should
> > just fix the easy case and leave the subtree checking as is for now.
> 
> So is this the proposed fix?  A bit ugly, but I guess it's OK.

Something like that, yep.  (Frank, can you confirm that this does the
job for you?)

It'd be nice if we could find a way to incorporate a little cleanup at
the same time, but I'm not sure exactly what to suggest.

--b.

> 
> NeilBrown
> 
> Signed-off-by: Neil Brown <neilb@suse.de>
> 
> ### Diffstat output
>  ./fs/nfsd/nfsfh.c |   29 ++++++++++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
> 
> diff .prev/fs/nfsd/nfsfh.c ./fs/nfsd/nfsfh.c
> --- .prev/fs/nfsd/nfsfh.c	2008-05-06 10:06:59.000000000 +1000
> +++ ./fs/nfsd/nfsfh.c	2008-05-08 13:01:06.000000000 +1000
> @@ -176,9 +176,24 @@ static __be32 nfsd_set_fh_dentry(struct 
>  	if (IS_ERR(exp))
>  		return nfserrno(PTR_ERR(exp));
>  
> -	error = nfsd_setuser_and_check_port(rqstp, exp);
> -	if (error)
> -		goto out;
> +	if (exp->ex_flags & NFSEXP_NOSUBTREECHECK) {
> +		/* Elevate privileges so that the lack of 'r' or 'x'
> +		 * permission on some parent directory will
> +		 * not stop exportfs_decode_fh from being able
> +		 * to reconnect a directory into the dentry cache.
> +		 * The same problem can affect "SUBTREECHECK" exports,
> +		 * but as nfsd_acceptable depends on correct
> +		 * access control settings being in effect, we cannot
> +		 * fix that case easily - so though.
> +		 */
> +		current->cap_effective =
> +			cap_raise_nfsd_set(current->cap_effective,
> +					   current->cap_permitted);
> +	} else {
> +		error = nfsd_setuser_and_check_port(rqstp, exp);
> +		if (error)
> +			goto out;
> +	}
>  
>  	/*
>  	 * Look up the dentry using the NFS file handle.
> @@ -215,6 +230,14 @@ static __be32 nfsd_set_fh_dentry(struct 
>  		goto out;
>  	}
>  
> +	if (exp->ex_flags & NFSEXP_NOSUBTREECHECK) {
> +		error = nfsd_setuser_and_check_port(rqstp, exp);
> +		if (error) {
> +			dput(dentry);
> +			goto out;
> +		}
> +	}
> +
>  	if (S_ISDIR(dentry->d_inode->i_mode) &&
>  			(dentry->d_flags & DCACHE_DISCONNECTED)) {
>  		printk("nfsd: find_fh_dentry returned a DISCONNECTED directory: %s/%s\n",

Re: [PATCH] exportfs: fix incorrect EACCES in reconnect_path()

[Frank van Maarseveen]

On Fri, May 09, 2008 at 12:34:25AM -0400, J. Bruce Fields wrote:
> On Thu, May 08, 2008 at 01:03:09PM +1000, Neil Brown wrote:
> > On Tuesday May 6, bfields@fieldses.org wrote:
> > > On Tue, May 06, 2008 at 10:35:46AM +1000, Neil Brown wrote:
> > > > 
> > > > To fix the current bug properly, reconnect_path still needs to bypass
> > > > normal permission checks even when subtree_check is in effect, so it
> > > > can be sure of getting read permission on the parent directory.
> > > 
> > > OK, but why not just forget the subtree_check case?  It would be just
> > > another item on the "reasons not to use subtree_check" list.
> > 
> > I guess so.
> > 
> > > 
> > > If a fix for the subtree checking case were easy (or if someone else had
> > > the time to do a very careful job of it), then fine, but maybe we should
> > > just fix the easy case and leave the subtree checking as is for now.
> > 
> > So is this the proposed fix?  A bit ugly, but I guess it's OK.
> 
> Something like that, yep.  (Frank, can you confirm that this does the
> job for you?)

It didn't apply on 2.6.25.1 due to indentation and context but it was
trivial to do that by hand. It survived my little testprogram so yes
it works.

-- 
Frank

Re: [PATCH] exportfs: fix incorrect EACCES in reconnect_path()

[J. Bruce Fields]

On Fri, May 09, 2008 at 12:11:34PM +0200, Frank van Maarseveen wrote:
> On Fri, May 09, 2008 at 12:34:25AM -0400, J. Bruce Fields wrote:
> > On Thu, May 08, 2008 at 01:03:09PM +1000, Neil Brown wrote:
> > > On Tuesday May 6, bfields@fieldses.org wrote:
> > > > On Tue, May 06, 2008 at 10:35:46AM +1000, Neil Brown wrote:
> > > > > 
> > > > > To fix the current bug properly, reconnect_path still needs to bypass
> > > > > normal permission checks even when subtree_check is in effect, so it
> > > > > can be sure of getting read permission on the parent directory.
> > > > 
> > > > OK, but why not just forget the subtree_check case?  It would be just
> > > > another item on the "reasons not to use subtree_check" list.
> > > 
> > > I guess so.
> > > 
> > > > 
> > > > If a fix for the subtree checking case were easy (or if someone else had
> > > > the time to do a very careful job of it), then fine, but maybe we should
> > > > just fix the easy case and leave the subtree checking as is for now.
> > > 
> > > So is this the proposed fix?  A bit ugly, but I guess it's OK.
> > 
> > Something like that, yep.  (Frank, can you confirm that this does the
> > job for you?)
> 
> It didn't apply on 2.6.25.1 due to indentation and context but it was
> trivial to do that by hand. It survived my little testprogram so yes
> it works.

I've applied this for 2.6.27, by the way.

--b.

commit 7547bdf6b3e7633a9b69b7bc270a3f0eecf7a2fd
Author: Neil Brown <neilb@suse.de>
Date:   Thu May 8 13:03:09 2008 +1000

    exportfs: fix incorrect EACCES in reconnect_path()
    
    nfsd: fix spurious EACCESS in reconnect_path()
    
    Thanks to Frank Van Maarseveen for the original problem report: "A
    privileged process on an NFS client which drops privileges after using
    them to change the current working directory, will experience incorrect
    EACCES after an NFS server reboot. This problem can also occur after
    memory pressure on the server, particularly when the client side is
    quiet for some time."
    
    This occurs because the filehandle points to a directory whose parents
    are no longer in the dentry cache, and we're attempting to reconnect the
    directory to its parents without adequate permissions to perform lookups
    in the parent directories.
    
    We can therefore fix the problem by acquiring the necessary capabilities
    before attempting the reconnection.  We do this only in the
    no_subtree_check case, since the documented behavior of the
    subtree_check export option requires the server to check that the user
    has lookup permissions on all parents.
    
    The subtree_check case still has a problem, since reconnect_path()
    unnecessarily requires both read and lookup permissions on all parent
    directories.  However, a fix in that case would be more delicate, and
    use of subtree_check is already discouraged for other reasons.
    
    Signed-off-by: Neil Brown <neilb@suse.de>
    Cc: Frank van Maarseveen <frankvm@frankvm.com>
    Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>

diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index c7b0fda..f45451e 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -176,9 +176,24 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp)
 	if (IS_ERR(exp))
 		return nfserrno(PTR_ERR(exp));
 
-	error = nfsd_setuser_and_check_port(rqstp, exp);
-	if (error)
-		goto out;
+	if (exp->ex_flags & NFSEXP_NOSUBTREECHECK) {
+		/* Elevate privileges so that the lack of 'r' or 'x'
+		 * permission on some parent directory will
+		 * not stop exportfs_decode_fh from being able
+		 * to reconnect a directory into the dentry cache.
+		 * The same problem can affect "SUBTREECHECK" exports,
+		 * but as nfsd_acceptable depends on correct
+		 * access control settings being in effect, we cannot
+		 * fix that case easily.
+		 */
+		current->cap_effective =
+			cap_raise_nfsd_set(current->cap_effective,
+					   current->cap_permitted);
+	} else {
+		error = nfsd_setuser_and_check_port(rqstp, exp);
+		if (error)
+			goto out;
+	}
 
 	/*
 	 * Look up the dentry using the NFS file handle.
@@ -215,6 +230,14 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp)
 		goto out;
 	}
 
+	if (exp->ex_flags & NFSEXP_NOSUBTREECHECK) {
+		error = nfsd_setuser_and_check_port(rqstp, exp);
+		if (error) {
+			dput(dentry);
+			goto out;
+		}
+	}
+
 	if (S_ISDIR(dentry->d_inode->i_mode) &&
 			(dentry->d_flags & DCACHE_DISCONNECTED)) {
 		printk("nfsd: find_fh_dentry returned a DISCONNECTED directory: %s/%s\n",