#255931 NFS fh_verify causes lost output

#255931 NFS fh_verify causes lost output

[Christoph Hellwig]

We have the following report from Paul Szabo in the Debian bugtracking system,
any chance someone could look into it?  The RedHat bugzilla reference is quite
interesting aswell.

-----------------------------------------------------------------------------
Package: kernel
Version: N/A; reported 2004-06-24
Severity: important
Tags: patch

With default root_squash and subtree_check options, there is a problem with
NFS exports. The bug may appear also with no_root_squash; can always be
avoided with no_subtree_check. The bug causes open filehandles to become
stale, and further read/write access to fail.

References to what I think are the same issue:
http://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=110421
http://groups.google.com.au/groups?hl=en&lr=&ie=UTF-8&threadm=fa.jnukgbv.3i82ba%40ifi.uio.no&rnum=3&prev=/groups%3Fq%3Dfh_verify%253A%2Bno%2Broot_squashed%26ie%3DISO-8859-1%26hl%3Den
showing that the problem is not specific to Debian, and is present on a
wide range of kernel versions.

Can reproduce the problem with lsof. Seems that if the "owner" runs lsof,
then root does lsof quite soon, there is no problem; root needs to wait a
while to produce the problem; there seems to be a 10-second cache period.
Demo (see output files vanish if user has NFS-mounted home):

su - psz -c 'sleep 1000 >sleep.out 2>&1 <&- &'            ## Start process
sleep 20; su - psz -c 'lsof -c sl'; sleep 1; lsof -u psz  ## OK (may repeat)
sleep 20; su - psz -c 'lsof -c sl'; sleep 20; lsof -u psz ## Fail

The server will log something like
  kernel: fh_verify: no root_squashed access at psz/sleep.out.

The problem causes loss of output, some or all may be lost e.g. for

sh -c 'echo hello; sleep 200; echo bye' >sh.out &
perl -e 'echo "hello\n"; sleep 200; echo "bye\n"' >pl.out &

The following patch (against kernel-source-2.4.22) seems to fix the
problem:

--- fs/nfsd/nfsfh.c.orig	Sat Aug 30 16:01:44 2003
+++ fs/nfsd/nfsfh.c	Wed Jun 23 12:57:52 2004
@@ -705,11 +705,23 @@
 			/* make sure parents give x permission to user */
 			error = permission(parent->d_inode, MAY_EXEC);
 			if (error)
+/*			{
+				printk("fh_verify: no UID=%d,GID=%d (userset=%d) access to %s at %s/%s\n",
+					current->fsuid, current->fsgid, rqstp->rq_userset,
+				       parent->d_name.name,
+				       dentry->d_parent->d_name.name,
+				       dentry->d_name.name); */
 				break;
+/*			} */
 			tdentry = parent;
 		}
 		if (exp->ex_dentry != tdentry) {
-			error = nfserr_stale;
+			error = nfserr_acces;
+/* PSz 23 Jun 04  Not STALE but ACCES: so NFS client code (RPC really)
+ *                net/sunrpc/clnt.c will handle and re-try as real user,
+ *                do not want fs/nfs/inode.c to remove the inode. */
+/* Should not say root_squashed without checking ROOTSQUASH or ALLSQUASH
+ * and UID/GID. (Probably should be dprintk: lucky it was not.) */
 			printk("fh_verify: no root_squashed access at %s/%s.\n",
 			       dentry->d_parent->d_name.name,
 			       dentry->d_name.name);

Cheers,

Paul Szabo - psz@maths.usyd.edu.au  http://www.maths.usyd.edu.au:8000/u/psz/
School of Mathematics and Statistics  University of Sydney   2006  Australia


-- System Information
Debian Release: 3.0
Architecture: i386
Kernel: Linux pisa.maths.usyd.edu.au 2.4.22-smssvr1.5 #1 SMP Fri Apr 16 13:55:30 EST 2004 i686
Locale: LANG=C, LC_CTYPE=C





-------------------------------------------------------
SF.Net email is sponsored by Shop4tech.com-Lowest price on Blank Media
100pk Sonic DVD-R 4x for only $29 -100pk Sonic DVD+R for only $33
Save 50% off Retail on Ink & Toner - Free Shipping and Free Gift.
http://www.shop4tech.com/z/Inkjet_Cartridges/9_108_r285
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: #255931 NFS fh_verify causes lost output

[Olaf Kirch]

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

On Wed, Aug 18, 2004 at 09:00:41PM +0100, Christoph Hellwig wrote:
> We have the following report from Paul Szabo in the Debian bugtracking system,
> any chance someone could look into it?  The RedHat bugzilla reference is quite
> interesting aswell.

I reported exactly this problem here a few months ago; the fix I proposed
was to return EACCES rather than ESTALE in find_exported_dentry()
(fs/exportfs/expfs.c). Not sure what happened to this patch; seems it
fell through the cracks.

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: #255931 NFS fh_verify causes lost output

[Neil Brown]

On Wednesday August 18, hch@infradead.org wrote:
> We have the following report from Paul Szabo in the Debian bugtracking system,
> any chance someone could look into it?  The RedHat bugzilla reference is quite
> interesting aswell.
> 
> -----------------------------------------------------------------------------
> Package: kernel
> Version: N/A; reported 2004-06-24
> Severity: important
> Tags: patch
> 
> With default root_squash and subtree_check options, there is a problem with
> NFS exports. The bug may appear also with no_root_squash; can always be
> avoided with no_subtree_check. The bug causes open filehandles to become
> stale, and further read/write access to fail.


Yes... I've seen this reported and seen the suggested patches, but I
haven't been comfortable with the suggestions and have been guilty of
leaving it in the too-hard basket.

I'm tempted to just remove the permission check altogether.

Originally, the check here was "does anyone other that root have x
permission" and "it is exported root_squash".
If the first was "no" and the second was "yes", then access was
denied.
Note that this test does not involve who the current user is, and so
can safely return STALE.

However the first check is not easy to perform when ACLs exist (have I
mentioned that I hate acls?).  So it was changed to the "permission"
check, which seemed close-enough.


Returning EPERM instead of ESTALE seems to fix the symptom, but I
really don't think it is the right thing to do.

So here are my options:

 1/ remove the permission check, and remove a tiny bit of security
    that it is vaguely possible that someone is relying on (if a
    directory is root owned and perm 700, and the fs is exported
    root_squash,subtree_check, then everything below is not
    accessible, even if the right filehandle can be guessed)
 2/ change the error to EPERM, which is wrong because filehandle
    lookup itself shouldn't return EPERM, and if the failure is
    because the file is outside the exported tree rather than inside
    but with wrong permission, then it really should be ESTALE ... but
    who cares what error is returned if someone is guessing
    filehandles, and if they aren't, then they can only get the
    filehandle if they really have access anyway.
 3/ leave it as it is, broken.
 4/ invent some way to test 'does anyone other than root have x
    access'?

As you can see, so far I have leant towards 3.

I suspect 2 is probably best, but I think I'll leave it for now, and
think through it again on Monday and see if I can convince my self of
anything else before I actually submit a patch.

NeilBrown


-------------------------------------------------------
SF.Net email is sponsored by Shop4tech.com-Lowest price on Blank Media
100pk Sonic DVD-R 4x for only $29 -100pk Sonic DVD+R for only $33
Save 50% off Retail on Ink & Toner - Free Shipping and Free Gift.
http://www.shop4tech.com/z/Inkjet_Cartridges/9_108_r285
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: #255931 NFS fh_verify causes lost output

[Olaf Kirch]

Hi Neil,

> Originally, the check here was "does anyone other that root have x
> permission" and "it is exported root_squash".
> If the first was "no" and the second was "yes", then access was
> denied.

But that is an access control check; and a failed access control
check should return EACCES.

Probably the worst aspect of returning ESTALE is that this information
is cached persistently by the client; it doesn't time out. So it will
never clear up, even if the directory is changed to mode 777 in the
meanwhile.

>  2/ change the error to EPERM, which is wrong because filehandle
>     lookup itself shouldn't return EPERM, and if the failure is

Then we shouldn't mix permission checks with file handle lookup.
As far as I understand, the only reason this permission check is
done inside the file handle lookup is because a file handle may
potentially map to more than on dentry chain?

For what it's worth, I can live with both solution 1 and solution 2.
I don't believe in 4.

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


-------------------------------------------------------
SF.Net email is sponsored by Shop4tech.com-Lowest price on Blank Media
100pk Sonic DVD-R 4x for only $29 -100pk Sonic DVD+R for only $33
Save 50% off Retail on Ink & Toner - Free Shipping and Free Gift.
http://www.shop4tech.com/z/Inkjet_Cartridges/9_108_r285
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs