DefineSimpleCache macro problem

PATCH In review Olaf Kirch 3 participants 5 messages May 13, 2004 – May 14, 2004

Subsystems (3)

Networking NFS client NFS server

lore

Share & tools

lore

Olaf Kirch

author

2004-05-13 08:26 UTC #0 lore raw

PATCH 1/1 DefineSimpleCache macro problem

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

Signature · 3 lines

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

include/linux/sunrpc/cache.h renamed from include/linux/sunrpc/cache.h.bug +1 -1

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

net/sunrpc/svcauth_unix.c renamed from net/sunrpc/svcauth_unix.c.aix +7 -1

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

Neil Brown

2004-05-13 09:36 UTC #1 lore raw

Quoted 8 lines · On Thursday May 13, [email protected] 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.

Quoted 8 lines

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

Olaf Kirch

author

2004-05-13 10:45 UTC #2 lore raw

Quoted 2 lines · Neil Brown

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.

Quoted 6 lines

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 ....

Quoted 2 lines

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

Signature · 13 lines

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


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

J. Bruce Fields

2004-05-13 19:15 UTC #3 lore raw

Quoted 6 lines · Olaf Kirch

Quoted 2 lines · Neil Brown
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.

Quoted 2 lines

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

include/linux/sunrpc/cache.h renamed from include/linux/sunrpc/cache.h~nfsd_cache_update_negative +4 -1

diff -puN include/linux/sunrpc/cache.h~nfsd_cache_update_negative include/linux/sunrpc/cache.h
@@ -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);				\

Neil Brown

2004-05-14 01:20 UTC #4 lore raw

In reply to Olaf Kirch

Quoted 4 lines · On Thursday May 13, [email protected] 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.

Quoted 5 lines

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