Re: [PATCH 1/4] or1k: Fix Linux user space signal ABI

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 20/03/24 11:13, Stafford Horne wrote:
> On Wed, Mar 20, 2024 at 10:24:15AM -0300, Adhemerval Zanella Netto wrote:
>>
>>
>> On 19/03/24 18:42, Stafford Horne wrote:
>>> The OpenRISC sigcontext structure has always been defined as:
>>>
>>>     struct user_regs_struct {
>>> 	    /* GPR R0-R31... */
>>> 	    unsigned long gpr[32];
>>> 	    unsigned long pc;
>>> 	    unsigned long sr;
>>>     };
>>>
>>>     struct sigcontext {
>>> 	    struct user_regs_struct regs;  /* needs to be first */
>>> 	    unsigned long oldmask;	/* unused */
>>>     };
>>>
>>> With Linux v6.8 we added FPU support and repurposed the oldmask
>>> to use for the FPCSR (floating point control status register).
>>>
>>>     struct sigcontext {
>>> 	    struct user_regs_struct regs;  /* needs to be first */
>>> 	    union {
>>> 		    unsigned long fpcsr;
>>> 		    unsigned long oldmask;	/* unused */
>>> 	    };
>>>     };
>>>
>>> The definition of mcontext_t was always missing the extra space for
>>> oldmask.  This patch adds the field __fpcsr to mcontext_t to fix the ABI
>>> mismatch between glibc and Linux.
>>
>> This is strictly an ABI break, this won't make the swapcontext functions 
>> to fail (since they are not update to take in consideration the new field),
>> but it also means that the fpcsr won't be save/restore and the application
>> can potentially read uninitialized values.
>>
>> But I take that the fpu support will be a new ABI, so I suggest to fix
>> when you add it (along with proper support to context functions).
> 
> OK, I got it.  I will post this when the hard float code is added and also fixup
> swapcontext etc to populated it correctly with or without hard float.
> 
> Note there is broken ABI already, as programs will not be able to access sigmask
> if needed due to:
> 
> Linux definition:
> 
>     struct ucontext {
> 	    unsigned long     uc_flags;
> 	    struct ucontext  *uc_link;
> 	    stack_t           uc_stack;
> 	    struct sigcontext uc_mcontext;  <-- size differs between glibc and linux
> 	    sigset_t          uc_sigmask;   <-- won't be able to access if needed
>     };
> > But still I will leave as is for now.  This hasn't cause any issues as far as I
> have seen so far.

But even changing the uc_sigmask offset on ucontext would require versioned
context functions.  Usually to access the signal frame ucontex_t, it is safer
to user the linux UAPI definition instead of the libc one.