2008-04-29 16:35:59

by [email protected]

[permalink] [raw]
Subject: Re: reconnect_path() breaks NFS server causing occasional EACCES

On Tue, Apr 29, 2008 at 03:20:30PM +1000, Neil Brown wrote:
> On Wednesday April 9, [email protected] 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.


2008-04-29 17:40:06

by Frank van Maarseveen

[permalink] [raw]
Subject: Re: reconnect_path() breaks NFS server causing occasional EACCES

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

2008-04-30 17:47:42

by [email protected]

[permalink] [raw]
Subject: Re: reconnect_path() breaks NFS server causing occasional EACCES

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

2008-05-02 15:16:49

by Frank van Maarseveen

[permalink] [raw]
Subject: [PATCH] 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]>
---

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

2008-05-02 15:34:44

by Christoph Hellwig

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

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.


2008-05-02 15:56:24

by [email protected]

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

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

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

2008-05-02 16:05:04

by Trond Myklebust

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


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 <[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]>
>
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html


2008-05-02 22:12:28

by [email protected]

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

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 <[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]>
> >
> > 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 [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>