reconnect_path() breaks NFS server causing occasional EACCES

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 renamed from fs/exportfs/expfs.c.orig +1 -2

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

Signature · 6 lines

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

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

Quoted 3 lines · 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.

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?

Quoted 57 lines

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 reconnect_path(struct vfsmount *mnt, str · line 173

Quoted 8 lines

fs/exportfs/expfs.c renamed from fs/exportfs/expfs.c.orig +1 -1

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

Quoted 20 lines

fs/exportfs/expfs.c renamed from fs/exportfs/expfs.c.orig +1 -2

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

Signature · 6 lines

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

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

Quoted 10 lines · J. Bruce Fields

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.

Quoted 10 lines · J. Bruce Fields

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.

Quoted 10 lines · J. Bruce Fields

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.

Quoted 12 lines · On Wednesday April 9, [email protected] wrote:

Quoted 10 lines · J. Bruce Fields

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

Quoted 4 lines

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.

Quoted 30 lines · Neil Brown

Quoted 12 lines · On Wednesday April 9, [email protected] wrote:

Quoted 10 lines · J. Bruce Fields

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

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 31 lines · J. Bruce Fields

Quoted 30 lines · Neil Brown

Quoted 12 lines · On Wednesday April 9, [email protected] wrote:

Quoted 10 lines · J. Bruce Fields

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

Quoted 4 lines

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.

Quoted 6 lines

Quoted 6 lines

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.

Quoted 17 lines

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.

Quoted 7 lines

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.

Signature · 1 line

Frank

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?

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.

Quoted 8 lines · 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.

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.

Quoted 10 lines · Christoph Hellwig

Quoted 8 lines · 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.

Quoted 1 line

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.

Quoted 2 lines

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.

Quoted 1 line

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 <[email protected]>
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 <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

fs/exportfs/expfs.c +2 -3

index 109ab5e..89dc7ae 100644
@@ -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) {

fs/namei.c +1 -1

index e179f71..c00150c 100644
@@ -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.
  *

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

Quoted 16 lines · J. Bruce Fields

Quoted 11 lines · Christoph Hellwig

Quoted 8 lines · 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.

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.

Quoted 1 line

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?

Quoted 73 lines

Quoted 3 lines

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.

Quoted 1 line

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 <[email protected]>
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 <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

fs/exportfs/expfs.c +2 -3

index 109ab5e..89dc7ae 100644
@@ -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) {

fs/namei.c +1 -1

index e179f71..c00150c 100644
@@ -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.
  *

fs/namei.c struct dentry *lookup_one_len(const char *name, struct dentry *base, int len) · line 1410

Quoted 10 lines

fs/namei.c +6 -1

- * 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?

Quoted 4 lines

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

Quoted 11 lines · J. Bruce Fields

Quoted 10 lines · Christoph Hellwig

Quoted 8 lines · 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.

Quoted 6 lines

Quoted 1 line

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?

Quoted 76 lines

Quoted 2 lines

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.

Quoted 1 line

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 <[email protected]>
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 <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

fs/exportfs/expfs.c +2 -3

index 109ab5e..89dc7ae 100644
@@ -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) {

fs/namei.c +4 -2

index e179f71..c00150c 100644
@@ -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

fs/namei.c struct dentry *lookup_one_len(const char *name, struct dentry *base, int len) · line 1410

Quoted 6 lines

fs/namei.c +3 -0

+ * 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?

Quoted 4 lines · Trond Myklebust

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.

Quoted 6 lines · On Friday May 2, [email protected] wrote:

Quoted 4 lines · Trond Myklebust

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?

Quoted 8 lines

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

Quoted 5 lines · Neil Brown

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.

Quoted 10 lines · On Monday May 5, [email protected] wrote:

Quoted 5 lines · Neil Brown

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

Quoted 8 lines

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

Quoted 3 lines

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

Quoted 4 lines · Neil Brown

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.

Quoted 8 lines · On Tuesday May 6, [email protected] wrote:

Quoted 4 lines · Neil Brown

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.

Quoted 4 lines

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 <[email protected]>

### Diffstat output

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

diff .prev/fs/nfsd/nfsfh.c ./fs/nfsd/nfsfh.c

fs/nfsd/nfsfh.c +26 -3

@@ -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",

Quoted 13 lines · Neil Brown

Quoted 8 lines · On Tuesday May 6, [email protected] wrote:

Quoted 4 lines · Neil Brown

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.

Quoted 1 line

Quoted 5 lines

Quoted 3 lines

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

Quoted 18 lines · J. Bruce Fields

Quoted 17 lines · Neil Brown

Quoted 8 lines · On Tuesday May 6, [email protected] wrote:

Quoted 4 lines · Neil Brown

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.

Quoted 4 lines

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.

Quoted 4 lines

Quoted 1 line

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.