[PATCH v2] nfs-utils: mountd: Use a dynamic buffer for storing lists of gid's

[PATCH v2] nfs-utils: mountd: Use a dynamic buffer for storing lists of gid's

[Sean Finney]

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, use malloc/realloc to grow the list on an as-needed basis.

Signed-off-by: Sean Finney <sean.finney@sonyericsson.com>
---
 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..34808cd 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

[PATCH v2] nfs-utils: Increase the stdio file buffer size for procfs files

[Sean Finney]

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 <sean.finney@sonyericsson.com>
---
 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 34808cd..bd67157 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

Re: [PATCH v2] nfs-utils: Increase the stdio file buffer size for procfs files

[J. Bruce Fields]

Looks sensible to me.--b.

On Fri, Apr 08, 2011 at 01:16:46PM +0200, Sean Finney wrote:
> 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 <sean.finney@sonyericsson.com>
> ---
>  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 34808cd..bd67157 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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] nfs-utils: Increase the stdio file buffer size for procfs files

[Steve Dickson]

On 04/08/2011 07:16 AM, Sean Finney wrote:
> 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 <sean.finney@sonyericsson.com>
The same with this one... Committed!

steved.

Re: [PATCH v2] nfs-utils: mountd: Use a dynamic buffer for storing lists of gid's

[Steve Dickson]

On 04/08/2011 07:16 AM, 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, use malloc/realloc to grow the list on an as-needed basis.
> 
> Signed-off-by: Sean Finney <sean.finney@sonyericsson.com>
My bad... I committed this early this week and forgot to 
acknowledge on the list... Sorry about that... 

steved.