This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] #13594: Avoid race in nscd


On Monday, May 14, 2012 23:22:47 Roland McGrath wrote:
> > +/* Try acquiring lock for mapptr, return 1 if not possible.  */
> > +extern int __nscd_acquire_maplock (struct locked_map_ptr *mapptr)
> > attribute_hidden;
> 
> Line too long.  Put attribute_hidden on the next line, indented two
> spaces.
> 
> If its return value is only 0 or 1, make it bool.
> 
> I had in mind an inline function so as not to perturb the performance
> characteristics of the hot case.  Probably that level of
> micro-optimization does not really matter.  (If it does, probably the
> best is to have the initial cmpxchg inlined and the cold case loop in
> an uninlined function.) But I'd like an opinion from someone who knows
> nscd better than I do.

I've made it an inline function now and used bool.

Here's the patch, tested on Linux/x86-64. Ok now?

Andreas

diff --git a/nscd/nscd-client.h b/nscd/nscd-client.h
index e57a23c..5003d39 100644
--- a/nscd/nscd-client.h
+++ b/nscd/nscd-client.h
@@ -1,5 +1,4 @@
-/* Copyright (c) 1998, 1999, 2000, 2003, 2004, 2005, 2006, 2007, 2009, 2011
-   Free Software Foundation, Inc.
+/* Copyright (c) 1998-2012 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
    Contributed by Thorsten Kukuk <kukuk@suse.de>, 1998.
 
@@ -322,6 +321,24 @@ struct locked_map_ptr
 };
 #define libc_locked_map_ptr(class, name) class struct locked_map_ptr name
 
+/* Try acquiring lock for mapptr, returns true if it succeeds, false 
+   if not.  */
+static inline bool __nscd_acquire_maplock (volatile struct locked_map_ptr *mapptr)
+{
+  int cnt = 0;
+  while (__builtin_expect (atomic_compare_and_exchange_val_acq (&mapptr->lock,
+								1, 0) != 0, 0))
+    {
+      // XXX Best number of rounds?
+      if (__builtin_expect (++cnt > 5, 0))
+	return false;
+
+      atomic_delay ();
+    }
+
+  return true;
+}
+
 
 /* Open socket connection to nscd server.  */
 extern int __nscd_open_socket (const char *key, size_t keylen,
diff --git a/nscd/nscd_gethst_r.c b/nscd/nscd_gethst_r.c
index c1661f8..d64ad2e 100644
--- a/nscd/nscd_gethst_r.c
+++ b/nscd/nscd_gethst_r.c
@@ -1,5 +1,4 @@
-/* Copyright (C) 1998-2005, 2006, 2007, 2008, 2009, 2011
-   Free Software Foundation, Inc.
+/* Copyright (C) 1998-2012 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
    Contributed by Ulrich Drepper <drepper@cygnus.com>, 1998.
 
@@ -100,9 +99,18 @@ libc_freeres_fn (hst_map_free)
 uint32_t
 __nscd_get_nl_timestamp (void)
 {
+  uint32_t retval;
   if (__nss_not_use_nscd_hosts != 0)
     return 0;
 
+  /* __nscd_get_mapping can change hst_map_handle.mapped to NO_MAPPING.
+   However, __nscd_get_mapping assumes the prior value was not NO_MAPPING.
+   Thus we have to acquire the lock to prevent this thread from changing
+   hst_map_handle.mapped to NO_MAPPING while another thread is inside
+    __nscd_get_mapping.  */
+  if (!__nscd_acquire_maplock (&__hst_map_handle))
+    return 0;
+
   struct mapped_database *map = __hst_map_handle.mapped;
 
   if (map == NULL
@@ -112,9 +120,14 @@ __nscd_get_nl_timestamp (void)
     map = __nscd_get_mapping (GETFDHST, "hosts", &__hst_map_handle.mapped);
 
   if (map == NO_MAPPING)
-    return 0;
+    retval = 0;
+  else
+    retval = map->head->extra_data[NSCD_HST_IDX_CONF_TIMESTAMP];
+
+  /* Release the lock.  */
+  __hst_map_handle.lock = 0;
 
-  return map->head->extra_data[NSCD_HST_IDX_CONF_TIMESTAMP];
+  return retval;
 }
 
 
diff --git a/nscd/nscd_helper.c b/nscd/nscd_helper.c
index 92558b6..96fb93d 100644
--- a/nscd/nscd_helper.c
+++ b/nscd/nscd_helper.c
@@ -1,4 +1,4 @@
-/* Copyright (C) 1998-2007, 2008, 2009 Free Software Foundation, Inc.
+/* Copyright (C) 1998-2012 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
    Contributed by Ulrich Drepper <drepper@cygnus.com>, 1998.
 
@@ -419,7 +419,6 @@ __nscd_get_mapping (request_type type, const char *key,
   return result;
 }
 
-
 struct mapped_database *
 __nscd_get_map_ref (request_type type, const char *name,
 		    volatile struct locked_map_ptr *mapptr, int *gc_cyclep)
@@ -428,16 +427,8 @@ __nscd_get_map_ref (request_type type, const char *name,
   if (cur == NO_MAPPING)
     return cur;
 
-  int cnt = 0;
-  while (__builtin_expect (atomic_compare_and_exchange_val_acq (&mapptr->lock,
-								1, 0) != 0, 0))
-    {
-      // XXX Best number of rounds?
-      if (__builtin_expect (++cnt > 5, 0))
-	return NO_MAPPING;
-
-      atomic_delay ();
-    }
+  if (!__nscd_acquire_maplock (mapptr))
+    return NO_MAPPING;
 
   cur = mapptr->mapped;
 


-- 
 Andreas Jaeger aj@{suse.com,opensuse.org} Twitter/Identica: jaegerandi
  SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
   GF: Jeff Hawn,Jennifer Guild,Felix Imendörffer,HRB16746 (AG Nürnberg)
    GPG fingerprint = 93A3 365E CE47 B889 DF7F  FED1 389A 563C C272 A126


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]