Re: [PATCH] Make pthread_getspecific async-signal-safe

[Andrew Hunter <ahh at google dot com>]

Ping -- anyone have thoughts on this?

On Mon, Dec 29, 2014 at 12:17 PM, Andrew Hunter <ahh@google.com> wrote:
> This is version 3 of the patch.  Changes:
>
> - formatting and documentation as requested by Roland
> - dropped tst-key{6,7} (as I always planned to)
> - use of atomics
>
> the last requires a bit of explanation w/r/t my earlier mail:
> essentially, the C{,++}11 standard is hugely in error when it comes to
> races with signal handler--my complaints above mostly were in
> reference to that language.  The C++14 standard largely fixes this,
> using the language of races used elsewhere in atomics.  My new
> versions of the code use atomic (relaxed) synchronization on
> data->seq, plus signal_fence, to make this (I think) a race-free
> program [1] (nit: so long as proper acq/rel barriers communicate the
> value of the key used to call getspecific into the signal handler, but
> we needed that anyway.)  It is fairly straightforward, and does not
> require doing all accesses via sigatomic_t or similar :).
>
> PTAL.
>
> [1] for keys in the static block.  Technically speaking it's still
> possible to race, though probably not worth fixing for now, the read
> of THREAD_GETMEM_NC (THREAD_SELF, specific, idx1st).  The easy way to
> fix this is to specify THREAD_GETMEM_NC as doing a relaxed atomic
> load, which we should do anyway (and it certainly does, on x86 at
> least.)
>
> On Mon, Dec 29, 2014 at 12:07 PM, Andrew Hunter <ahh@google.com> wrote:
>> While not specified as such as POSIX, projects (notably MSAN) rely on
>> pthread_getspecific from signal handlers as being async-signal-safe,
>> and it very nearly is.  However, currently, getspecific clears out
>> "out of date" data values (with old seqnums) instead of just returning
>> NULL and not touching the data. This is unnecessary (nothing will ever
>> look at the stale data), and in the signal handler case, is dangerous:
>> if a signal interrupts pthread_setspecific, and the compiler (legally)
>> decides to write data before seq, we can stomp on a perfectly valid
>> piece of per-thread data.
>>
>> Fix this by making pthread_getspecific look but don't touch.  This
>> very modestly _improves_ its performance, in both the case where data
>> is present and absent (but it's fast enough that we really don't
>> care--I'm mostly just pointing out that this isn't a performance
>> *loss*.)  More importantly, it's now safe to call getspecific from a
>> signal handler, even in the presence of a racing setspecific. This is
>> verified by tst-key5.
>> ---
>>  ChangeLog                  |   5 +++
>>  include/atomic.h           |  17 ++++++++
>>  manual/threads.texi        |   3 +-
>>  nptl/Makefile              |   3 +-
>>  nptl/pthread_getspecific.c |  19 ++++----
>>  nptl/pthread_setspecific.c |   6 ++-
>>  nptl/tst-key5.c            | 106 +++++++++++++++++++++++++++++++++++++++++++++
>>  7 files changed, 146 insertions(+), 13 deletions(-)
>>  create mode 100644 nptl/tst-key5.c
>>
>> diff --git a/ChangeLog b/ChangeLog
>> index fede1bb..b511884 100644
>> --- a/ChangeLog
>> +++ b/ChangeLog
>> @@ -1,3 +1,8 @@
>> +2014-12-16  Andrew Hunter <ahh@google.com>
>> +       * nptl/pthread_getspecific.c : fix async signal safety
>> +       * nptl/tst-key5.c : test for async signal safety
>> +       * include/atomic.h : add atomic_signal_fence
>> +
>>  2014-12-05  Adhemerval Zanella  <azanella@linux.vnet.ibm.com>
>>
>>         * libio/tst-ftell-active-handler.c (do_ftell_test): Fix buffer overrun
>> diff --git a/include/atomic.h b/include/atomic.h
>> index c4df8f1..dc500b8 100644
>> --- a/include/atomic.h
>> +++ b/include/atomic.h
>> @@ -575,6 +575,13 @@ void __atomic_link_error (void);
>>  # define atomic_thread_fence_seq_cst() \
>>    __atomic_thread_fence (__ATOMIC_SEQ_CST)
>>
>> +# define atomic_signal_fence_acquire() \
>> +  __atomic_signal_fence (__ATOMIC_ACQUIRE)
>> +# define atomic_signal_fence_release() \
>> +  __atomic_signal_fence (__ATOMIC_RELEASE)
>> +# define atomic_signal_fence_seq_cst() \
>> +  __atomic_signal_fence (__ATOMIC_SEQ_CST)
>> +
>>  # define atomic_load_relaxed(mem) \
>>    ({ __atomic_check_size((mem)); __atomic_load_n ((mem), __ATOMIC_RELAXED); })
>>  # define atomic_load_acquire(mem) \
>> @@ -651,6 +658,16 @@ void __atomic_link_error (void);
>>  #  define atomic_thread_fence_seq_cst() atomic_full_barrier ()
>>  # endif
>>
>> +# ifndef atomic_signal_fence_acquire
>> +#  define atomic_signal_fence_acquire() __asm ("" ::: "memory")
>> +# endif
>> +# ifndef atomic_signal_fence_release
>> +#  define atomic_signal_fence_release() __asm ("" ::: "memory")
>> +# endif
>> +# ifndef atomic_signal_fence_seq_cst
>> +#  define atomic_signal_fence_seq_cst() __asm ("" ::: "memory")
>> +# endif
>> +
>>  # ifndef atomic_load_relaxed
>>  #  define atomic_load_relaxed(mem) \
>>     ({ __typeof (*(mem)) __atg100_val;                                        \
>> diff --git a/manual/threads.texi b/manual/threads.texi
>> index 4d080d4..18ae530 100644
>> --- a/manual/threads.texi
>> +++ b/manual/threads.texi
>> @@ -55,7 +55,8 @@ is it called during thread exit.
>>  @safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}}
>>  @c pthread_getspecific ok
>>  Return the thread-specific data associated with @var{key} in the calling
>> -thread.
>> +thread.  This function is async-signal-safe, by special glibc
>> +guarantee over and above the POSIX specification.
>>  @end deftypefun
>>
>>  @comment pthread.h
>> diff --git a/nptl/Makefile b/nptl/Makefile
>> index ac76596..dcfccbb 100644
>> --- a/nptl/Makefile
>> +++ b/nptl/Makefile
>> @@ -224,7 +224,7 @@ tests = tst-typesizes \
>>         tst-rwlock5 tst-rwlock6 tst-rwlock7 tst-rwlock8 tst-rwlock9 \
>>         tst-rwlock10 tst-rwlock11 tst-rwlock12 tst-rwlock13 tst-rwlock14 \
>>         tst-once1 tst-once2 tst-once3 tst-once4 \
>> -       tst-key1 tst-key2 tst-key3 tst-key4 \
>> +       tst-key1 tst-key2 tst-key3 tst-key4 tst-key5 \
>>         tst-sem1 tst-sem2 tst-sem3 tst-sem4 tst-sem5 tst-sem6 tst-sem7 \
>>         tst-sem8 tst-sem9 tst-sem10 tst-sem11 tst-sem12 tst-sem13 tst-sem14 \
>>         tst-barrier1 tst-barrier2 tst-barrier3 tst-barrier4 \
>> @@ -529,6 +529,7 @@ endif
>>
>>  $(objpfx)tst-cancel17: $(librt)
>>  $(objpfx)tst-cancelx17: $(librt)
>> +$(objpfx)tst-key5: $(librt)
>>  $(objpfx)tst-_res1mod2.so: $(objpfx)tst-_res1mod1.so
>>  LDFLAGS-tst-_res1mod1.so = -Wl,-soname,tst-_res1mod1.so
>>  LDFLAGS-tst-_res1mod2.so = -Wl,-soname,tst-_res1mod2.so
>> diff --git a/nptl/pthread_getspecific.c b/nptl/pthread_getspecific.c
>> index e0cc199..355f147 100644
>> --- a/nptl/pthread_getspecific.c
>> +++ b/nptl/pthread_getspecific.c
>> @@ -53,16 +53,15 @@ __pthread_getspecific (key)
>>        data = &level2[idx2nd];
>>      }
>>
>> -  void *result = data->data;
>> -  if (result != NULL)
>> -    {
>> -      uintptr_t seq = data->seq;
>> -
>> -      if (__glibc_unlikely (seq != __pthread_keys[key].seq))
>> -       result = data->data = NULL;
>> -    }
>> -
>> -  return result;
>> +  /* Since we might be inside a signal handler, use atomic ops to
>> +     synchronize with ourselves, and prevent speculative (and racy)
>> +     reading of data--we don't know it's valid until after we see a
>> +     valid seqnum.  */
>> +  if (__glibc_unlikely (
>> +          atomic_load_relaxed(&data->seq) != __pthread_keys[key].seq))
>> +    return NULL;
>> +  atomic_signal_fence_acquire();
>> +  return data->data;
>>  }
>>  strong_alias (__pthread_getspecific, pthread_getspecific)
>>  hidden_def (__pthread_getspecific)
>> diff --git a/nptl/pthread_setspecific.c b/nptl/pthread_setspecific.c
>> index 0ace861..52ce948 100644
>> --- a/nptl/pthread_setspecific.c
>> +++ b/nptl/pthread_setspecific.c
>> @@ -86,8 +86,12 @@ __pthread_setspecific (key, value)
>>
>>    /* Store the data and the sequence number so that we can recognize
>>       stale data.  */
>> -  level2->seq = seq;
>>    level2->data = (void *) value;
>> +  /* pthread_getspecific might be called from a signal handler--order
>> +     writes so that if it can see a valid seqnum, it can
>> +     see our data.  */
>> +  atomic_signal_fence_release();
>> +  atomic_store_relaxed(&level2->seq, seq);
>>
>>    return 0;
>>  }
>> diff --git a/nptl/tst-key5.c b/nptl/tst-key5.c
>> new file mode 100644
>> index 0000000..56bd4f0
>> --- /dev/null
>> +++ b/nptl/tst-key5.c
>> @@ -0,0 +1,106 @@
>> +/* Copyright (C) 2014 Free Software Foundation, Inc.
>> +   This file is part of the GNU C Library.
>> +
>> +   The GNU C Library is free software; you can redistribute it and/or
>> +   modify it under the terms of the GNU Lesser General Public
>> +   License as published by the Free Software Foundation; either
>> +   version 2.1 of the License, or (at your option) any later version.
>> +
>> +   The GNU C Library is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> +   Lesser General Public License for more details.
>> +
>> +   You should have received a copy of the GNU Lesser General Public
>> +   License along with the GNU C Library; if not, see
>> +   <http://www.gnu.org/licenses/>.  */
>> +
>> +#include <pthread.h>
>> +#include <unistd.h>
>> +#include <signal.h>
>> +#include <time.h>
>> +#include <assert.h>
>> +#include <string.h>
>> +#include <stdio.h>
>> +
>> +/* Test that pthread_getspecific called from a signal handler doesn't
>> +   cause problems, even when racing with potential key_create or
>> +   setspecific calls.  */
>> +
>> +pthread_key_t key;
>> +void *value;
>> +size_t r;
>> +static void
>> +handler (int signo)
>> +{
>> +  void *ret = pthread_getspecific (key);
>> +  /* We race with the setspecific--either result is fine, just not junk.  */
>> +  assert (ret == value || ret == NULL);
>> +  r++;
>> +}
>> +
>> +
>> +int
>> +do_test (void)
>> +{
>> +  struct sigaction sa;
>> +  memset (&sa, 0, sizeof (sa));
>> +  sa.sa_handler = handler;
>> +
>> +  if (0 != == sigaction (SIGUSR1, &sa, NULL))
>> +    {
>> +      puts ("sigaction failed");
>> +      exit (1);
>> +    }
>> +
>> +  timer_t timer;
>> +  struct sigevent sevp;
>> +  sevp.sigev_notify = SIGEV_SIGNAL;
>> +  sevp.sigev_signo = SIGUSR1;
>> +  if (0 != timer_create (CLOCK_MONOTONIC, &sevp, &timer))
>> +    {
>> +      puts ("timer_create failed");
>> +      exit (1);
>> +    }
>> +  struct itimerspec spec;
>> +  spec.it_value.tv_sec = 0;
>> +  spec.it_value.tv_nsec = 500;
>> +  spec.it_interval = spec.it_value;
>> +  timer_settime (timer, 0, &spec, NULL);
>> +#define NITERS (1000 * 1000)
>> +  for (int i = 0; i < NITERS; ++i)
>> +    {
>> +      /* This is an arbitrary value, unique between iterations, so
>> +         we're not fooled by seeing an old value.  */
>> +      value = (void *) ((intptr_t)i + 1);
>> +      if (0 != pthread_key_create (&key, NULL))
>> +        {
>> +          puts ("pthread_key_create failed");
>> +          exit (1);
>> +        }
>> +      if (0 != pthread_setspecific (key, value))
>> +        {
>> +          puts ("pthread_setspecific failed");
>> +          exit (1);
>> +        }
>> +      /* This is the key test: if we raced with a getspecific from
>> +         signal handler, it's important we get back the key we asked
>> +         for--it should not have been removed or changed by the other
>> +         call.  */
>> +      if (value != pthread_getspecific (key))
>> +        {
>> +          printf ("Lost a value\n");
>> +          return 1;
>> +        }
>> +      if (0 != pthread_key_delete (key))
>> +        {
>> +          puts ("pthread_key_delete failed");
>> +          exit (1);
>> +        }
>> +    }
>> +  timer_delete (timer);
>> +  return 0;
>> +}
>> +
>> +#define TEST_FUNCTION do_test ()
>> +#include "../test-skeleton.c"
>> --
>> 2.2.0.rc0.207.ga3a616c
>>