Re: [PATCH][BZ #16892] Check value of futex before updating in __lll_timedlock

[Will Newton <will dot newton at linaro dot org>]

On 7 August 2014 13:11, Bernard Ogden <bernie.ogden@linaro.org> wrote:
> Changelog fixed (provided my mail client is now behaving).

It looks like your mail client may still be mangling the patch with
line-wrapping. It is possible to get Thunderbird to stop doing that by
setting the line length to something huge although I think its a
hidden setting somewhere. Alternatively git send-email does the right
thing too.

The patch itself looks good to me though and it would be nice to get
this into 2.20 particularly as it has regressed on MIPS.

> I'm not sure how much detail to put into in the comment - is this better?
>
> I may attempt some comments in other part of the file when I've got a little time.
>
>
> 2014-08-07  Bernard Ogden  <bernie.ogden@linaro.org>
>         [BZ #16892]
>         * sysdeps/nptl/lowlevellock.h (__lll_timedlock): Use
>         atomic_compare_and_exchange_bool_acq rather than atomic_exchange_acq.
>
> ---
> diff --git a/sysdeps/nptl/lowlevellock.h b/sysdeps/nptl/lowlevellock.h
> index 548a9c8..28f4ba3 100644
> --- a/sysdeps/nptl/lowlevellock.h
> +++ b/sysdeps/nptl/lowlevellock.h
> @@ -88,12 +88,15 @@ extern int __lll_timedlock_wait (int *futex, const struct
> timespec *,
>  extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *,
>                                         int private) attribute_hidden;
>
> +/* Take futex if it is untaken.
> +   Otherwise block until either we get the futex or we time out.  */
>  #define __lll_timedlock(futex, abstime, private)                \
>    ({                                                            \
>      int *__futex = (futex);                                     \
>      int __val = 0;                                              \
>                                                                  \
> -    if (__glibc_unlikely (atomic_exchange_acq (__futex, 1)))    \
> +    if (__glibc_unlikely                                        \
> +        (atomic_compare_and_exchange_bool_acq (__futex, 1, 0))) \
>        __val = __lll_timedlock_wait (__futex, abstime, private); \
>      __val;                                                      \
>    })
>
>
> On 06/08/14 22:27, Roland McGrath wrote:
>>> This bug previously affected arm, aarch64, m68k and sh/sh4. Since mips
>>> switched to the generic lowlevellock.h, it is also affected. Applying
>>> this patch will fix arm, aarch64 and mips. m68k and sh would need to
>>> switch to the generic header to get the fix.
>>
>> m68k uses the generic code now.
>>
>>> Change is pretty simple, but has been built and tested on arm.
>>
>>> 2014-08-05  Bernard Ogden  <bernie.ogden@linaro.org>
>>>
>>>         [BZ #16892]
>>>         * sysdeps/nptl/lowlevellock.h: Use
>>>         atomic_compare_and_exchange_bool_acq rather than atomic_exchange_acq
>>>         in __lll_timedlock.
>>
>> This should look like:
>>
>>       [BZ #16892]
>>       * sysdeps/nptl/lowlevellock.h (__lll_timedlock): Use
>>       atomic_compare_and_exchange_bool_acq rather than atomic_exchange_acq.
>>
>> 1. Use 1 tab to indent, not 8 spaces.
>> 2. Put the name of the affected macro/function/variable in parens after the
>>    file name, not just in regular English.
>>
>>> --- a/sysdeps/nptl/lowlevellock.h
>>> +++ b/sysdeps/nptl/lowlevellock.h
>>> @@ -93,7 +93,8 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *,
>>>      int *__futex = (futex);                                     \
>>>      int __val = 0;                                              \
>>>                                                                  \
>>> -    if (__glibc_unlikely (atomic_exchange_acq (__futex, 1)))    \
>>> +    if (__glibc_unlikely                                        \
>>> +        (atomic_compare_and_exchange_bool_acq (__futex, 1, 0))) \
>>>        __val = __lll_timedlock_wait (__futex, abstime, private); \
>>>      __val;                                                      \
>>>    })
>>
>> Please add a comment explaining what the code is doing.  The rest of the
>> file needs more comments like this and I'm not asking you to add all those
>> (unless you want to!).  But where you're touching it, and especially
>> replacing one magic atomic operation with a different magic atomic
>> operation to fix a bug, a comment is really warranted.  If there had been a
>> good comment on the original code, it probably would have prevented us from
>> letting the bug go by in the first place.
>>
>>
>> Thanks,
>> Roland
>>



-- 
Will Newton
Toolchain Working Group, Linaro