Date: Fri, 24 Aug 2012 13:07:14 -0700 From: Alfred Perlstein <alfred@freebsd.org> To: Steven Haber <steven.haber@isilon.com> Cc: snnn119@gmail.com, freebsd-threads@freebsd.org Subject: Re: netconfig is not threadsafe Message-ID: <20120824200714.GM68801@elvis.mu.org> In-Reply-To: <56CE86D6660FF84498426FA7A33170B40413DF8E@seaxch01.desktop.isilon.com> References: <56CE86D6660FF84498426FA7A33170B40413DF8E@seaxch01.desktop.isilon.com>
next in thread | previous in thread | raw e-mail | index | archive | help
I suppose it can be fixed like this and the idea is pretty sound, however I was wondering if you can you look at how Linux/Solaris/OSX handle this? I would think there would be some form of "_r" routines that they would use instead. Or maybe thier libc avoids calling these routines somehow? That said this is good work and should go in barring other implementations that may make more sense. -Alfred * Steven Haber <steven.haber@isilon.com> [120824 13:00] wrote: > netconfig (libc routines for reading the netconfig file) is currently > not threadsafe. It is a transactional API set, that is, you call > setnetconfig() to open the file, read from the file with getnetconfig(), > and close/cleanup with endnetconfig(). The problem is that the order > that these routines are called in is important, but the state of the > current transaction is stored in a per-process static, so only a single > thread can do a transaction at one time. A patch from five years ago > attempted to fix this: > > > > http://lists.freebsd.org/mailman/confirm/freebsd-threads/292f06b38808587 > 41c9034fb4173a14740c63a9e > > > > This patch does not solve the problem. It adds protection for the > statics so that concurrent calls into set/get/endnetgrent() don't trash > the statics. However, there is no use case where you would want two > threads accessing any of these statics concurrently. To solve this > problem, I have stored the FILE and netconfig_info statics on a > per-thread basis using the pthreads key setup that was already in place > for the error code. Now threads have their own set of stored state data, > so no statics are necessary. I removed the original locking that had no > effect. > > > > My repro is to spawn 32 threads that all create an RPC client > simultaneously using libc's clnt_create(), which calls into netconfig. > After patching, I no longer see the issue. Here is the stack when a > thread collision occurs (same as in the original bug from 2007): > > > > #0 fclose (fp=0x0) at > /root/Source/onefs/git/src/lib/libc/stdio/fclose.c:52 > > #1 0x0000000802c83a79 in endnetconfig (handlep=0x80b1030a0) at > /root/Source/onefs/git/src/lib/libc/rpc/getnetconfig.c:433 > > #2 0x0000000802c811d0 in __rpc_endconf (vhandle=0x80b103090) at > /root/Source/onefs/git/src/lib/libc/rpc/rpc_generic.c:445 > > #3 0x0000000802c7331a in clnt_create_timed (hostname=0x804d7cd9b > "tgroup %s", prog=134420864, vers=1, netclass=Variable "netclass" is not > available. > > ) at /root/Source/onefs/git/src/lib/libc/rpc/clnt_generic.c:271 > > > > This issue is present in FreeBSD head, though I'm reproducing on 7.1. > > > > Does the patch make sense? Should I open a bug on this somehow? I'm > relatively new to contributing to FreeBSD. > > > > Thanks, > > Steven > > > > > > >From f3639afbe7993f3c699f23f4d77749ea01e8e8c8 Mon Sep 17 00:00:00 2001 > > From: Steven Haber <shaber@isilon.com> > > Date: Thu, 23 Aug 2012 17:45:51 -0700 > > Subject: [PATCH] Fix for bug 95757. Make netconfig threadsafe so we can > make > > lots of RPC clients concurrently. The original locking had > > no effect; there is no point in protecting the static > > members since you should never have two threads in the same > > get/set/endnetconfig. The point of these functions is to be > > called in sequence as a transaction. You can't have > > multiple threads calling them if the state is stored as a > > process global. This fix moves the static vars to thread- > > specific storage so that each thread can have its own > > transaction, and removes the exiting locking. > > --- > > src/lib/libc/rpc/getnetconfig.c | 74 > +++++++++++---------------------------- > > 1 file changed, 21 insertions(+), 53 deletions(-) > > > > diff --git a/src/lib/libc/rpc/getnetconfig.c > b/src/lib/libc/rpc/getnetconfig.c > > index 0f7168f..2957d14 100644 > > --- a/src/lib/libc/rpc/getnetconfig.c > > +++ b/src/lib/libc/rpc/getnetconfig.c > > @@ -119,22 +119,21 @@ struct netconfig_vars { > > struct netconfig_list *nc_configs; /* pointer to the current > netconfig entry */ > > }; > > +struct nc_thread_vars { > > + int nc_error; > > + FILE *nc_file; > > + struct netconfig_info nc_ni; > > +}; > > + > > #define NC_VALID 0xfeed > > #define NC_STORAGE 0xf00d > > #define NC_INVALID 0 > > > > -static int *__nc_error(void); > > static int parse_ncp(char *, struct netconfig *); > > static struct netconfig *dup_ncp(struct netconfig *); > > > > -static FILE *nc_file; /* for netconfig db */ > > -static mutex_t nc_file_lock = MUTEX_INITIALIZER; > > - > > -static struct netconfig_info ni = { 0, 0, NULL, NULL}; > > -static mutex_t ni_lock = MUTEX_INITIALIZER; > > - > > static thread_key_t nc_key; > > static once_t nc_once = ONCE_INITIALIZER; > > static int nc_key_error; > > @@ -148,34 +147,37 @@ nc_key_init(void) > > #define MAXNETCONFIGLINE 1000 > > -static int * > > -__nc_error() > > +static struct nc_thread_vars * > > +__nc_thread() > > { > > - static int nc_error = 0; > > - int *nc_addr; > > + static struct nc_thread_vars nc_vars = {0, NULL, {0, 0, > NULL, NULL}}; > > + struct nc_thread_vars *nc_addr; > > /* > > - * Use the static `nc_error' if we are the main thread > > + * Use the static `nc_vars' if we are the main thread > > * (including non-threaded programs), or if an allocation > > * fails. > > */ > > if (thr_main()) > > - return (&nc_error); > > + return (&nc_vars); > > if (thr_once(&nc_once, nc_key_init) != 0 || nc_key_error > != 0) > > - return (&nc_error); > > - if ((nc_addr = (int *)thr_getspecific(nc_key)) == NULL) { > > - nc_addr = (int *)malloc(sizeof (int)); > > + return (&nc_vars); > > + if ((nc_addr = (struct nc_thread_vars > *)thr_getspecific(nc_key)) > > + == NULL) { > > + nc_addr = (struct nc_thread_vars *) > > + calloc(1, sizeof(struct > nc_thread_vars)); > > if (thr_setspecific(nc_key, (void *) > nc_addr) != 0) { > > if (nc_addr) > > > free(nc_addr); > > - return (&nc_error); > > + return (&nc_vars); > > } > > - *nc_addr = 0; > > } > > return (nc_addr); > > } > > -#define nc_error (*(__nc_error())) > > +#define nc_error (*(&(__nc_thread()->nc_error))) > > +#define nc_file (*(&(__nc_thread()->nc_file))) > > +#define ni (*(&(__nc_thread()->nc_ni))) > > /* > > * A call to setnetconfig() establishes a /etc/netconfig "session". A > session > > * "handle" is returned on a successful call. At the start of a > session (after > > @@ -209,23 +211,14 @@ setnetconfig() > > * For multiple calls, i.e. nc_file is not NULL, we just return the > > * handle without reopening the netconfig db. > > */ > > - mutex_lock(&ni_lock); > > ni.ref++; > > - mutex_unlock(&ni_lock); > > - > > - mutex_lock(&nc_file_lock); > > if ((nc_file != NULL) || (nc_file = fopen(NETCONFIG, "r")) != NULL) > { > > nc_vars->valid = NC_VALID; > > nc_vars->flag = 0; > > nc_vars->nc_configs = ni.head; > > - mutex_unlock(&nc_file_lock); > > return ((void *)nc_vars); > > } > > - mutex_unlock(&nc_file_lock); > > - > > - mutex_lock(&ni_lock); > > ni.ref--; > > - mutex_unlock(&ni_lock); > > nc_error = NC_NONETCONFIG; > > free(nc_vars); > > @@ -254,13 +247,10 @@ void *handlep; > > /* > > * Verify that handle is valid > > */ > > - mutex_lock(&nc_file_lock); > > if (ncp == NULL || nc_file == NULL) { > > nc_error = NC_NOTINIT; > > - mutex_unlock(&nc_file_lock); > > return (NULL); > > } > > - mutex_unlock(&nc_file_lock); > > switch (ncp->valid) { > > case NC_VALID: > > @@ -274,9 +264,7 @@ void *handlep; > > */ > > if (ncp->flag == 0) { /* first time */ > > ncp->flag = 1; > > - mutex_lock(&ni_lock); > > ncp->nc_configs = ni.head; > > - mutex_unlock(&ni_lock); > > if (ncp->nc_configs != NULL) /* entry already exist > */ > > return(ncp->nc_configs->ncp); > > } > > @@ -289,12 +277,9 @@ void *handlep; > > * If we cannot find the entry in the list and is end of > file, > > * we give up. > > */ > > - mutex_lock(&ni_lock); > > if (ni.eof == 1) { > > - mutex_unlock(&ni_lock); > > return(NULL); > > } > > - mutex_unlock(&ni_lock); > > break; > > default: > > @@ -316,18 +301,13 @@ void *handlep; > > /* > > * Read a line from netconfig file. > > */ > > - mutex_lock(&nc_file_lock); > > do { > > if (fgets(stringp, MAXNETCONFIGLINE, nc_file) == NULL) { > > free(stringp); > > - mutex_lock(&ni_lock); > > ni.eof = 1; > > - mutex_unlock(&ni_lock); > > - mutex_unlock(&nc_file_lock); > > return (NULL); > > } > > } while (*stringp == '#'); > > - mutex_unlock(&nc_file_lock); > > list = (struct netconfig_list *) malloc(sizeof (struct > netconfig_list)); > > if (list == NULL) { > > @@ -357,7 +337,6 @@ void *handlep; > > * Reposition the current pointer of the handle to the > last entry > > * in the list. > > */ > > - mutex_lock(&ni_lock); > > if (ni.head == NULL) { /* first entry */ > > ni.head = ni.tail = list; > > } > > @@ -367,7 +346,6 @@ void *handlep; > > } > > ncp->nc_configs = ni.tail; > > result = ni.tail->ncp; > > - mutex_unlock(&ni_lock); > > return(result); > > } > > } > > @@ -402,9 +380,7 @@ void *handlep; > > nc_handlep->valid = NC_INVALID; > > nc_handlep->flag = 0; > > nc_handlep->nc_configs = NULL; > > - mutex_lock(&ni_lock); > > if (--ni.ref > 0) { > > - mutex_unlock(&ni_lock); > > free(nc_handlep); > > return(0); > > } > > @@ -417,7 +393,6 @@ void *handlep; > > ni.eof = ni.ref = 0; > > ni.head = NULL; > > ni.tail = NULL; > > - mutex_unlock(&ni_lock); > > while (q != NULL) { > > p = q->next; > > @@ -429,10 +404,8 @@ void *handlep; > > } > > free(nc_handlep); > > - mutex_lock(&nc_file_lock); > > fclose(nc_file); > > nc_file = NULL; > > - mutex_unlock(&nc_file_lock); > > return (0); > > } > > @@ -481,21 +454,16 @@ getnetconfigent(netid) > > * If all the netconfig db has been read and placed into the list > and > > * there is no match for the netid, return NULL. > > */ > > - mutex_lock(&ni_lock); > > if (ni.head != NULL) { > > for (list = ni.head; list; list = list->next) { > > if (strcmp(list->ncp->nc_netid, netid) == 0) { > > - mutex_unlock(&ni_lock); > > return(dup_ncp(list->ncp)); > > } > > } > > if (ni.eof == 1) { /* that's all the entries */ > > - mutex_unlock(&ni_lock); > > return(NULL); > > } > > } > > - mutex_unlock(&ni_lock); > > - > > if ((file = fopen(NETCONFIG, "r")) == NULL) { > > nc_error = NC_NONETCONFIG; > > -- > > 1.7.9.5 > > > > _______________________________________________ > freebsd-threads@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-threads > To unsubscribe, send any mail to "freebsd-threads-unsubscribe@freebsd.org" -- - Alfred Perlstein .- VMOA #5191, 03 vmax, 92 gs500, 85 ch250, 07 zx10 .- FreeBSD committer
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120824200714.GM68801>