From owner-freebsd-threads@FreeBSD.ORG Fri Aug 24 20:17:46 2012 Return-Path: Delivered-To: freebsd-threads@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 28E48106564A for ; Fri, 24 Aug 2012 20:17:46 +0000 (UTC) (envelope-from bright@elvis.mu.org) Received: from elvis.mu.org (elvis.mu.org [192.203.228.196]) by mx1.freebsd.org (Postfix) with ESMTP id 10B088FC17 for ; Fri, 24 Aug 2012 20:17:45 +0000 (UTC) Received: by elvis.mu.org (Postfix, from userid 1192) id BDB111A3D6C; Fri, 24 Aug 2012 13:17:45 -0700 (PDT) Date: Fri, 24 Aug 2012 13:17:45 -0700 From: Alfred Perlstein To: Steven Haber Message-ID: <20120824201745.GO68801@elvis.mu.org> References: <56CE86D6660FF84498426FA7A33170B40413DF8E@seaxch01.desktop.isilon.com> <20120824200714.GM68801@elvis.mu.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20120824200714.GM68801@elvis.mu.org> User-Agent: Mutt/1.4.2.3i Cc: snnn119@gmail.com, freebsd-threads@freebsd.org Subject: Re: netconfig is not threadsafe X-BeenThere: freebsd-threads@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Threading on FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Aug 2012 20:17:46 -0000 I did a bit of research and this manual page: http://www.unix.com/man-page/All/3n/setnetconfig/ seems to indicate that this is the best that is "expected" for thread safety: MULTITHREAD USAGE Thread Safe: Yes Cancel Safe: Yes Fork Safe: No Async-cancel Safe: No Async-signal Safe: No These functions can be called safely in a multithreaded environment. They may be cancellation points in that they call functions that are cancel points. In a multithreaded environment, these functions are not safe to be called by a child process after and before These functions should not be called by a multithreaded application that supports asynchronous cancellation or asynchronous signals. So this is awesome. Thank you. -Alfred * Alfred Perlstein [120824 13:14] wrote: > 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 [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 > > > > 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 > _______________________________________________ > 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