Date: Fri, 24 Aug 2012 12:58:45 -0700 From: Steven Haber <steven.haber@isilon.com> To: <freebsd-threads@freebsd.org>, <snnn119@gmail.com> Subject: netconfig is not threadsafe Message-ID: <56CE86D6660FF84498426FA7A33170B40413DF8E@seaxch01.desktop.isilon.com>
next in thread | raw e-mail | index | archive | help
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: =20 http://lists.freebsd.org/mailman/confirm/freebsd-threads/292f06b38808587 41c9034fb4173a14740c63a9e =20 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. =20 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): =20 #0 fclose (fp=3D0x0) at /root/Source/onefs/git/src/lib/libc/stdio/fclose.c:52 #1 0x0000000802c83a79 in endnetconfig (handlep=3D0x80b1030a0) at /root/Source/onefs/git/src/lib/libc/rpc/getnetconfig.c:433 #2 0x0000000802c811d0 in __rpc_endconf (vhandle=3D0x80b103090) at /root/Source/onefs/git/src/lib/libc/rpc/rpc_generic.c:445 #3 0x0000000802c7331a in clnt_create_timed (hostname=3D0x804d7cd9b "tgroup %s", prog=3D134420864, vers=3D1, netclass=3DVariable "netclass" = is not available. ) at /root/Source/onefs/git/src/lib/libc/rpc/clnt_generic.c:271 =20 This issue is present in FreeBSD head, though I'm reproducing on 7.1. =20 Does the patch make sense? Should I open a bug on this somehow? I'm relatively new to contributing to FreeBSD. =20 Thanks, Steven =20 =20 >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(-) =20 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 =20 -static int *__nc_error(void); static int parse_ncp(char *, struct netconfig *); static struct netconfig *dup_ncp(struct netconfig *); =20 -static FILE *nc_file; /* for netconfig db */ -static mutex_t nc_file_lock =3D MUTEX_INITIALIZER; - -static struct netconfig_info ni =3D { 0, 0, NULL, NULL}; -static mutex_t ni_lock =3D MUTEX_INITIALIZER; - static thread_key_t nc_key; static once_t nc_once =3D 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 =3D 0; - int *nc_addr; + static struct nc_thread_vars nc_vars =3D {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) !=3D 0 || = nc_key_error !=3D 0) - return (&nc_error); - if ((nc_addr =3D (int *)thr_getspecific(nc_key)) =3D=3D = NULL) { - nc_addr =3D (int *)malloc(sizeof (int)); + return (&nc_vars); + if ((nc_addr =3D (struct nc_thread_vars *)thr_getspecific(nc_key)) + =3D=3D NULL) { + nc_addr =3D (struct nc_thread_vars *) + calloc(1, sizeof(struct nc_thread_vars)); if (thr_setspecific(nc_key, (void *) nc_addr) !=3D 0) { if (nc_addr) =20 free(nc_addr); - return (&nc_error); + return (&nc_vars); } - *nc_addr =3D 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 !=3D NULL) || (nc_file =3D fopen(NETCONFIG, "r")) !=3D = NULL) { nc_vars->valid =3D NC_VALID; nc_vars->flag =3D 0; nc_vars->nc_configs =3D 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 =3D NC_NONETCONFIG; free(nc_vars); @@ -254,13 +247,10 @@ void *handlep; /* * Verify that handle is valid */ - mutex_lock(&nc_file_lock); if (ncp =3D=3D NULL || nc_file =3D=3D NULL) { nc_error =3D 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 =3D=3D 0) { /* first time */ ncp->flag =3D 1; - mutex_lock(&ni_lock); ncp->nc_configs =3D ni.head; - mutex_unlock(&ni_lock); if (ncp->nc_configs !=3D 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 =3D=3D 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) =3D=3D = NULL) { free(stringp); - mutex_lock(&ni_lock); ni.eof =3D 1; - mutex_unlock(&ni_lock); - mutex_unlock(&nc_file_lock); return (NULL); } } while (*stringp =3D=3D '#'); - mutex_unlock(&nc_file_lock); list =3D (struct netconfig_list *) malloc(sizeof (struct netconfig_list)); if (list =3D=3D 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 =3D=3D NULL) { /* first entry */ ni.head =3D ni.tail =3D list; } @@ -367,7 +346,6 @@ void *handlep; } ncp->nc_configs =3D ni.tail; result =3D ni.tail->ncp; - mutex_unlock(&ni_lock); return(result); } } @@ -402,9 +380,7 @@ void *handlep; nc_handlep->valid =3D NC_INVALID; nc_handlep->flag =3D 0; nc_handlep->nc_configs =3D 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 =3D ni.ref =3D 0; ni.head =3D NULL; ni.tail =3D NULL; - mutex_unlock(&ni_lock); while (q !=3D NULL) { p =3D q->next; @@ -429,10 +404,8 @@ void *handlep; } free(nc_handlep); - mutex_lock(&nc_file_lock); fclose(nc_file); nc_file =3D 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 !=3D NULL) { for (list =3D ni.head; list; list =3D list->next) { if (strcmp(list->ncp->nc_netid, netid) =3D=3D 0) { - mutex_unlock(&ni_lock); return(dup_ncp(list->ncp)); } } if (ni.eof =3D=3D 1) { /* that's all the entries */ - mutex_unlock(&ni_lock); return(NULL); } } - mutex_unlock(&ni_lock); - if ((file =3D fopen(NETCONFIG, "r")) =3D=3D NULL) { nc_error =3D NC_NONETCONFIG; --=20 1.7.9.5 =20
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?56CE86D6660FF84498426FA7A33170B40413DF8E>