Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Wed, Feb 28, 2018 at 3:23 PM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 02/25/2018 07:55 PM, H.J. Lu wrote:
>> On Sun, Feb 25, 2018 at 6:13 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
>>> * H. J. Lu:
>>>
>>>> On Sun, Feb 25, 2018 at 5:49 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
>>>>> * H. J. Lu:
>>>>>
>>>>>> On Sun, Feb 25, 2018 at 5:31 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
>>>>>>> * H. J. Lu:
>>>>>>>
>>>>>>>> libpthread cancellation implementation passes cancel_jmp_buf to
>>>>>>>> libgcc unwinder,
>>>>>>>
>>>>>>> Oh.  Where does it do that?  If you mean _Unwind_ForcedUnwind, I think
>>>>>>> that's just an opaque closure argument for the callback.
>>>>>>
>>>>>> Yes.  Libgcc unwinder needs to deal with it.
>>>>>
>>>>> Please point me to the code.  Thanks.
>>>>
>>>> sysdeps/nptl/unwind-forcedunwind.c has
>>>>
>>>> _Unwind_Reason_Code
>>>> _Unwind_ForcedUnwind (struct _Unwind_Exception *exc, _Unwind_Stop_Fn stop,
>>>>       void *stop_argument)
>>>> {
>>>>   if (__glibc_unlikely (libgcc_s_handle == NULL))
>>>>     pthread_cancel_init ();
>>>>   else
>>>>     atomic_read_barrier ();
>>>>
>>>>   _Unwind_Reason_Code (*forcedunwind)
>>>>     (struct _Unwind_Exception *, _Unwind_Stop_Fn, void *)
>>>>     = libgcc_s_forcedunwind;
>>>>   PTR_DEMANGLE (forcedunwind);
>>>>   return forcedunwind (exc, stop, stop_argument);
>>>> }
>>>
>>> Thanks.  I think stop_argument ends up in the private_2 member inside
>>> unwind.inc, which is only passed back to the callback (the stop
>>> function pointer) in _Unwind_ForcedUnwind_Phase2, and not interpreted
>>> by libgcc itself.  So this shouldn't be a problem.
>>
>> Please take a look at hjl/setjmp/cancel branch:
>>
>> https://github.com/hjl-tools/glibc/tree/hjl/setjmp/cancel
>
> OK.
>
>> Functions, like LIBC_START_MAIN, START_THREAD_DEFN as well as these
>> with thread cancellation, call setjmp, but never return to their
>> callers after longjmp returns.  This patch adds <bits/setjmp-cancel.h>
>> and <setjmp-cancelP.h> to provide a version of setjmp family functions,
>> __setjmp_cancel and __sigsetjmp_cancel, which are used to implement
>> thread cancellation.  The default __setjmp_cancel and __sigsetjmp_cancel
>> are defined as setjmp and __sigsetjmp, respectively, which are the same
>> as before.
>
>> On x86, a different version is added to avoid saving and restoring
>> shadow stack register.  __libc_longjmp, which is a private interface
>> for cancellation implementation in libpthread, is changed to call
>> __longjmp_cancel instead of __longjmp.
>
> The consequence of this solution is that any function calls after the
> first longjmp will continue to extend the shadow stack because there is
> no restoration? I think this is OK, you effectively need to have enough
> shadow stack to run through all of the unwind functions as if they were
> nested frames on the shadow stack.
>
> Do we see that being a problem?
>
>> This leaves cancel_jmp_buf unchanged.  But is it worth it?
>
> Absolutely it is worth it. This new design looks much simpler to support.
>
> The technical trade is as follows:
>
> * Three new versioned symbols for __sigsetjmp, __sigsetjmp_cancel,
>   and __setjmp_cancel.
>
>   * You can avoid versioning by placing the shadow stack pointer such
>     that it is written into the pad[4] of the cancel_jmp_buf, and thus
>     it would always have space to be written. However, it is cleaner
>     to have cancel versions of these functions that do less.
>
> vs.
>
> * An ABI change that requires coordination across glibc, gcc, and
>   binutils to ensure there is a "flag day" where the ABI can be correctly
>   selected at runtime.
>
> The latter is not easy to execute for downstream, and the versioned
> symbols provide a good barrier in terms of compatibility and errors
> reported by the dynamic loader.
>
>> 1. I have to add  __setjmp_cancel and __sigsetjmp_cancel which won't
>> save and restore shadow stack register.

I have been testing this.  I ran into one issue.  GCC knows that setjmp will
return via longjmp and inserts ENDBR after it.  But it doesn't know
  __setjmp_cancel and __sigsetjmp_cancel.   We can either add them to GCC
or add NOTRACK prefix to the corresponding longjmps.

> OK.
>
>> 2. I still need yet to add the new version of __sigsetjmp for older binaries.
>
> OK.
>
>> 3, Older .o files compiled against glibc 2.27 are still incompatible with
>> glibc 2.28.
>
> This is normal, and has never been assured.
>
> --
> Cheers,
> Carlos.



-- 
H.J.