rpc.mountd crashes when extensively using netgroups

rpc.mountd crashes when extensively using netgroups

[Stefan Walter]

Hi all,

we are seeing rpc.mountd crashes on our Red Hat EL4 systems.
We have tracked down the bug and it seems to be still present
in the current nfs-utils source.

We are making extensive use of netgroups for NFS exports. On
a large file server with hundreds of home directories we export
every directory to a unique netgroup. Member netgroups are used
to export to sets of machines. The following example illustrates
what we do:

# cat /etc/exports
/export/home/jane @jane(async,rw,no_subtree_check,fsid=10000)
/export/home/joe @joe(async,rw,no_subtree_check,fsid=10001)
# cat /etc/netgroup
lab_1 (workstation1,,) (workstation2,,) (workstation3)
offices_1 (workstation4,,) (workstation5,,)
jane lab_1 offices_1
joe offices_1 (joeslaptop,,)

We do this on a much larger scale though. The bug we ran into is
in line 96 in utils/mountd/auth.c. The strcpy can corrupt
memory when it copies the string returned by client_compose() to
my_client.m_hostname which has a fixed size of 1024 bytes. 
For our example above, client_compose() returns "@joe,@jane"
for any machine in the offices_1 netgroup. Unfortunately we have
a machine to which roughly 150 netgroups like @joe or @jane
export to and client_compose() returns a string over 1300 bytes
long and rpc.mountd nicely segfaults.
 
To prevent the crash is of course trivial: Inserting a simple
'if (strlen(n) > 1024) return NULL;' before line 96 does the job.

There are however two issues for which we could not find an easy
solution:

 1. For every client rpc.mountd and the kernel seem to exchange
    and use lists with _all_ netgroups used in exports that are
    relevant for granting permission to some share for a particular
    client. We could imagine two optimizations here:

       * Resolve netgroups and only put the (member) netgroups that
         contained the host name that would be used to authorize
         a mount in the list.

       * Use the list of mounted paths per client and only put the
         netgroup(s) used to export paths that are actually mounted
         on a client. 

    This also caused us severe performance problems because
    rpc.mountd queries all these netgroups. We were initially using
    a LDAP and mouting a directory took up to ten seconds
    during which rpc.mountd was busily querying the LDAP server.
    We got this down to two seconds using file based netgroups.
 
 2. Using a fixed size for NFSCLNT_IDMAX does not scale. Mounting
    shares on a client for which the 'if' clause of the quick fix
    becomes true will not be possible. We thought about enlarging
    NFSCLNT_IDMAX and using a custom kernel but dropped the idea. 

Our ultimate goal is to get Red Hat fix the code in nfs-utils 1.0.6
that is used in RHEL4. A first step would be to get a suitable fix in
the current nfs-utils. 

Is there somebody on the mailing list who could see an easy fix or
would have an opinion on how to best address the issues we see?

Thanks in advance and best regards,

Stefan

Re: rpc.mountd crashes when extensively using netgroups

[Steve Dickson]

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

Stefan Walter wrote:
> 
> We do this on a much larger scale though. The bug we ran into is
> in line 96 in utils/mountd/auth.c. The strcpy can corrupt
> memory when it copies the string returned by client_compose() to
> my_client.m_hostname which has a fixed size of 1024 bytes. 
> For our example above, client_compose() returns "@joe,@jane"
> for any machine in the offices_1 netgroup. Unfortunately we have
> a machine to which roughly 150 netgroups like @joe or @jane
> export to and client_compose() returns a string over 1300 bytes
> long and rpc.mountd nicely segfaults.
>  
> To prevent the crash is of course trivial: Inserting a simple
> 'if (strlen(n) > 1024) return NULL;' before line 96 does the job.
Does the attached patch help?

> 
> There are however two issues for which we could not find an easy
> solution:
> 
>  1. For every client rpc.mountd and the kernel seem to exchange
>     and use lists with _all_ netgroups used in exports that are
>     relevant for granting permission to some share for a particular
>     client. We could imagine two optimizations here:
> 
>        * Resolve netgroups and only put the (member) netgroups that
>          contained the host name that would be used to authorize
>          a mount in the list.
> 
>        * Use the list of mounted paths per client and only put the
>          netgroup(s) used to export paths that are actually mounted
>          on a client. 
These sound reasonable...


>  
>  2. Using a fixed size for NFSCLNT_IDMAX does not scale. Mounting
>     shares on a client for which the 'if' clause of the quick fix
>     becomes true will not be possible. We thought about enlarging
>     NFSCLNT_IDMAX and using a custom kernel but dropped the idea. 
True...

> 
> Our ultimate goal is to get Red Hat fix the code in nfs-utils 1.0.6
> that is used in RHEL4. A first step would be to get a suitable fix in
> the current nfs-utils. 
Please open up bugs on all three of these issues and
we'll see what can done...

steved.


[-- Attachment #2: mountd-netgroup.patch --]
[-- Type: text/x-patch, Size: 763 bytes --]

commit 851ce1cb766cf295db85900aab804c0f82c12ab3
Author: Steve Dickson <steved@redhat.com>
Date:   Tue Jul 31 09:57:19 2007 -0400

    Stop rpc.mound from crashing by m_hostname becoming
    corrupted with very long host names.
    
    Signed-off-by: Steve Dickson <steved@redhat.com>

diff --git a/utils/mountd/auth.c b/utils/mountd/auth.c
index f7fe23d..eff0ba7 100644
--- a/utils/mountd/auth.c
+++ b/utils/mountd/auth.c
@@ -93,7 +93,8 @@ auth_authenticate_internal(char *what, struct sockaddr_in *caller,
 		*error = unknown_host;
 		if (!n)
 			return NULL;
-		strcpy(my_client.m_hostname, *n?n:"DEFAULT");
+		snprintf(my_client.m_hostname, (NFSCLNT_IDMAX+1), 
+			"%s", *n?n:"DEFAULT");
 		free(n);
 		my_client.m_naddr = 1;
 		my_exp.m_client = &my_client;

Re: rpc.mountd crashes when extensively using netgroups

[Stefan Walter]

Steve Dickson wrote:
> Stefan Walter wrote:
>>
>> We do this on a much larger scale though. The bug we ran into is
>> in line 96 in utils/mountd/auth.c. The strcpy can corrupt
>> memory when it copies the string returned by client_compose() to
>> my_client.m_hostname which has a fixed size of 1024 bytes. For our
>> example above, client_compose() returns "@joe,@jane"
>> for any machine in the offices_1 netgroup. Unfortunately we have
>> a machine to which roughly 150 netgroups like @joe or @jane
>> export to and client_compose() returns a string over 1300 bytes
>> long and rpc.mountd nicely segfaults.
>>  
>> To prevent the crash is of course trivial: Inserting a simple
>> 'if (strlen(n) > 1024) return NULL;' before line 96 does the job.
> Does the attached patch help?
>
rpc.mountd does not crash anymore but I get a 'permission denied' when
trying
to mount a share. Doing an 'strace rpc.mountd -F' reveals:

...
open("/proc/net/rpc/auth.unix.ip/channel", O_WRONLY|O_CREAT|O_TRUNC,
0666) = 9
fstat64(9, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0
mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
0) = 0xb7f41000
time(NULL)                              = 1186041882
write(9, "nfsd 129.132.10.33 1186043682 @a"..., 1024) = -1 EINVAL
(Invalid argument)
write(9, "\n", 1)                       = -1 EINVAL (Invalid argument)
close(9)                                = 0
munmap(0xb7f41000, 4096)                = 0
open("/proc/net/rpc/nfsd.export/channel", O_WRONLY|O_CREAT|O_TRUNC,
0666) = 9
fstat64(9, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0
mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
0) = 0xb7f41000
write(9, "@anbuehle,@anhorni,@antoinet,@ap"..., 1024) = -1 EINVAL
(Invalid argument)
time(NULL)                              = 1186041882
write(9, "/export/groups/grossm/h1/home/gr"..., 68) = -1 ENOENT (No such
file or directory)
close(9)                                = 0
munmap(0xb7f41000, 4096)                = 0
open("/proc/fs/nfsd/filehandle", O_RDWR) = 9
fstat64(9, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0
mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
0) = 0xb7f41000
write(9, "@anbuehle,@anhorni,@antoinet,@ap"..., 1066) = -1 EPERM
(Operation not permitted)
...

>>
>> Our ultimate goal is to get Red Hat fix the code in nfs-utils 1.0.6
>> that is used in RHEL4. A first step would be to get a suitable fix in
>> the current nfs-utils. 
> Please open up bugs on all three of these issues and
> we'll see what can done...
>

Just did that (request IDs 1765949, 1765938 and 1765930 on sourceforge).

- Stefan

Re: rpc.mountd crashes when extensively using netgroups

[Satyam Sharma]

Hi,


On Thu, 2 Aug 2007, Stefan Walter wrote:

> Steve Dickson wrote:
> > Stefan Walter wrote:
> >>
> >> We do this on a much larger scale though. The bug we ran into is
> >> in line 96 in utils/mountd/auth.c. The strcpy can corrupt
> >> memory when it copies the string returned by client_compose() to
> >> my_client.m_hostname which has a fixed size of 1024 bytes. For our
> >> example above, client_compose() returns "@joe,@jane"
> >> for any machine in the offices_1 netgroup. Unfortunately we have
> >> a machine to which roughly 150 netgroups like @joe or @jane
>> export to and client_compose() returns a string over 1300 bytes
> >> long and rpc.mountd nicely segfaults.
> >>  
> >> To prevent the crash is of course trivial: Inserting a simple
> >> 'if (strlen(n) > 1024) return NULL;' before line 96 does the job.
> > Does the attached patch help?
> >
> rpc.mountd does not crash anymore but I get a 'permission denied' when
> trying
> to mount a share. Doing an 'strace rpc.mountd -F' reveals:
> 
> ...
> open("/proc/net/rpc/auth.unix.ip/channel", O_WRONLY|O_CREAT|O_TRUNC,
> 0666) = 9
> fstat64(9, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0
> mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
> 0) = 0xb7f41000
> time(NULL)                              = 1186041882
> write(9, "nfsd 129.132.10.33 1186043682 @a"..., 1024) = -1 EINVAL
> (Invalid argument)
> write(9, "\n", 1)                       = -1 EINVAL (Invalid argument)
> close(9)                                = 0
> munmap(0xb7f41000, 4096)                = 0
> open("/proc/net/rpc/nfsd.export/channel", O_WRONLY|O_CREAT|O_TRUNC,
> 0666) = 9
> fstat64(9, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0
> mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
> 0) = 0xb7f41000
> write(9, "@anbuehle,@anhorni,@antoinet,@ap"..., 1024) = -1 EINVAL
> (Invalid argument)
> time(NULL)                              = 1186041882
> write(9, "/export/groups/grossm/h1/home/gr"..., 68) = -1 ENOENT (No such
> file or directory)
> close(9)                                = 0
> munmap(0xb7f41000, 4096)                = 0
> open("/proc/fs/nfsd/filehandle", O_RDWR) = 9
> fstat64(9, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0
> mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
> 0) = 0xb7f41000
> write(9, "@anbuehle,@anhorni,@antoinet,@ap"..., 1066) = -1 EPERM
> (Operation not permitted)
> ...

Yup, the snprintf() in the patch would've truncated the input string.

Steve (D), you should check the return of snprintf() and compare against
the size specified (NFSCLNT_IDMAX+1) and do a graceful cleanup + print
an error message to the user, when detecting truncation of input:


err = snprintf(my_client.m_hostname, (NFSCLNT_IDMAX+1), "%s", *n?n:"DEFAULT");
if (err >= (NFSCLNT_IDMAX+1)) {
	printf("too large input string ...\n");
	/* cleanups and graceful exit */
}


Sorry, I don't have rpc.mountd sources nearby, so cannot make a patch
myself (I'm an exclusively kernel guy :-)


Thanks,
Satyam

Re: rpc.mountd crashes when extensively using netgroups

[Steve Dickson]

Satyam Sharma wrote:
> Hi,
> 
> 
> On Thu, 2 Aug 2007, Stefan Walter wrote:
> 
>> Steve Dickson wrote:
>>> Stefan Walter wrote:
>>>> We do this on a much larger scale though. The bug we ran into is
>>>> in line 96 in utils/mountd/auth.c. The strcpy can corrupt
>>>> memory when it copies the string returned by client_compose() to
>>>> my_client.m_hostname which has a fixed size of 1024 bytes. For our
>>>> example above, client_compose() returns "@joe,@jane"
>>>> for any machine in the offices_1 netgroup. Unfortunately we have
>>>> a machine to which roughly 150 netgroups like @joe or @jane
>>> export to and client_compose() returns a string over 1300 bytes
>>>> long and rpc.mountd nicely segfaults.
>>>>  
>>>> To prevent the crash is of course trivial: Inserting a simple
>>>> 'if (strlen(n) > 1024) return NULL;' before line 96 does the job.
>>> Does the attached patch help?
>>>
>> rpc.mountd does not crash anymore but I get a 'permission denied' when
>> trying
>> to mount a share. Doing an 'strace rpc.mountd -F' reveals:
>>
>> ...
>> open("/proc/net/rpc/auth.unix.ip/channel", O_WRONLY|O_CREAT|O_TRUNC,
>> 0666) = 9
>> fstat64(9, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0
>> mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
>> 0) = 0xb7f41000
>> time(NULL)                              = 1186041882
>> write(9, "nfsd 129.132.10.33 1186043682 @a"..., 1024) = -1 EINVAL
>> (Invalid argument)
>> write(9, "\n", 1)                       = -1 EINVAL (Invalid argument)
>> close(9)                                = 0
>> munmap(0xb7f41000, 4096)                = 0
>> open("/proc/net/rpc/nfsd.export/channel", O_WRONLY|O_CREAT|O_TRUNC,
>> 0666) = 9
>> fstat64(9, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0
>> mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
>> 0) = 0xb7f41000
>> write(9, "@anbuehle,@anhorni,@antoinet,@ap"..., 1024) = -1 EINVAL
>> (Invalid argument)
>> time(NULL)                              = 1186041882
>> write(9, "/export/groups/grossm/h1/home/gr"..., 68) = -1 ENOENT (No such
>> file or directory)
>> close(9)                                = 0
>> munmap(0xb7f41000, 4096)                = 0
>> open("/proc/fs/nfsd/filehandle", O_RDWR) = 9
>> fstat64(9, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0
>> mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
>> 0) = 0xb7f41000
>> write(9, "@anbuehle,@anhorni,@antoinet,@ap"..., 1066) = -1 EPERM
>> (Operation not permitted)
>> ...
> 
> Yup, the snprintf() in the patch would've truncated the input string.
> 
> Steve (D), you should check the return of snprintf() and compare against
> the size specified (NFSCLNT_IDMAX+1) and do a graceful cleanup + print
> an error message to the user, when detecting truncation of input:
> 
> 
> err = snprintf(my_client.m_hostname, (NFSCLNT_IDMAX+1), "%s", *n?n:"DEFAULT");
> if (err >= (NFSCLNT_IDMAX+1)) {
> 	printf("too large input string ...\n");
> 	/* cleanups and graceful exit */
> }
> 
cool... thanks!

steved.

Re: rpc.mountd crashes when extensively using netgroups

[J. Bruce Fields]

On Mon, Jul 30, 2007 at 02:55:14PM +0200, Stefan Walter wrote:
> There are however two issues for which we could not find an easy
> solution:
> 
>  1. For every client rpc.mountd and the kernel seem to exchange
>     and use lists with _all_ netgroups used in exports that are
>     relevant for granting permission to some share for a particular
>     client. We could imagine two optimizations here:
> 
>        * Resolve netgroups and only put the (member) netgroups that
>          contained the host name that would be used to authorize
>          a mount in the list.
> 
>        * Use the list of mounted paths per client and only put the
>          netgroup(s) used to export paths that are actually mounted
>          on a client. 
> 
>     This also caused us severe performance problems because
>     rpc.mountd queries all these netgroups. We were initially using
>     a LDAP and mouting a directory took up to ten seconds
>     during which rpc.mountd was busily querying the LDAP server.
>     We got this down to two seconds using file based netgroups.
>  
>  2. Using a fixed size for NFSCLNT_IDMAX does not scale. Mounting
>     shares on a client for which the 'if' clause of the quick fix
>     becomes true will not be possible. We thought about enlarging
>     NFSCLNT_IDMAX and using a custom kernel but dropped the idea. 
> 
> Our ultimate goal is to get Red Hat fix the code in nfs-utils 1.0.6
> that is used in RHEL4. A first step would be to get a suitable fix in
> the current nfs-utils. 

That's an interesting problem.  Thanks for the report!

I don't believe that long comma-delimited string actually has any
meaning to the kernel--as far as the kernel is concerned, it's just an
opaque object that will be passed back to mountd later (along with a
path name) to get export options.

So I suppose that string could be replaced by a hash, or maybe even just
by the ip address of the particular host--the disadvantage to the latter
being that it would require the kernel to keep a separate export for
each client address.

--b.

Re: [NFS] rpc.mountd crashes when extensively using netgroups

[Jeff Layton]

On Tue, 31 Jul 2007 10:48:24 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Mon, Jul 30, 2007 at 02:55:14PM +0200, Stefan Walter wrote:
> > There are however two issues for which we could not find an easy
> > solution:
> > 
> >  1. For every client rpc.mountd and the kernel seem to exchange
> >     and use lists with _all_ netgroups used in exports that are
> >     relevant for granting permission to some share for a particular
> >     client. We could imagine two optimizations here:
> > 
> >        * Resolve netgroups and only put the (member) netgroups that
> >          contained the host name that would be used to authorize
> >          a mount in the list.
> > 
> >        * Use the list of mounted paths per client and only put the
> >          netgroup(s) used to export paths that are actually mounted
> >          on a client. 
> > 
> >     This also caused us severe performance problems because
> >     rpc.mountd queries all these netgroups. We were initially using
> >     a LDAP and mouting a directory took up to ten seconds
> >     during which rpc.mountd was busily querying the LDAP server.
> >     We got this down to two seconds using file based netgroups.
> >  
> >  2. Using a fixed size for NFSCLNT_IDMAX does not scale. Mounting
> >     shares on a client for which the 'if' clause of the quick fix
> >     becomes true will not be possible. We thought about enlarging
> >     NFSCLNT_IDMAX and using a custom kernel but dropped the idea. 
> > 
> > Our ultimate goal is to get Red Hat fix the code in nfs-utils 1.0.6
> > that is used in RHEL4. A first step would be to get a suitable fix in
> > the current nfs-utils. 
> 
> That's an interesting problem.  Thanks for the report!
> 
> I don't believe that long comma-delimited string actually has any
> meaning to the kernel--as far as the kernel is concerned, it's just an
> opaque object that will be passed back to mountd later (along with a
> path name) to get export options.
> 
> So I suppose that string could be replaced by a hash, or maybe even just
> by the ip address of the particular host--the disadvantage to the latter
> being that it would require the kernel to keep a separate export for
> each client address.
> 
> --b.
> 

I started having a look at this today. The original patches that I
proposed to clean up the rmtab a few months ago also eliminated this
comma-delimited string. Neil had valid objections to it at the time,
but if we switched to using the IP address as a cache key like Bruce
describes then doing that becomes more reasonable.

The only downside I see is the one Bruce points out -- the size of
the kernel export cache would increase. I don't have a feel for whether
this is a show stopper, however.

Neil, do you have thoughts on what you'd consider a reasonable approach
to fixing this?

-- 
Jeff Layton <jlayton@redhat.com>

Re: [NFS] rpc.mountd crashes when extensively using netgroups

[J. Bruce Fields]

On Thu, Aug 02, 2007 at 11:32:55AM -0400, Jeff Layton wrote:
> On Tue, 31 Jul 2007 10:48:24 -0400
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > That's an interesting problem.  Thanks for the report!
> > 
> > I don't believe that long comma-delimited string actually has any
> > meaning to the kernel--as far as the kernel is concerned, it's just an
> > opaque object that will be passed back to mountd later (along with a
> > path name) to get export options.
> > 
> > So I suppose that string could be replaced by a hash, or maybe even just
> > by the ip address of the particular host--the disadvantage to the latter
> > being that it would require the kernel to keep a separate export for
> > each client address.
> 
> I started having a look at this today. The original patches that I
> proposed to clean up the rmtab a few months ago also eliminated this
> comma-delimited string. Neil had valid objections to it at the time,
> but if we switched to using the IP address as a cache key like Bruce
> describes then doing that becomes more reasonable.
> 
> The only downside I see is the one Bruce points out -- the size of
> the kernel export cache would increase. I don't have a feel for whether
> this is a show stopper, however.

Yeah, there might be some risk of solving that problem at the expense of
people with tons of clients all matching *.example.com.  The actual
export objects are actually pretty small--68 bytes, last I checked?
You'd also need two upcalls to mountd per client (for ip address and
export, as opposed to just ip address).  I don't know what other costs
there might be.

What about the idea of hashing the comma-delimited list and passing
that?  You'd want hash large enough to be collision-free--might as well
use some cryptographic hash, I guess, though it may be mild overkill.
Is there any reason why that wouldn't work?

Could you remind me what problems you were trying to fix with your rmtab
cleanup?

--b.

Re: [NFS] rpc.mountd crashes when extensively using netgroups

[Jeff Layton]

On Thu, 2 Aug 2007 12:05:36 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Thu, Aug 02, 2007 at 11:32:55AM -0400, Jeff Layton wrote:
> > On Tue, 31 Jul 2007 10:48:24 -0400
> > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > > That's an interesting problem.  Thanks for the report!
> > > 
> > > I don't believe that long comma-delimited string actually has any
> > > meaning to the kernel--as far as the kernel is concerned, it's just an
> > > opaque object that will be passed back to mountd later (along with a
> > > path name) to get export options.
> > > 
> > > So I suppose that string could be replaced by a hash, or maybe even just
> > > by the ip address of the particular host--the disadvantage to the latter
> > > being that it would require the kernel to keep a separate export for
> > > each client address.
> > 
> > I started having a look at this today. The original patches that I
> > proposed to clean up the rmtab a few months ago also eliminated this
> > comma-delimited string. Neil had valid objections to it at the time,
> > but if we switched to using the IP address as a cache key like Bruce
> > describes then doing that becomes more reasonable.
> > 
> > The only downside I see is the one Bruce points out -- the size of
> > the kernel export cache would increase. I don't have a feel for whether
> > this is a show stopper, however.
> 
> Yeah, there might be some risk of solving that problem at the expense of
> people with tons of clients all matching *.example.com.  The actual
> export objects are actually pretty small--68 bytes, last I checked?
> You'd also need two upcalls to mountd per client (for ip address and
> export, as opposed to just ip address).  I don't know what other costs
> there might be.
> 

Yeah, that could make using the ipaddr too costly...

> What about the idea of hashing the comma-delimited list and passing
> that?  You'd want hash large enough to be collision-free--might as well
> use some cryptographic hash, I guess, though it may be mild overkill.
> Is there any reason why that wouldn't work?
> 

Hashing would certainly work. The only issue though is that this
doesn't take care of the second problem, which is that NFSCLNT_IDMAX is
too small for some environments. So this change would need to be
coupled with an increase in this hardcoded value, change m_hostname to
be dynamically allocated, or change the comma-delimited string to some
other scheme entirely (maybe a linked list of some sort?).

As far as the hashing scheme, I guess we could use the glibc crypt()
routine that does md5 hashing. I'd like to avoid adding dependencies
on new libs if possible.

> Could you remind me what problems you were trying to fix with your rmtab
> cleanup?
>

Those patches were really intended to get showmount -a to look correct.
My first pass at it changed the m_hostname field to hold an actual
hostname, got rid of the comma-separated strings and just had mountd do
repeated calls to client_check to check the host's validity.

This was problematic though with a multi-homed client though since the
export table key then became a hostname rather than the comma-delimited
string and more than 1 ip addr could resolve to it. If we use the ipaddr
as a key, then this problem goes away, but then we have the other issues
you've detailed above.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [NFS] rpc.mountd crashes when extensively using netgroups

[Neil Brown]

On Thursday August 2, jlayton@redhat.com wrote:
> 
> I started having a look at this today. The original patches that I
> proposed to clean up the rmtab a few months ago also eliminated this
> comma-delimited string. Neil had valid objections to it at the time,
> but if we switched to using the IP address as a cache key like Bruce
> describes then doing that becomes more reasonable.
> 
> The only downside I see is the one Bruce points out -- the size of
> the kernel export cache would increase. I don't have a feel for whether
> this is a show stopper, however.
> 
> Neil, do you have thoughts on what you'd consider a reasonable approach
> to fixing this?
> 

I'm having troubling visualising the problematic setup.  Is it
possible to get a copy of the /etc/exports file, and some idea of what
hosts are in which netgroups?  Knowing that would help assess the
appropriateness of various solutions.


The core issue is this:

 We need to map IPADDRESS to THING, and  THING + PATH to EXPORTPOINT.
(roughly).
So what do we choose for "THING"?

One obvious option is the dotted quad of the IP address.
People tend to have large sets of similar clients, so doing this seems
to be missing out on a valuable optimisation and adding bloat to the
lookup tables in the kernel.

Another apparently obvious solution is to use FQDN, but we really
don't want to do that, as a multihomed host might have one IP address
that you want to trust, and another that you don't (as the subnet is
less secure against forgery).  It doesn't help must over dotted-quad
anyway.

Another option is to use whatever strings are included in /etc/exports
to identify things.  e.g. *.my.domain or @mynetgroup.
This seems simple and elegant, but it can be messy in some
situations.  In particular, if one IP address matches several strings,
which one do you use?  We have to use them all.
One path might be exported to *.my.domain and a different path might
be exported to @mynetgroup.  If a.b.c.d is in both of those sets of
machines, then the THING that a.b.c.d is mapped to must allow the
correct
    THING + PATH -> EXPORTPOINT
mapping for both paths.
    *.my.domain,@mynetgroup
works quite effectively.
But if you have lots and lots of different netgroups mentioned in
/etc/exports, and one machine is in all of those netgroups, then you
have to generate a very long string @a,@b,@c,@d,.....

I guess we could use a hash and keep a hash table in mountd.
So mountd:
  1/ Generates the comma-separated name as needed, being careful to
     allocate enough space.
  2a/ If this is small enough, write it to the kernel as is.
  2b/ If it is too big, Find it in a table (adding if needed) and 
      write the address of the table entry to the kernel

  When a THING + PATH request arrives:
    If THING looks like an address in the table, do the lookup to get
    the string.

This would work to keep the strings in the kernel shorter, but is
rather ugly - storing in the kernel addresses in a user-space program.

Also, if you have lots of netgroups mentioned, then finding out which
ones contain a given IP address might be slow, and all the information
isn't really needed.  Once you get THING, it will be paired with a
specific path, so you only really need to look up netgroups that are
related to that path.

So maybe we want to combine the two workable approaches.

Sometimes IPADDRESS maps to DOTTED_QUAD
Sometimes IPADDRESS maps to LIST,OF,STRINGS,FROM,ETC,EXPORTS

Possibly the choice could be based on a command line switch.
In the absence of such a switch, it could be based on the number of
entries in /etc/exports.

	int i;
	int size = 0;
	nfs_client	*clp;

	for (i = 0 ; i < MCL_MAXTYPES; i++)
		for (clp = clientlist[i]; clp ; clp = clp->m_next)
			size += strlen(clp->m_hostname)+1;

	if (size > 1000)
		use DOTTED_QUAD;
	els
		don't.


??

NeilBrown

Re: [NFS] rpc.mountd crashes when extensively using netgroups

[Stefan Walter]

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

Neil Brown wrote:
> I'm having troubling visualising the problematic setup.  Is it
> possible to get a copy of the /etc/exports file, and some idea of what
> hosts are in which netgroups?  Knowing that would help assess the
> appropriateness of various solutions.
>   
The attached file contains a synthetic configuration with 500 exports
that triggers the bug. Put some useful hostnames in the workstations
netgroup
in the enclosed /etc/netgroups, start the nfs service and try to mount
one of the exported directories from a host you put in the workstations
netgroup.

The mount should fail. Whether mountd crashes or not seems to depend on
previous mounts from the host. You may need to start with a truncated
/etc/exports,
successfully mount a shared path on a host, export the full /etc/exports and
mount another share on the host.

- Stefan


[-- Attachment #2: files.tar.gz --]
[-- Type: application/x-gzip, Size: 8098 bytes --]

Re: [NFS] rpc.mountd crashes when extensively using netgroups

[J. Bruce Fields]

On Fri, Aug 03, 2007 at 01:01:09PM +1000, Neil Brown wrote:
> The core issue is this:
> 
>  We need to map IPADDRESS to THING, and  THING + PATH to EXPORTPOINT.
> (roughly).
> So what do we choose for "THING"?
> 
> One obvious option is the dotted quad of the IP address.
> People tend to have large sets of similar clients, so doing this seems
> to be missing out on a valuable optimisation and adding bloat to the
> lookup tables in the kernel.

$ gdb vmlinux
GNU gdb 6.6-debian
...
(gdb) print sizeof(struct ip_map)
$1 = 40
(gdb) print sizeof(struct svc_export)
$2 = 136

Hm, OK, one-address-per-export loses.

> I guess we could use a hash and keep a hash table in mountd.
> So mountd:
>   1/ Generates the comma-separated name as needed, being careful to
>      allocate enough space.
>   2a/ If this is small enough, write it to the kernel as is.
>   2b/ If it is too big, Find it in a table (adding if needed) and 
>       write the address of the table entry to the kernel
> 
>   When a THING + PATH request arrives:
>     If THING looks like an address in the table, do the lookup to get
>     the string.
> 
> This would work to keep the strings in the kernel shorter, but is
> rather ugly - storing in the kernel addresses in a user-space program.

If you use an actual collision-free hash function then the result will
be reproduceable, so you don't have to depend on temporary information
in mountd.

> So maybe we want to combine the two workable approaches.
> 
> Sometimes IPADDRESS maps to DOTTED_QUAD
> Sometimes IPADDRESS maps to LIST,OF,STRINGS,FROM,ETC,EXPORTS
> 
> Possibly the choice could be based on a command line switch.
> In the absence of such a switch, it could be based on the number of
> entries in /etc/exports.

Seems OK.

--b.