[PATCH] #13594: Avoid race in nscd

[Andreas Jaeger]

On 05/14/2012 10:37 PM, Carlos O'Donell wrote:
> On Mon, May 14, 2012 at 4:11 PM, Andreas Jaeger<aj@suse.de>  wrote:
>>
>> Jeff Law says in bugzilla:
>> "Based on just reading the code, I wonder if a one thread is mucking up
>> hst_map_handle.mapped behind the back of nscd_get_mapping.
>>
>> nscd_get_nl_timestamp doesn't bother to grab the hst_map_handle lock and
>> calls
>> into nscd_get_mapping which could potentially change hst_map_handle.mapped
>> to
>> NO_MAPPING.
>>
>> If this occurs after another thread had passed the NO_MAPPING check in
>> nscd_get_map_ref, but hasn't yet hit the atomic_decrement_val in
>> nscd_get_mapping then it could cause the failure mode reported in this
>> report
>> (and several others across various distros, upstream kde and possibly
>> elsewhere)."
>>
>> I agree with his analysis and cleaned up the patch submission (added
>> copyright header and ChangeLog entry).
>>
>> This patch has been tested in openSUSE and Fedora.
>>
>> Ok to commit? Andreas
>>
>> 2012-05-14  Jeff Law<law@redhat.com>
>>
>>         [BZ #13594]
>>         * nscd/nscd_gethst_r.c (__nscd_get_nl_timestamp): Add locking to
>>         code changing __hst_map_handle.map.
>>
>> diff --git a/nscd/nscd_gethst_r.c b/nscd/nscd_gethst_r.c
>> index c1661f8..36c4bb3 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,27 @@ 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;
>>   +  int cnt = 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.  */
>> +  while (__builtin_expect
>> +        (atomic_compare_and_exchange_val_acq (&__hst_map_handle.lock,
>> +                                              1, 0) != 0, 0))
>> +    {
>> +      /* XXX Best number of rounds?  */
>> +      if (__builtin_expect (++cnt>  5, 0))
>> +       return 0;
>> +
>> +      atomic_delay ();
>> +    }
>> +
>
> Roland has already OK'd the patch for checkin so my questions are
> purely academic for now.

I haven't committed it yet so please speak up if you disagree...

> Why are we rolling a bespoke pthread_spin_lock()?
>
> If we have threads, why aren't we using the pthreads interface?

The handle is already in the code and used this way, so we're compatible.

>>    struct mapped_database *map = __hst_map_handle.mapped;
>>     if (map == NULL
>> @@ -112,9 +129,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];
>>   -  return map->head->extra_data[NSCD_HST_IDX_CONF_TIMESTAMP];
>> +  /* Release the lock.  */
>> +  __hst_map_handle.lock = 0;
>
> Is this sufficient to guarantee atomicity? Is it because we use the
> *_acq sematic atomic that it works?

The acq code checks first that the value is 0 - and only then changes 
it. So with setting it to 0 we release the lock.

Andreas
-- 
  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