Re: [PATCH 1/2] Linux/x86: Update cancel_jmp_buf to match __jmp_buf_tag [BZ #22563]

[Carlos O'Donell <carlos at redhat dot com>]

On 01/24/2018 05:09 PM, H.J. Lu wrote:
> On Wed, Jan 24, 2018 at 4:32 PM, Carlos O'Donell <carlos@redhat.com> wrote:
>> On 01/24/2018 10:23 AM, Florian Weimer wrote:
>>> On 01/24/2018 07:08 PM, H.J. Lu wrote:
>>>> We opened a bug:
>>>>
>>>> https://sourceware.org/bugzilla/show_bug.cgi?id=22743
>>>>
>>>> Any help to track down the root cause is appreciated.
>>>
>>> Doesn't the bug report clearly show the root cause?  The offset of
>>> priv.data.cleanup changed, and old binaries have an insufficiently
>>> large stack allocation for the new offset.
>>>
>>> (Congratulations for tracking it down, by the way.  I know that such
>>> bugs are hard.)
>>>
>>> You need to add a symbol version for pthread_register_cancel.  It's
>>> too late for that now, so I recommend reverting the faulty commit.
>>
>> I have finished analyzing this and debugging the root cause myself,
>> and I agree with Florian, we need to revert:
>>
>> commit f81ddabffd76ac9dd600b02adbf3e1dac4bb10ec
>> commit cba595c350e52194e10c0006732e1991e3d0803b
>>
>> At a minimum. I am testing with them reverted locally.
>>
>> To be honest I'm surprised that this passed review and was checked
>> in, because the __pthread_unwind_buf_t has only at most 4-bytes of
>> space left before it is an ABI change. In the future please ping
>> me if you have any doubts and I'll review.
>>
>> The addition of __sigset_t saved_mask moves pthread_unwind_buf's
>> priv.data.cleanup forward by 124-bytes. The on-stack allocation of
>> the pthread_cleanup_push's __pthread_unwind_buf_t is not that big
>> and so __pthread_register_cancel writes to other structures which
>> are allocated on the stack.
>>
>> You cannot expand struct pthread_unwind_buf because the on-stack
>> allocated __pthread_unwind_buf_t is not large enough in existing
>> applications.
>>
>> You *might* have used feature_1 to change between two different
>> layouts of struct pthread_unwind_buf, but that will have to wait
>> for 2.28. As Florian suggests though it is cleaner to version
>> __pthread_register_cancel for x86 and the older version expects
>> the smaller non-CET-enabled structure.
>>
> 
> I will try to fix it by next Monday.
 
I will be reverting these changes in the next 24 hours.

They have a directly proven negative ABI consequences and will be
removed as soon as I finish validation that the reverted patches
are functional and pass the expected tests.

On Monday when you have a full fix ready we can discuss this with 
Dmitry Levin and the other developers to see if everyone agrees
that the solution is acceptable risk for 2.27.

-- 
Cheers,
Carlos.