[PATCH] DefineSimpleCache macro problem

[PATCH] DefineSimpleCache macro problem

[Olaf Kirch]

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

Hi,

the 2.6 code has this new cache stuff to create caches for various
things, which is used for the export table and ip client list.

I came across a strange problem where a host exported several directories
to a group of clients, but clients were unable to mount more than one.

It turned out that the problem was caused by svc_export_lookup, which
will create bogus entries even if called in lookup-only mode.  This caused
bogus export entries to be created, which didn't have the VALID flag set.

Essentially, the macro-defined function in sunrpc/cache.h will always
create entries in the cache, even if "set == 0". The attached patch
fixes this, and changes the lookup functions to return NULL in this
case, rather than a bogus entry.

However, this change uncovered another problem. AIX clients will
do a NULL procedure call before attempting the mount (I think this
is a known issue). If the file system is exported to a wildcard
group, the kernel will not know about the client when this call
arrives.

Previously, this would always succeed, because the ip_map_lookup() call
from svcauth_unix_accept() would always return an entry, even if it was
a bogus one. With the change to cache.h, ip_map_lookup() will now return
NULL for unknown clients, causing the NULL call from the AIX client
to be dropped. The second patch attached below will fix this issue
as well.

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

[-- Attachment #2: sunrpc-bogus-export-entry --]
[-- Type: text/plain, Size: 489 bytes --]

--- linux-2.6.5/include/linux/sunrpc/cache.h.bug	2004-03-12 12:17:26.000000000 +0100
+++ linux-2.6.5/include/linux/sunrpc/cache.h	2004-05-10 13:19:58.000000000 +0200
@@ -223,7 +223,7 @@
 	else read_unlock(&(DETAIL)->hash_lock);						\
 	if (new && set)									\
 		cache_fresh(DETAIL, &new->MEMBER, item->MEMBER.expiry_time);		\
-	if (new)				       					\
+	if (new || !set)			       					\
 		return new;								\
 	new = kmalloc(sizeof(*new), GFP_KERNEL);					\
 	if (new) {									\

[-- Attachment #3: sunrpc-null-noauth --]
[-- Type: text/plain, Size: 567 bytes --]

--- linux-2.6.5/net/sunrpc/svcauth_unix.c.aix	2004-05-12 21:40:41.000000000 +0200
+++ linux-2.6.5/net/sunrpc/svcauth_unix.c	2004-05-13 10:05:38.000000000 +0200
@@ -390,7 +390,13 @@
 			break;
 		default: BUG();
 		}
-	else rv = SVC_DROP;
+	else if (rqstp->rq_proc == 0) {
+		/* We always accept calls to NULL. AIX clients try
+		   a NULL call before mounting, so when we get here
+		   we may not know about the export entry yet. */
+		rv = SVC_OK;
+	} else
+		rv = SVC_DROP;
 
 	if (rqstp->rq_client == NULL && rqstp->rq_proc != 0)
 		*authp = rpc_autherr_badcred;

Re: [PATCH] DefineSimpleCache macro problem

[Neil Brown]

On Thursday May 13, okir@suse.de wrote:
> Hi,
> 
> the 2.6 code has this new cache stuff to create caches for various
> things, which is used for the export table and ip client list.
> 
> I came across a strange problem where a host exported several directories
> to a group of clients, but clients were unable to mount more than one.
> 
> It turned out that the problem was caused by svc_export_lookup, which
> will create bogus entries even if called in lookup-only mode.  This caused
> bogus export entries to be created, which didn't have the VALID flag set.
> 
> Essentially, the macro-defined function in sunrpc/cache.h will always
> create entries in the cache, even if "set == 0". The attached patch
> fixes this, and changes the lookup functions to return NULL in this
> case, rather than a bogus entry.

That would be wrong.  Always returning an entry (possibly non-VALID)
- except on kmalloc failure - is a design feature.
If a non-VALID entry is returned the caller typically initiates an
upcall for the information in that entry (unless one is already
pending), and then the request is deferred until the entry is filled
in. When the cache item is marked VALID, all the deferred requests
attached to it are queued for re-processing.
So we definitely want to return the non-VALID entry.

Can you help understand more about your particular problem?
Why do you think that svc_export_lookup returning a non-VALID entry
causes a problem.
It is only called with "set" == 0 in exp_get_by_name, and
exp_get_by_name will only return it to the caller it is VALID.
So I cannot figure out what the real problem is yet.

Also, are you running with the nfsd filesystem mounted on
/proc/fs/nfsd (or /proc/fs/nfs) or with it unmounted?


NeilBrown


-------------------------------------------------------
This SF.Net email is sponsored by: SourceForge.net Broadband
Sign-up now for SourceForge Broadband and get the fastest
6.0/768 connection for only $19.95/mo for the first 3 months!
http://ads.osdn.com/?ad_id=2562&alloc_id=6184&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] DefineSimpleCache macro problem

[Olaf Kirch]

On Thu, May 13, 2004 at 07:36:27PM +1000, Neil Brown wrote:
> That would be wrong.  Always returning an entry (possibly non-VALID)
> - except on kmalloc failure - is a design feature.

Does this mean I can eat all your memory by flooding your NFS server
with NULL calls from bogus addresses? That would be bad.

> Can you help understand more about your particular problem?
> Why do you think that svc_export_lookup returning a non-VALID entry
> causes a problem.
> It is only called with "set" == 0 in exp_get_by_name, and
> exp_get_by_name will only return it to the caller it is VALID.
> So I cannot figure out what the real problem is yet.

The problem is that somewhere somehow an invalid entry is created,
which remains in the cache and the flags don't get updated.

Here's a trace of mountd, after reboot with a fresh slate (i.e. 
/proc/net/rpc/nfsd.export/contents is empty). The mount attempts
are pretty close together; this happens reproduceably if the client
is the automounter using /net/<hostname>

open("/proc/net/rpc/nfsd.export/channel", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 6
write(6, "someclient.suse.de / 2147483647 32 65534 65534 2053 \n", 50) = 50
open("/proc/net/rpc/nfsd.fh/channel", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 6
write(6, "someclient.suse.de 0 \\x0008000502000000 2147483647 / \n", 51) = 51
open("/proc/net/rpc/nfsd.export/channel", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 6
write(6, "someclient.suse.de /boot 2147483647 32 65534 65534 2049 \n", 54) = 54
open("/proc/net/rpc/nfsd.fh/channel", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 6
write(6, "someclient.suse.de 0 \\x0008000102000000 2147483647 /boot \n", 55) = -1 ENOENT (No such file or directory)
open("/proc/net/rpc/nfsd.fh/channel", O_WRONLY) = 6
open("/proc/net/rpc/nfsd.export/channel", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 6
write(6, "someclient.suse.de /home 2147483647 32 65534 65534 2054 \n", 54) = 54
open("/proc/net/rpc/nfsd.fh/channel", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 6
write(6, "someclient.suse.de 0 \\x0008000602000000 2147483647 /home \n", 55) = -1 ENOENT (No such file or directory)

As you can see, the attempt to write to nfsd.fh fails with ENOENT.

Here's a snippet from syslog:

May 10 12:54:52 testnfs kernel: nfsd: exp_rootfh(/ [c7dd2280] appserv.suse.de:sda5/2)
...
May 10 12:54:52 testnfs kernel: svc_expkey_lookup called
May 10 12:54:52 testnfs kernel: cache_check:67: item=c54c09c0
May 10 12:54:52 testnfs kernel: cache_check:80: rv=0
				^^^ we repeatedly call svc_expkey_lookup,
				^^^ I see 2 or 3 different item pointers
...
May 10 12:54:52 testnfs kernel: svc_export_lookup called
May 10 12:54:52 testnfs kernel: cache_init:40: item=c54c0900
May 10 12:54:52 testnfs kernel: exp_get_by_name:589: exp=c54c0900 flags=8
				^^^ here we get a fresh entry but don't init it
				^^^ not sure who called exp_get_by_name here
May 10 12:54:52 testnfs kernel: cache_check:67: item=c54c0900
May 10 12:54:52 testnfs kernel: cache_check:71: flags=0x8 expiry=1084186612 now=1084186492
May 10 12:54:52 testnfs kernel: cache_check:86: rv=-11
May 10 12:54:52 testnfs kernel: Want update, refage=120, age=0
May 10 12:54:52 testnfs kernel: cache_fresh:130: item=c54c0900, expiry=1084186612
...
May 10 12:54:52 testnfs kernel: svc_export_parse:402: an_int=32
May 10 12:54:52 testnfs kernel: svc_export_parse:425: err=0
May 10 12:54:52 testnfs kernel: svc_export_lookup called
May 10 12:54:52 testnfs kernel: svc_export_update:494: new=c54c0900 flags=b
				^^^ and by now the item has turned bad
				^^^ (HASHED,VALID,NEGATIVE)
May 10 12:54:52 testnfs kernel: cache_fresh:130: item=c54c0900, expiry=2147483647
May 10 12:54:52 testnfs kernel: svc_export_parse:428: expp=c54c0900 flags=b
May 10 12:54:52 testnfs kernel: svc_export_parse:440: err=0
May 10 12:54:52 testnfs kernel: found domain someclient.suse.de
May 10 12:54:52 testnfs kernel: found fsidtype 0
May 10 12:54:52 testnfs kernel: found fsid length 8
May 10 12:54:52 testnfs kernel: Path seems to be </boot>
May 10 12:54:52 testnfs kernel: Found the path /boot
...

I haven't yet figured out exactly what happens, but _something_ calls
exp_get_by_name for /boot prior to mount's call. This leaves an invalid
item in the cache, and svc_export_update doesn't fix the flags.

These bad export entries are visible in
/proc/net/rpc/nfsd.export/contents; they're shown as

# someclient.suse.de ....

> Also, are you running with the nfsd filesystem mounted on
> /proc/fs/nfsd (or /proc/fs/nfs) or with it unmounted?

The nfsd file system is not mounted, but I think this shouldn't 
make a difference.

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


-------------------------------------------------------
This SF.Net email is sponsored by: SourceForge.net Broadband
Sign-up now for SourceForge Broadband and get the fastest
6.0/768 connection for only $19.95/mo for the first 3 months!
http://ads.osdn.com/?ad_id=2562&alloc_id=6184&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] DefineSimpleCache macro problem

[J. Bruce Fields]

On Thu, May 13, 2004 at 12:45:52PM +0200, Olaf Kirch wrote:
> On Thu, May 13, 2004 at 07:36:27PM +1000, Neil Brown wrote:
> > That would be wrong.  Always returning an entry (possibly non-VALID)
> > - except on kmalloc failure - is a design feature.
> 
> Does this mean I can eat all your memory by flooding your NFS server
> with NULL calls from bogus addresses? That would be bad.

There is an expiration time on those entries (2 minutes), but I don't
see any bound on the number of them.

I'm guessing one of those ip_map cache entries is about 80 bytes on
i386?  I'm not sure what a reasonable assumption is about packet rates,
but it seems like it could be a problem, unless I'm missing some other
constraint here.

> The problem is that somewhere somehow an invalid entry is created,
> which remains in the cache and the flags don't get updated.

Ugh, and it's the failure to update the flags that's the bug, I suspect.
Does this help?  (Caveat: looks straightforward enough, but I haven't
actually tested it!)

--b.

Normally the _lookup functions are only called with set = 1 on items with
CACHE_NEGATIVE not set; however in the case of the export cache it can happen,
and is especially likely if mountd isn't listening in on the
proc/net/rpc/*/content files, since then any lookup (if followed by
cache_check()) generates a CACHE_NEGATIVE entry.

Make sure we clear the CACHE_NEGATIVE bit on update if necessary in
DefineCacheLookup.


 include/linux/sunrpc/cache.h |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletion(-)

diff -puN include/linux/sunrpc/cache.h~nfsd_cache_update_negative include/linux/sunrpc/cache.h
--- linux-2.6.6-rc3/include/linux/sunrpc/cache.h~nfsd_cache_update_negative	2004-05-13 14:44:13.000000000 -0400
+++ linux-2.6.6-rc3-bfields/include/linux/sunrpc/cache.h	2004-05-13 14:45:47.000000000 -0400
@@ -195,7 +195,10 @@ RTN *FNAME ARGS										\
 				}							\
 				if (test_bit(CACHE_NEGATIVE,  &item->MEMBER.flags))	\
 					 set_bit(CACHE_NEGATIVE, &tmp->MEMBER.flags);	\
-				else {UPDATE;}						\
+				else {							\
+					clear_bit(CACHE_NEGATIVE, &tmp->MEMBER.flags); \
+					UPDATE;						\
+				}							\
 			}								\
 			if (set||new) write_unlock(&(DETAIL)->hash_lock);		\
 			else read_unlock(&(DETAIL)->hash_lock);				\

_


-------------------------------------------------------
This SF.Net email is sponsored by: SourceForge.net Broadband
Sign-up now for SourceForge Broadband and get the fastest
6.0/768 connection for only $19.95/mo for the first 3 months!
http://ads.osdn.com/?ad_id=2562&alloc_id=6184&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] DefineSimpleCache macro problem

[Neil Brown]

On Thursday May 13, okir@suse.de wrote:
> 
> I haven't yet figured out exactly what happens, but _something_ calls
> exp_get_by_name for /boot prior to mount's call. This leaves an invalid
> item in the cache, and svc_export_update doesn't fix the flags.

My only guess for what could be calling exp_get_by_name is exp_parent,
which would be called when user-space asked for a file handle
at-or-below the point in question.  But I don't know if or why that is
happening.

Bruce's patch is the right thing to fix the problem, though I would
clear the bit *after* the UPDATE has been completed, just to make sure
no-one see an inconsistent structure.  Thanks Bruce.

> > Also, are you running with the nfsd filesystem mounted on
> > /proc/fs/nfsd (or /proc/fs/nfs) or with it unmounted?
> 
> The nfsd file system is not mounted, but I think this shouldn't 
> make a difference.

It makes a big difference to how exportfs and mountd do their stuff.
They are less pro-active about giving information to the kernel, and
more reactive, only giving it when asked for.

NeilBrown


-------------------------------------------------------
This SF.Net email is sponsored by: SourceForge.net Broadband
Sign-up now for SourceForge Broadband and get the fastest
6.0/768 connection for only $19.95/mo for the first 3 months!
http://ads.osdn.com/?ad_id=2562&alloc_id=6184&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs