Re: [PATCH] Fix incorrect double-checked locking in __nss_database_lookup. [BZ #20483]

[Florian Weimer <fweimer at redhat dot com>]

On 08/18/2016 10:49 PM, Torvald Riegel wrote:
>  DB_LOOKUP_FCT (service_user **ni, const char *fct_name, const char *fct2_name,
>  	       void **fctp)
>  {
> -  if (DATABASE_NAME_SYMBOL == NULL
> -      && __nss_database_lookup (DATABASE_NAME_STRING, ALTERNATE_NAME_STRING,
> -				DEFAULT_CONFIG, &DATABASE_NAME_SYMBOL) < 0)
> +  /* Relaxed MO is fine because this is just about whether we have to perform
> +     the lookup; we will do another acquire-MO load next before assuming that
> +     the lookup has happened.  */
> +  if ((atomic_load_relaxed (&(DATABASE_NAME_SYMBOL)) == NULL)
> +      && (__nss_database_lookup (DATABASE_NAME_STRING, ALTERNATE_NAME_STRING,
> +				 DEFAULT_CONFIG, &DATABASE_NAME_SYMBOL) < 0))
>      return -1;
>
> -  *ni = DATABASE_NAME_SYMBOL;
> +  /* Acquire MO as required by __nss_database_lookup.  */
> +  *ni = atomic_load_acquire (&(DATABASE_NAME_SYMBOL));
>
>    return __nss_lookup (ni, fct_name, fct2_name, fctp);
>  }

I'm not sure if this is the double-checked locking pattern we want to 
use for new code.  GCC currently cannot merge the two loads (and maybe 
it never will do so because it is beneficial to treat the separate loads 
as an optimization hint).

I still think we would be better off if we centralize this particular 
code in a single function with an interface similar to 
__nss_database_lookup.  The manual inlining seems unnecessary.

Thanks,
Florian