2011-04-04 08:03:02

by Sean Finney

[permalink] [raw]
Subject: Fixes for NFS in environments with large group memberships

SGkgVGhlcmUhCgpMYXN0IHdlZWsgSSBmaXJlZCBvZmYgYSBjb3VwbGUgcGF0Y2hlcyB0byB0aGlz
IGxpc3QgYW5kIGluIGhpbmRzaWdodCBJCnJlYWxpemUgdGhhdCBJIG1pZ2h0IG5vdCBoYXZlIGJl
ZW4gdG90YWxseSBjbGVhciBmb3IgdGhlIG1vdGl2YXRpb24KYmVoaW5kIHRoZSBwYXRjaGVzOgoK
W1BBVENIXSBVc2UgYSBuYW1lZCBjb25zdGFudCBmb3IgbWF4IG51bWJlciBvZiBtYW5hZ2VkIGdy
b3VwcywgYW5kIGluYwpbUEFUQ0hdIEluY3JlYXNlIHRoZSBkZWZhdWx0IGJ1ZmZlciBzaXplIGZv
ciBjYWNoZWxpc3QgY2hhbm5lbCBmaWxlcy4KClNpbmNlIG5vYm9keSBoYXMgcmV2aWV3ZWQgdGhl
bSB5ZXQsIEkgdGhvdWdodCBJJ2QgbWFrZSBpdCBjbGVhciB0aGF0CnRoZXNlIGNoYW5nZXMgKG9y
IHNvbWV0aGluZyBhbG9uZyBzdWNoIGxpbmVzKSBhcmUgcmVxdWlyZWQgZm9yIE5GUwphY2Nlc3Mg
dG8gbW91bnRwb2ludHMgd2hlbiB1c2VycyBhcmUgaW4gbGFyZ2Vpc2ggKD4xMDApIG51bWJlcnMg
b2YKZ3JvdXBzLiAgVGhlcmUgYXJlIGFjdHVhbGx5IHR3byBwcm9ibGVtczoKCiogSGFyZC1jb2Rl
ZCBsaW1pdCBmb3IgZ3JvdXAgbWVtYmVyc2hpcCBhdCAxMDAgZ3JvdXBzIGluIG1vdW50ZCBzb3Vy
Y2UKKiBMYXJnZSBzZXJ2ZXItc2lkZSBwcm9jZnMgd3JpdGVzIGFyZSBzcGxpdCBpbnRvIG11bGl0
cGxlIHdyaXRlcyBkdWUgdG8Kc3RkaW8gYnVmZmVyaW5nIGluIG1vdW50ZCBhbmQgc3ZjZ3NzZCBz
b3VyY2VzLCByZXN1bHRpbmcgaW4gY2xpZW50CmhhbmdzLgoKVGhlIHBhdGNoZXMgaW4gcXVlc3Rp
b24gc29sdmUgdGhpcyBpbiBhcyBub24taW50cnVzaXZlIGEgbWFubmVyIGFzCnBvc3NpYmxlLCB0
aG91Z2ggSSdtIG9mIGNvdXJzZSBvcGVuIHRvIGNoYW5naW5nIHRoaW5ncyB1cCBpZiB5b3UgZmVl
bCBpdApzaG91bGQgYmUgZG9uZSBvdGhlcndpc2UuICBTbyBwbGVhc2UgbGV0IG1lIGtub3cgd2hh
dCB5b3UgdGhpbmssIG9yIGlmCml0IHdvdWxkIGJlIGJldHRlciBmb3IgbWUgdG8gZmlyc3QgZ28g
YW5kIG9wZW4gYSBidWcsIHN0YXJ0IGEKZGlzY3Vzc2lvbiwgZXRjLgoKClRoYW5rcywKU2VhbiAK
Tu+/ve+/ve+/ve+/ve+/vXLvv73vv71577+977+977+9Yu+/vVjvv73vv73Hp3bvv71e77+9Kd66
ey5u77+9K++/ve+/ve+/ve+/vXvvv73vv73vv70i77+977+9Xm7vv71y77+977+977+9eu+/vRrv
v73vv71o77+977+977+977+9Ju+/ve+/vR7vv71H77+977+977+9aO+/vQMo77+96ZqO77+93aJq
Iu+/ve+/vRrvv70bbe+/ve+/ve+/ve+/ve+/vXrvv73elu+/ve+/ve+/vWbvv73vv73vv71o77+9
77+977+9fu+/vW3vv70=


2011-04-07 07:22:41

by Sean Finney

[permalink] [raw]
Subject: [PATCH v2 1/2] nfs-utils: mountd: Use a dynamic buffer for storing lists of gid's

Previously, in auth_unix_gid, group lists were stored in an array of
hard-coded length 100, and in the situation that the group lists for a
particular call were too large, the array was swapped with a dynamically
allocated/freed buffer. For environments where users are commonly in
a large number of groups, this isn't an ideal approach.

Instead, make the group list static scoped to the function, and
use malloc/realloc to grow it on an as-needed basis.

Signed-off-by: Sean Finney <[email protected]>
---
utils/mountd/cache.c | 29 ++++++++++++++++++++---------
1 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index 9bbbfb3..7deb050 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -37,6 +37,7 @@
#include "blkid/blkid.h"
#endif

+#define INITIAL_MANAGED_GROUPS 100

enum nfsd_fsid {
FSID_DEV = 0,
@@ -127,11 +128,21 @@ void auth_unix_gid(FILE *f)
*/
int uid;
struct passwd *pw;
- gid_t glist[100], *groups = glist;
- int ngroups = 100;
+ static gid_t *groups = NULL;
+ static int groups_len = 0;
+ gid_t *more_groups;
+ int ngroups = 0;
int rv, i;
char *cp;

+ if (groups_len == 0) {
+ groups = malloc(sizeof(gid_t)*INITIAL_MANAGED_GROUPS);
+ if (!groups)
+ return;
+
+ groups_len = ngroups = INITIAL_MANAGED_GROUPS;
+ }
+
if (readline(fileno(f), &lbuf, &lbuflen) != 1)
return;

@@ -144,13 +155,16 @@ void auth_unix_gid(FILE *f)
rv = -1;
else {
rv = getgrouplist(pw->pw_name, pw->pw_gid, groups, &ngroups);
- if (rv == -1 && ngroups >= 100) {
- groups = malloc(sizeof(gid_t)*ngroups);
- if (!groups)
+ if (rv == -1 && ngroups >= groups_len) {
+ more_groups = realloc(groups, sizeof(gid_t)*ngroups);
+ if (!more_groups)
rv = -1;
- else
+ else {
+ groups = more_groups;
+ groups_len = ngroups;
rv = getgrouplist(pw->pw_name, pw->pw_gid,
groups, &ngroups);
+ }
}
}
qword_printint(f, uid);
@@ -162,9 +176,6 @@ void auth_unix_gid(FILE *f)
} else
qword_printint(f, 0);
qword_eol(f);
-
- if (groups != glist)
- free(groups);
}

#if USE_BLKID
--
1.7.4.1


2011-04-07 07:22:46

by Sean Finney

[permalink] [raw]
Subject: [PATCH v2 2/2] nfs-utils: Increase the stdio file buffer size for procfs files

Previously, when writing to /proc/net/rpc/*/channel, if a cache line
were larger than the default buffer size (likely 1024 bytes), mountd
and svcgssd would split writes into a number of buffer-sized writes.
Each of these writes would get an EINVAL error back from the kernel
procfs handle (it expects line-oriented input and does not account for
multiple/split writes), and no cache update would occur.

When such behavior occurs, NFS clients depending on mountd to finish
the cache operation would block/hang, or receive EPERM, depending on
the context of the operation. This is likely to happen if a user is a
member of a large (~100-200) number of groups.

Instead, every fopen() on the procfs files in question is followed by
a call to setvbuf(), using a per-file dedicated buffer of
RPC_CHAN_BUF_SIZE length.

Really, mountd should not be using stdio-style buffered file operations
on files in /proc to begin with. A better solution would be to use
internally managed buffers and calls to write() instead of these stdio
calls, but that would be a more extensive change; so this is proposed
as a quick and not-so-dirty fix in the meantime.

Signed-off-by: Sean Finney <[email protected]>
---
support/include/misc.h | 3 +++
utils/gssd/svcgssd_proc.c | 3 +++
utils/mountd/cache.c | 2 ++
3 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/support/include/misc.h b/support/include/misc.h
index 9a1b25d..7e3874e 100644
--- a/support/include/misc.h
+++ b/support/include/misc.h
@@ -24,4 +24,7 @@ struct hostent *get_reliable_hostbyaddr(const char *addr, int len, int type);

extern int is_mountpoint(char *path);

+/* size of the file pointer buffers for rpc procfs files */
+#define RPC_CHAN_BUF_SIZE 32768
+
#endif /* MISC_H */
diff --git a/utils/gssd/svcgssd_proc.c b/utils/gssd/svcgssd_proc.c
index 6f2ba61..8f6548e 100644
--- a/utils/gssd/svcgssd_proc.c
+++ b/utils/gssd/svcgssd_proc.c
@@ -56,6 +56,7 @@
#include "gss_util.h"
#include "err_util.h"
#include "context.h"
+#include "misc.h"

extern char * mech2file(gss_OID mech);
#define SVCGSSD_CONTEXT_CHANNEL "/proc/net/rpc/auth.rpcsec.context/channel"
@@ -78,6 +79,7 @@ do_svc_downcall(gss_buffer_desc *out_handle, struct svc_cred *cred,
FILE *f;
int i;
char *fname = NULL;
+ char vbuf[RPC_CHAN_BUF_SIZE];
int err;

printerr(1, "doing downcall\n");
@@ -90,6 +92,7 @@ do_svc_downcall(gss_buffer_desc *out_handle, struct svc_cred *cred,
SVCGSSD_CONTEXT_CHANNEL, strerror(errno));
goto out_err;
}
+ setvbuf(f, vbuf, _IOLBF, RPC_CHAN_BUF_SIZE);
qword_printhex(f, out_handle->value, out_handle->length);
/* XXX are types OK for the rest of this? */
/* For context cache, use the actual context endtime */
diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index 7deb050..4eded17 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -739,6 +739,7 @@ struct {
char *cache_name;
void (*cache_handle)(FILE *f);
FILE *f;
+ char vbuf[RPC_CHAN_BUF_SIZE];
} cachelist[] = {
{ "auth.unix.ip", auth_unix_ip},
{ "auth.unix.gid", auth_unix_gid},
@@ -757,6 +758,7 @@ void cache_open(void)
continue;
sprintf(path, "/proc/net/rpc/%s/channel", cachelist[i].cache_name);
cachelist[i].f = fopen(path, "r+");
+ setvbuf(cachelist[i].f, cachelist[i].vbuf, _IOLBF, RPC_CHAN_BUF_SIZE);
}
}

--
1.7.4.1


2011-04-07 12:22:22

by Jim Rees

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] nfs-utils: mountd: Use a dynamic buffer for storing lists of gid's

Sean Finney wrote:

Previously, in auth_unix_gid, group lists were stored in an array of
hard-coded length 100, and in the situation that the group lists for a
particular call were too large, the array was swapped with a dynamically
allocated/freed buffer. For environments where users are commonly in
a large number of groups, this isn't an ideal approach.

Instead, make the group list static scoped to the function, and
use malloc/realloc to grow it on an as-needed basis.

I would re-word this. The list isn't static, the list pointer is. I don't
think you need to mention that, it's just confusing. "Use malloc/realloc to
grow the list on an as-needed basis."

+ groups = malloc(sizeof(gid_t)*INITIAL_MANAGED_GROUPS);

Style nit: Put blanks around the "*".

I was going to suggest you first call getgrouplist with groups set to 0 so
you can always alloc() the correct size list, but then I found this in the
man page:

BUGS
In glibc versions before 2.3.3, the implementation of this function
contains a buffer-overrun bug: it returns the complete list of groups
for user in the array groups, even when the number of groups exceeds
*ngroups.

2011-04-07 05:53:33

by Sean Finney

[permalink] [raw]
Subject: Re: Fixes for NFS in environments with large group memberships

SGkgQnJ1Y2UsCgpPbiBUdWUsIDIwMTEtMDQtMDUgYXQgMTk6MzUgLTA0MDAsIEouIEJydWNlIEZp
ZWxkcyB3cm90ZToKCj4gPiAKPiA+IFtQQVRDSF0gVXNlIGEgbmFtZWQgY29uc3RhbnQgZm9yIG1h
eCBudW1iZXIgb2YgbWFuYWdlZCBncm91cHMsIGFuZCBpbmMKPiA+IFtQQVRDSF0gSW5jcmVhc2Ug
dGhlIGRlZmF1bHQgYnVmZmVyIHNpemUgZm9yIGNhY2hlbGlzdCBjaGFubmVsIGZpbGVzLgo+IAo+
IEkgY2FuJ3QgZmluZCB0aG9zZTsgY291bGQgeW91IHJlc2VuZD8KClN1cmUgdGhpbmcuICBPbiBh
IHNlY29uZCByZXZpZXcsIGl0IHNlZW1zIHRoYXQgdGhlICJuYW1lZCBjb25zdGFudCIKcGF0Y2gg
bWF5IG5vdCBiZSBzdHJpY3RseSBuZWNlc3NhcnksIHRob3VnaCBidW1waW5nIHRoZSB2YWx1ZSBh
dm9pZHMKbmVlZGluZyB0byBkbyBhIG1hbGxvYyBldmVyeSBvcGVyYXRpb24uICBJIHRoaW5rIEkn
bGwgdGFrZSBhIHF1aWNrIHNob3QKYXQgcmUtc3Bpbm5pbmcgdGhhdCBwYXRjaCB0byB1c2UgYSBk
eW5hbWljYWxseSBncm93aW5nIHN0YXRpYyBidWZmZXIKaW5zdGVhZFsxXSwgYW5kIHJlLXNlbmQg
dGhlIHBhdGNoIGFmdGVyIHRoYXQuCgoJU2VhbgoKWzFdIEkgd2FzIGhlc2l0YW50IHRvIGRvIHRo
aXMgYXQgZmlyc3QgZHVlIHRvIGFsbCB0aGUgcmVmZXJlbmNlcyB0bwoidGhyZWFkcyIsIGJ1dCBm
cm9tIHdoYXQgSSBjYW4gdGVsbCAoYSkgbmZzLXV0aWxzIGRvZXNuJ3QgdXNlIHRocmVhZHMKYW5k
IGluc3RlYWQgZm9yaygpLCBhbmQgKGIpIHRoZSBzYW1lIHRlY2huaXF1ZSBpcyBhbHJlYWR5IHVz
ZWQgaW4gYXQKbGVhc3Qgb25lIG90aGVyIHBsYWNlIGluIHRoZSBjb2RlLgoT77+977+97Lm7HO+/
vSbvv71+77+9Ju+/vRjvv73vv70rLe+/ve+/vd22F++/ve+/vXfvv73vv73Lm++/ve+/ve+/vW3v
v71i77+977+9Z37Ip++/vRfvv73vv73cqH3vv73vv73vv73GoHrvv70majordu+/ve+/ve+/vQfv
v73vv73vv73vv716Wivvv73vv70rembvv73vv73vv71o77+977+977+9fu+/ve+/ve+/ve+/vWnv
v73vv73vv71677+9Hu+/vXfvv73vv73vv70/77+977+977+977+9Ju+/vSnfohtm

2011-04-07 14:29:54

by Chuck Lever

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] nfs-utils: mountd: Use a dynamic buffer for storing lists of gid's


On Apr 7, 2011, at 10:15 AM, Finney, Sean wrote:

> Hi Jim,
>
> On Thu, 2011-04-07 at 08:22 -0400, Jim Rees wrote:
>
>> I was going to suggest you first call getgrouplist with groups set to 0 so
>> you can always alloc() the correct size list, but then I found this in the
>> man page:
>>
>> BUGS
>> In glibc versions before 2.3.3, the implementation of this function
>> contains a buffer-overrun bug: it returns the complete list of groups
>> for user in the array groups, even when the number of groups exceeds
>> *ngroups.
>
> Yuck. The function is also not in POSIX or any other standards, so
> maybe it's worth discussing at a later point whether getgroups(2) would
> be a better interface to use, even if it means an extra step or two.

The standard RPC library function authunix_create_default() uses getgroups(2). Take a look at the glibc version of this function to see the preferred method for using getgroups(2).

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com





2011-04-07 14:15:40

by Sean Finney

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] nfs-utils: mountd: Use a dynamic buffer for storing lists of gid's

SGkgSmltLAoKT24gVGh1LCAyMDExLTA0LTA3IGF0IDA4OjIyIC0wNDAwLCBKaW0gUmVlcyB3cm90
ZToKPiBTZWFuIEZpbm5leSB3cm90ZToKPiAKPiAgIFByZXZpb3VzbHksIGluIGF1dGhfdW5peF9n
aWQsIGdyb3VwIGxpc3RzIHdlcmUgc3RvcmVkIGluIGFuIGFycmF5IG9mCj4gICBoYXJkLWNvZGVk
IGxlbmd0aCAxMDAsIGFuZCBpbiB0aGUgc2l0dWF0aW9uIHRoYXQgdGhlIGdyb3VwIGxpc3RzIGZv
ciBhCj4gICBwYXJ0aWN1bGFyIGNhbGwgd2VyZSB0b28gbGFyZ2UsIHRoZSBhcnJheSB3YXMgc3dh
cHBlZCB3aXRoIGEgZHluYW1pY2FsbHkKPiAgIGFsbG9jYXRlZC9mcmVlZCBidWZmZXIuICBGb3Ig
ZW52aXJvbm1lbnRzIHdoZXJlIHVzZXJzIGFyZSBjb21tb25seSBpbgo+ICAgYSBsYXJnZSBudW1i
ZXIgb2YgZ3JvdXBzLCB0aGlzIGlzbid0IGFuIGlkZWFsIGFwcHJvYWNoLgo+ICAgCj4gICBJbnN0
ZWFkLCBtYWtlIHRoZSBncm91cCBsaXN0IHN0YXRpYyBzY29wZWQgdG8gdGhlIGZ1bmN0aW9uLCBh
bmQKPiAgIHVzZSBtYWxsb2MvcmVhbGxvYyB0byBncm93IGl0IG9uIGFuIGFzLW5lZWRlZCBiYXNp
cy4KPiAKPiBJIHdvdWxkIHJlLXdvcmQgdGhpcy4gIFRoZSBsaXN0IGlzbid0IHN0YXRpYywgdGhl
IGxpc3QgcG9pbnRlciBpcy4gIEkgZG9uJ3QKPiB0aGluayB5b3UgbmVlZCB0byBtZW50aW9uIHRo
YXQsIGl0J3MganVzdCBjb25mdXNpbmcuICAiVXNlIG1hbGxvYy9yZWFsbG9jIHRvCj4gZ3JvdyB0
aGUgbGlzdCBvbiBhbiBhcy1uZWVkZWQgYmFzaXMuIgoKT2theS4gIEkgd2FzIHRyeWluZyB0byBq
dXN0IHB1dCBhIGhpbnQgdGhhdCB0aGVyZSB3YXMgYSBwZXJzaXN0YW50CmJ1ZmZlciB0aGF0IHdv
dWxkIGhhbmcgYXJvdW5kIHRocm91Z2ggbXVsdGlwbGUgaW52b2NhdGlvbnMsIHRob3VnaApwZXJo
YXBzIHRoYXQncyBhIGxpdHRsZSB0b28gbXVjaCAnaW1wbGVtZW50YXRpb24gZGV0YWlscycuICBU
b3RhbGx5IGZpbmUKd2l0aCBtb2RpZnlpbmcgdGhlIGRlc2NyaXB0aW9uIGVpdGhlciB3YXkgOikK
Cgo+IAo+ICAgKwkJZ3JvdXBzID0gbWFsbG9jKHNpemVvZihnaWRfdCkqSU5JVElBTF9NQU5BR0VE
X0dST1VQUyk7Cj4gCj4gU3R5bGUgbml0OiAgUHV0IGJsYW5rcyBhcm91bmQgdGhlICIqIi4KCldp
bGwgZG8uCgo+IEkgd2FzIGdvaW5nIHRvIHN1Z2dlc3QgeW91IGZpcnN0IGNhbGwgZ2V0Z3JvdXBs
aXN0IHdpdGggZ3JvdXBzIHNldCB0byAwIHNvCj4geW91IGNhbiBhbHdheXMgYWxsb2MoKSB0aGUg
Y29ycmVjdCBzaXplIGxpc3QsIGJ1dCB0aGVuIEkgZm91bmQgdGhpcyBpbiB0aGUKPiBtYW4gcGFn
ZToKPiAKPiBCVUdTCj4gICAgICAgIEluICBnbGliYyAgdmVyc2lvbnMgIGJlZm9yZSAgMi4zLjMs
IHRoZSBpbXBsZW1lbnRhdGlvbiBvZiB0aGlzIGZ1bmN0aW9uCj4gICAgICAgIGNvbnRhaW5zIGEg
YnVmZmVyLW92ZXJydW4gYnVnOiBpdCByZXR1cm5zIHRoZSBjb21wbGV0ZSBsaXN0ICBvZiAgZ3Jv
dXBzCj4gICAgICAgIGZvciAgdXNlciAgaW4gIHRoZSBhcnJheSBncm91cHMsIGV2ZW4gd2hlbiB0
aGUgbnVtYmVyIG9mIGdyb3VwcyBleGNlZWRzCj4gICAgICAgICpuZ3JvdXBzLgoKWXVjay4gIFRo
ZSBmdW5jdGlvbiBpcyBhbHNvIG5vdCBpbiBQT1NJWCBvciBhbnkgb3RoZXIgc3RhbmRhcmRzLCBz
bwptYXliZSBpdCdzIHdvcnRoIGRpc2N1c3NpbmcgYXQgYSBsYXRlciBwb2ludCB3aGV0aGVyIGdl
dGdyb3VwcygyKSB3b3VsZApiZSBhIGJldHRlciBpbnRlcmZhY2UgdG8gdXNlLCBldmVuIGlmIGl0
IG1lYW5zIGFuIGV4dHJhIHN0ZXAgb3IgdHdvLgoKCkFueXdheSwgSSB3aWxsIHJlLXNwaW4gdGhp
cyBwYXRjaCB3aXRoIHRoZSB0d28gY2hhbmdlcyB0aGF0IHlvdSd2ZQpzdWdnZXN0ZWQgYW5kIHN1
Ym1pdCBpdCB0b21vcnJvdy4gICBDYXJlIHRvIHRha2UgYSBsb29rIGF0IHBhdGNoCjIvMj8gIDop
CgoKCVNlYW4KTu+/ve+/ve+/ve+/ve+/vXLvv73vv71577+977+977+9Yu+/vVjvv73vv73Hp3bv
v71e77+9Kd66ey5u77+9K++/ve+/ve+/ve+/vXvvv73vv73vv70i77+977+9Xm7vv71y77+977+9
77+9eu+/vRrvv73vv71o77+977+977+977+9Ju+/ve+/vR7vv71H77+977+977+9aO+/vQMo77+9
6ZqO77+93aJqIu+/ve+/vRrvv70bbe+/ve+/ve+/ve+/ve+/vXrvv73elu+/ve+/ve+/vWbvv73v
v73vv71o77+977+977+9fu+/vW3vv70=

2011-04-07 15:01:23

by Jim Rees

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] nfs-utils: mountd: Use a dynamic buffer for storing lists of gid's

Finney, Sean wrote:

> I would re-word this. The list isn't static, the list pointer is. I don't
> think you need to mention that, it's just confusing. "Use malloc/realloc to
> grow the list on an as-needed basis."

Okay. I was trying to just put a hint that there was a persistant
buffer that would hang around through multiple invocations, though
perhaps that's a little too much 'implementation details'. Totally fine
with modifying the description either way :)

If it were me, I would alloc and free the list each time rather than keeping
it around. It's not like malloc is super expensive. But that's a matter of
taste.

> BUGS
> In glibc versions before 2.3.3, the implementation of this function
> contains a buffer-overrun bug: it returns the complete list of groups
> for user in the array groups, even when the number of groups exceeds
> *ngroups.

Yuck. The function is also not in POSIX or any other standards, so
maybe it's worth discussing at a later point whether getgroups(2) would
be a better interface to use, even if it means an extra step or two.

There's no sense in making nfs-utils portable to non-linux, so depending on
glibc is probably ok. 2.3.3 was released in December 2003 so I would argue
we don't care about the buffer-overrun bug.

Anyway, I will re-spin this patch with the two changes that you've
suggested and submit it tomorrow. Care to take a look at patch
2/2? :)

It looked fine to me.

2011-04-05 23:35:52

by J. Bruce Fields

[permalink] [raw]
Subject: Re: Fixes for NFS in environments with large group memberships

On Mon, Apr 04, 2011 at 10:02:59AM +0200, Finney, Sean wrote:
> Hi There!
>=20
> Last week I fired off a couple patches to this list and in hindsight =
I
> realize that I might not have been totally clear for the motivation
> behind the patches:
>=20
> [PATCH] Use a named constant for max number of managed groups, and in=
c
> [PATCH] Increase the default buffer size for cachelist channel files.

I can't find those; could you resend?

--b.

>=20
> Since nobody has reviewed them yet, I thought I'd make it clear that
> these changes (or something along such lines) are required for NFS
> access to mountpoints when users are in largeish (>100) numbers of
> groups. There are actually two problems:
>=20
> * Hard-coded limit for group membership at 100 groups in mountd sourc=
e
> * Large server-side procfs writes are split into mulitple writes due =
to
> stdio buffering in mountd and svcgssd sources, resulting in client
> hangs.
>=20
> The patches in question solve this in as non-intrusive a manner as
> possible, though I'm of course open to changing things up if you feel=
it
> should be done otherwise. So please let me know what you think, or i=
f
> it would be better for me to first go and open a bug, start a
> discussion, etc.
>=20
>=20
> Thanks,
> Sean=20
> N?????r??y????b?X??=C7=A7v?^?)=DE=BA{.n?+????{???"??^n?r???z?=1A??h??=
???&??=1E?G???h?=03(?=E9=9A=8E?=DD=A2j"??=1A?=1Bm??????z?=DE=96???f???h=
???~?m