Marking inodes as stale can be wrong

DISCUSSION Olaf Kirch 4 participants 12 messages Apr 29, 2004 – Apr 30, 2004

Subsystems (3)

NFS client NFS server Filesystems exportfs

lore

Share & tools

lore

Olaf Kirch

author

2004-04-29 14:43 UTC #0 lore raw

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

Signature · 13 lines

Olaf Kirch     |  The Hardware Gods hate me.
[email protected]   |
---------------+ 


-------------------------------------------------------
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  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Trond Myklebust

2004-04-29 16:44 UTC #1 lore raw

Quoted 4 lines · Olaf Kirch

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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Olaf Kirch

author

2004-04-29 16:58 UTC #2 lore raw

Quoted 7 lines · Trond Myklebust

Quoted 4 lines · Olaf Kirch
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

Signature · 13 lines

Olaf Kirch     |  The Hardware Gods hate me.
[email protected]   |
---------------+ 


-------------------------------------------------------
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  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Trond Myklebust

2004-04-29 17:09 UTC #3 lore raw

Quoted 11 lines · Olaf Kirch

Quoted 7 lines · Trond Myklebust
Quoted 4 lines · Olaf Kirch
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.

Quoted 2 lines

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

Olaf Kirch

author

2004-04-29 21:28 UTC #4 lore raw

Quoted 3 lines · Trond Myklebust

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

Signature · 13 lines

Olaf Kirch     |  The Hardware Gods hate me.
[email protected]   |
---------------+ 


-------------------------------------------------------
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  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Greg Banks

2004-04-29 23:51 UTC #5 lore raw

In reply to Trond Myklebust

Quoted 10 lines · Trond Myklebust

Quoted 9 lines · Olaf Kirch
Quoted 7 lines · Trond Myklebust
Quoted 4 lines · Olaf Kirch
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!

Quoted 8 lines

Quoted 2 lines
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

Quoted 4 lines

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.

Signature · 12 lines

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  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Trond Myklebust

2004-04-30 00:43 UTC #6 lore raw

Quoted 8 lines · Greg Banks

Quoted 4 lines
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.

Quoted 2 lines

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

Greg Banks

2004-04-30 01:51 UTC #7 lore raw

Quoted 5 lines · Trond Myklebust

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.

Signature · 12 lines

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  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Olaf Kirch

author

2004-04-30 08:34 UTC #8 lore raw

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

Signature · 3 lines

Olaf Kirch     |  The Hardware Gods hate me.
[email protected]   |
---------------+

fs/exportfs/expfs.c renamed from fs/exportfs/expfs.c.nostale +9 -1

@@ -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);

Greg Banks

2004-04-30 08:49 UTC #9 lore raw

Quoted 5 lines · Olaf Kirch

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.

Signature · 12 lines

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  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Olaf Kirch

author

2004-04-30 09:00 UTC #10 lore raw

Quoted 3 lines · Greg Banks

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

Signature · 13 lines

Olaf Kirch     |  The Hardware Gods hate me.
[email protected]   |
---------------+ 


-------------------------------------------------------
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  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Trond Myklebust

2004-04-29 17:21 UTC #11 lore raw

In reply to Olaf Kirch

Quoted 5 lines · Olaf Kirch

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