Marking inodes as stale can be wrong

Marking inodes as stale can be wrong

[Olaf Kirch]

Hi,

I've been debugging a strange problem with kmail on an NFS mounted
home directory. Attaching files to an outgoing message would cause
the mail to fail. For instance, after opening the file selection
dialog, kmail was no longer able to read the outbox.
This was 100 % reproduceable

The problem was caused by the following:

 -	a KDE component would invoke some silly setuid
 	helper program prior to displaying the file
	selection dialog. This helper would receive all
	open file descriptors, including those of the
	open mail boxes.

 -	the helper program itself did not access any of
 	these files, but for some reason, we still ended
	up calling revalidate_inode on the open files.
	I haven't quite found out why.

 -	nfs_revalidate_inode would do a GETATTR call to
 	the server (running the 2.4.22 kernel), using
	uid 0.

	The file system was exported with root_squash
	and subtree_check.

	fh_verify did the subtree check, and found that
	~/Mail was mode 700, and hence user nobody didn't
	have x permissions on the directory. So it would
	return ESTALE to the client.

 -	The client marks the file handle as STALE

 -	The next attempt to read from the file fails because
 	of this.

How should we fix this? I asked the KDE folks to mark their mail box
file descriptor FD_CLOEXEC, but that is just a workaround, as other
application may exhibit similar problems.

I think it's wrong to mark the file handle STALE for everyone; whether
GETATTR reports NFSERR_STALE can depend on the process doing the call.

What is the rationale behind making the stale-ness information sticky?
Can we safely get rid of the NFS_INO_STALE flag?

Olaf
-- 
Olaf Kirch     |  The Hardware Gods hate me.
okir@suse.de   |
---------------+ 


-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g. 
Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id=3149&alloc_id=8166&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Marking inodes as stale can be wrong

[Trond Myklebust]

On Thu, 2004-04-29 at 10:40, Olaf Kirch wrote:
> 	fh_verify did the subtree check, and found that
> 	~/Mail was mode 700, and hence user nobody didn't
> 	have x permissions on the directory. So it would
> 	return ESTALE to the client.

This is a bug!

Cheers,
  Trond


-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g. 
Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id=3149&alloc_id=8166&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Marking inodes as stale can be wrong

[Olaf Kirch]

On Thu, Apr 29, 2004 at 12:44:56PM -0400, Trond Myklebust wrote:
> On Thu, 2004-04-29 at 10:40, Olaf Kirch wrote:
> > 	fh_verify did the subtree check, and found that
> > 	~/Mail was mode 700, and hence user nobody didn't
> > 	have x permissions on the directory. So it would
> > 	return ESTALE to the client.
> 
> This is a bug!

Yes, probably. Even though I'm not entirely sure NFSERR_ACCES is a valid
return for all NFS operations.

However, machines with 2.4 will be out there for a while, so the question
I'm asking myself is how to cope with those in the client.

Olaf
-- 
Olaf Kirch     |  The Hardware Gods hate me.
okir@suse.de   |
---------------+ 


-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g. 
Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id=3149&alloc_id=8166&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Marking inodes as stale can be wrong

[Trond Myklebust]

On Thu, 2004-04-29 at 12:58, Olaf Kirch wrote:
> On Thu, Apr 29, 2004 at 12:44:56PM -0400, Trond Myklebust wrote:
> > On Thu, 2004-04-29 at 10:40, Olaf Kirch wrote:
> > > 	fh_verify did the subtree check, and found that
> > > 	~/Mail was mode 700, and hence user nobody didn't
> > > 	have x permissions on the directory. So it would
> > > 	return ESTALE to the client.
> > 
> > This is a bug!
> 
> Yes, probably. Even though I'm not entirely sure NFSERR_ACCES is a valid
> return for all NFS operations.

The problem is that Neil is using NFSERR_STALE as a standin for
NFSERR_ACCES precisely because the latter is not on the list of accepted
return value for GETATTR in RFC1813.
IMO this is *worse* behaviour than just returning the non-rfc compatible
error.

> However, machines with 2.4 will be out there for a while, so the question
> I'm asking myself is how to cope with those in the client.

I am totally uninterested in fixing up a broken server option (even if
it is a default option) inside the client. These "fixes" have a tendency
to persist for way too long in the kernel code, and end up causing way
more problems in the long run.

"subtree_check" has a number of other RFC-violating bugs (most notably
with cross-directory renames) that mean we should *never* have made it a
default option in the first place.
The correct workaround here is to advise people to *always* use
"no_subtree_check" and then to look into fixing these problems in future
revisions of the server, not the client.

Cheers,
  Trond


-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g. 
Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id=3149&alloc_id=8166&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Marking inodes as stale can be wrong

[Olaf Kirch]

On Thu, Apr 29, 2004 at 01:09:30PM -0400, Trond Myklebust wrote:
> The correct workaround here is to advise people to *always* use
> "no_subtree_check" and then to look into fixing these problems in future
> revisions of the server, not the client.

I agree with that.

Olaf
-- 
Olaf Kirch     |  The Hardware Gods hate me.
okir@suse.de   |
---------------+ 


-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g. 
Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id=3149&alloc_id=8166&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Marking inodes as stale can be wrong

[Greg Banks]

On Thu, Apr 29, 2004 at 01:09:30PM -0400, Trond Myklebust wrote:
> On Thu, 2004-04-29 at 12:58, Olaf Kirch wrote:
> > On Thu, Apr 29, 2004 at 12:44:56PM -0400, Trond Myklebust wrote:
> > > On Thu, 2004-04-29 at 10:40, Olaf Kirch wrote:
> > > > 	fh_verify did the subtree check, and found that
> > > > 	~/Mail was mode 700, and hence user nobody didn't
> > > > 	have x permissions on the directory. So it would
> > > > 	return ESTALE to the client.
> > > 
> > > This is a bug!
> > 
> > Yes, probably. Even though I'm not entirely sure NFSERR_ACCES is a valid
> > return for all NFS operations.
> 
> The problem is that Neil is using NFSERR_STALE as a standin for
> NFSERR_ACCES precisely because the latter is not on the list of accepted
> return value for GETATTR in RFC1813.
> IMO this is *worse* behaviour than just returning the non-rfc compatible
> error.

FWIW there's a little known sentence in RFC1813 Section 3 which says

> The ERRORS section lists the errors returned for specific types of
> failures. These lists are not intended to be the definitive statement
> of all of the errors which can be returned by any specific procedure,
> but as a guide for the more common errors which may be returned.

This has been interpreted in discussions here as a loophole that allows
the server to return any of the defined NFS errors for any of the calls
at its discretion.

I don't know why the server returns NFSERR_STALE instead of NFSERR_ACCES,
but RFC compliance is not a good reason.

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
I don't speak for SGI.


-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g. 
Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id=3149&alloc_id=8166&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Marking inodes as stale can be wrong

[Trond Myklebust]

On Thu, 2004-04-29 at 19:50, Greg Banks wrote:
> > The ERRORS section lists the errors returned for specific types of
> > failures. These lists are not intended to be the definitive statement
> > of all of the errors which can be returned by any specific procedure,
> > but as a guide for the more common errors which may be returned.
> 
> This has been interpreted in discussions here as a loophole that allows
> the server to return any of the defined NFS errors for any of the calls
> at its discretion.

True, and indeed I should have known that.

Note for instance that the error NFS3ERR_JUKEBOX is not listed as a
return value for any of the procedures: I'm sure that it would come as a
surprise to most server implementors if you were to disallow its use
solely on those grounds.

> I don't know why the server returns NFSERR_STALE instead of NFSERR_ACCES,
> but RFC compliance is not a good reason.

Right. It is quite simply a bug...

NFS3ERR_STALE can be returned if the server unexports the filesystem
("revokes access") but here we are talking about a pure credential-based
access check, and so NFS3ERR_ACCES is the correct return value.

Cheers,
  Trond


-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g. 
Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id=3149&alloc_id=8166&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Marking inodes as stale can be wrong

[Greg Banks]

Trond Myklebust wrote:
> 
> Note for instance that the error NFS3ERR_JUKEBOX is not listed as a
> return value for any of the procedures: I'm sure that it would come as a
> surprise to most server implementors if you were to disallow its use
> solely on those grounds.

Oh indeed ;-)  My next server patch does precisely that.

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
I don't speak for SGI.


-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g. 
Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id=3149&alloc_id=8166&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Marking inodes as stale can be wrong

[Olaf Kirch]

[-- Attachment #1: Type: text/plain, Size: 333 bytes --]

Here's a minimalistic fix. It may be better to change nfsd_acceptable
to return 0 on success and negative error code on error, but I'm not
sure this is really needed as long as the only acceptance test
in nfsd_acceptable is the subtree check.

Olaf
-- 
Olaf Kirch     |  The Hardware Gods hate me.
okir@suse.de   |
---------------+ 

[-- Attachment #2: subtreecheck-nostale.patch --]
[-- Type: text/plain, Size: 657 bytes --]

--- linux-2.6.5/fs/exportfs/expfs.c.nostale	2004-04-04 05:37:44.000000000 +0200
+++ linux-2.6.5/fs/exportfs/expfs.c	2004-04-30 10:32:19.000000000 +0200
@@ -278,7 +278,15 @@
 
 	/* drat - I just cannot find anything acceptable */
 	dput(result);
-	return ERR_PTR(-ESTALE);
+
+	/* Originally, we would return ESTALE here. This is not
+	 * correct, however, as the file handle is valid; it just
+	 * failed our acceptance test.
+	 * This will lead to unexpected results in the client if
+	 * there are two processes accessing the same file; one
+	 * with proper permissions and one without.
+	 */
+	return ERR_PTR(-EACCES);
 
  err_target:
 	dput(target_dir);

Re: Marking inodes as stale can be wrong

[Greg Banks]

Olaf Kirch wrote:
> 
> Here's a minimalistic fix. It may be better to change nfsd_acceptable
> to return 0 on success and negative error code on error, but I'm not
> sure this is really needed as long as the only acceptance test
> in nfsd_acceptable is the subtree check.

I think the patch makes the opposite mistake to the current code.
RFC1813 says:

NFS3ERR_ACCES
       Permission denied. The caller does not have the correct
       permission to perform the requested operation.[...]

NFS3ERR_STALE
       Invalid file handle. The file handle given in the
       arguments was invalid. The file referred to by that file
       handle no longer exists or access to it has been
       revoked.

So in the case where "subtree_check" is on and the client sends a
file handle to a file in a tree which has been unexported, a server
with the patch will incorrectly send ACCES instead of STALE.

I think the real fix is your other suggestion: changing the calling
convention of nfsd_acceptable to allow it to choose the error.

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
I don't speak for SGI.


-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g. 
Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id=3149&alloc_id=8166&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Marking inodes as stale can be wrong

[Olaf Kirch]

On Fri, Apr 30, 2004 at 06:47:24PM +1000, Greg Banks wrote:
> So in the case where "subtree_check" is on and the client sends a
> file handle to a file in a tree which has been unexported, a server
> with the patch will incorrectly send ACCES instead of STALE.

When we get there, we have already found a connected, valid dentry. We
also have an export entry, which was specified by the client. If we don't
have either, we'll error mout with ESTALE long before.

All that's left to do at this point is make sure the dentry

 a)	actually resides below the exported inode
 b)	all intermediate directories have +x for the caller

So the only case where this patch incorrectly(?) returns EACCESS is if
the specified dentry does not reside below the exported dentry.

So yes, the cleaner approach is probably to change nfsd_acceptable.

Olaf
-- 
Olaf Kirch     |  The Hardware Gods hate me.
okir@suse.de   |
---------------+ 


-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g. 
Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id=3149&alloc_id=8166&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Marking inodes as stale can be wrong

[Trond Myklebust]

On Thu, 2004-04-29 at 10:40, Olaf Kirch wrote:
> Hi,

>  -	nfs_revalidate_inode would do a GETATTR call to
>  	the server (running the 2.4.22 kernel), using
> 	uid 0.

Note: several of the nfs_file_*() functions call nfs_revalidate_inode(),
and they do so with task creds instead of using the actual credentials
in the struct file.

While I disagree that the client's interpretation of NFSERR_STALE is
incorrect, I do agree that using the wrong creds when revalidating an
open file is indeed a client bug, and should be fixed.

Cheers,
  Trond


-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g. 
Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id=3149&alloc_id=8166&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs