This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCHv3] powerpc: Fix write-after-destroy in lock elision
On 02/01/2017 15:39, Torvald Riegel wrote:
> On Mon, 2017-01-02 at 15:25 -0200, Adhemerval Zanella wrote:
>>
>> On 02/01/2017 15:21, Torvald Riegel wrote:
>>> On Mon, 2017-01-02 at 15:03 -0200, Adhemerval Zanella wrote:
>>>> LGTM with a nit only on the comment about the adapt_count decrement in
>>>> unlock.
>>>>
>>>> On 21/12/2016 16:03, Tulio Magno Quites Machado Filho wrote:
>>>>> From: Rajalakshmi Srinivasaraghavan <raji@linux.vnet.ibm.com>
>>>>>
>>>>> Changes since v2:
>>>>> - Fixed coding style.
>>>>>
>>>>> Changes since v1:
>>>>> - Removed references to data race by the actual error: write-after-destroy.
>>>>> - Enclosed adapt_count accesses in C11 atomics.
>>>>>
>>>>> --- 8< ---
>>>>>
>>>>> The update of *adapt_count after the release of the lock causes a race
>>>>> condition when thread A unlocks, thread B continues and destroys the
>>>>> mutex, and thread A writes to *adapt_count.
>>>>
>>>>
>>>>>
>>>>> diff --git a/sysdeps/unix/sysv/linux/powerpc/elision-unlock.c b/sysdeps/unix/sysv/linux/powerpc/elision-unlock.c
>>>>> index 43c5a67..0e0baf5 100644
>>>>> --- a/sysdeps/unix/sysv/linux/powerpc/elision-unlock.c
>>>>> +++ b/sysdeps/unix/sysv/linux/powerpc/elision-unlock.c
>>>>> @@ -28,13 +28,17 @@ __lll_unlock_elision (int *lock, short *adapt_count, int pshared)
>>>>> __libc_tend (0);
>>>>> else
>>>>> {
>>>>> - lll_unlock ((*lock), pshared);
>>>>> -
>>>>> - /* Update the adapt count AFTER completing the critical section.
>>>>> - Doing this here prevents unneeded stalling when entering
>>>>> - a critical section. Saving about 8% runtime on P8. */
>>>>> + /* Update adapt_count in the critical section to prevent a
>>>>> + write-after-destroy error as mentioned in BZ 20822. The
>>>>> + following update of adapt_count is clearly contained within
>>>>> + the critical region of the fall-back lock as it immediately
>>>>> + precedes the unlock. Attempting to use C11 atomics to access
>>>>> + adapt_count would be (at minimum) misleading and (at worse) a
>>>>> + serious error forcing a larx-hit-stcx flush. */
>>>>> if (*adapt_count > 0)
>>>>> (*adapt_count)--;
>>>>
>>>> I think C11 atomic comment should outline that using atomic_fetch_add_relaxed
>>>> generates suboptimal code, not misleading, since it generates an atomic sequence
>>>> using lharx/sthcx. on a transaction region. Something like:
>>>>
>>>> /* Update adapt_count in the critical section to prevent a
>>>> write-after-destroy error as mentioned in BZ 20822. The
>>>> following update of adapt_count is clearly contained within
>>>> the critical region of the fall-back lock as it immediately
>>>> precedes the unlock.
>>>> Using C11 atomics like atomic_fetch_add_relaxed generates
>>>> suboptimal code (lharx/sthcx. sequence) where an plain load,
>>>> update, store is suffice inside a HTM region (the whole
>>>> idea of using this construction). */
>>>
>
> Looking at this again, this is not guaranteed to be in a transaction.
> (Wasn't all this supposed to always be in transactional code? Or am I
> mixing this up with another patch?)
You are right, the adapt_count decrement is indeed on the default LL/SC
patch, not the HTM one.
>
> It uses the lock, so can be concurrent with other accesses (that use
> atomics). Thus, it must use atomics too to prevent data races.
Indeed, we need to use atomic decrement on this one as well. I am not
sure if is needed to use a stronger semantic than relaxed.
>
> The comment should also mention the mutex destruction requirements by
> POSIX because this is how we name that requirement everywhere else.
>
Nod.