Re: [RFC PATCH v2] openrisc: Union fpcsr and oldmask in sigcontext to unbreak userspace ABI

[Stafford Horne]

On Wed, Jul 05, 2023 at 08:59:16PM -0400, Rich Felker wrote:
> On Wed, Jul 05, 2023 at 10:26:45PM +0100, Stafford Horne wrote:
> > With commit 27267655c531 ("openrisc: Support floating point user api") I
> > inproperly added an entry to the struct sigcontext which caused an
> > unwanted to chagne to the userspace ABI.
> > 
> > Since oldmask was never used in OpenRISC we now use it's space for the
> > floating point fpcsr state.  We do this with a union to restore the ABI
> > back to the pre kernel v6.4 ABI and keep API compatibility.
> > 
> > This does mean if there is some code somewhere that is setting oldmask
> > in a userspace sighandler it would end up setting the floating point
> > register status, but I think it's unlikely as oldmask was never used
> > before.
> > 
> > Fixes: 27267655c531 ("openrisc: Support floating point user api")
> > Reported-by: Szabolcs Nagy <nsz@xxxxxxxxxx>
> > Closes: https://lore.kernel.org/openrisc/20230626213840.GA1236108@xxxxxxxxxx/
> > Signed-off-by: Stafford Horne <shorne@xxxxxxxxx>
> > ---
> > Changes since v1:
> >  - Rather than revert the change, just use the oldmask slot for fpu state.
> > 
> >  arch/openrisc/include/uapi/asm/sigcontext.h | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/openrisc/include/uapi/asm/sigcontext.h b/arch/openrisc/include/uapi/asm/sigcontext.h
> > index ca585e4af6b8..93817d7051aa 100644
> > --- a/arch/openrisc/include/uapi/asm/sigcontext.h
> > +++ b/arch/openrisc/include/uapi/asm/sigcontext.h
> > @@ -28,8 +28,10 @@
> >  
> >  struct sigcontext {
> >  	struct user_regs_struct regs;  /* needs to be first */
> > -	struct __or1k_fpu_state fpu;
> > -	unsigned long oldmask;
> > +	union {
> > +		struct __or1k_fpu_state fpu;
> > +		unsigned long oldmask;	/* unused */
> > +	};
> >  };
> >  
> >  #endif /* __ASM_OPENRISC_SIGCONTEXT_H */
> > -- 
> > 2.39.1
> 
> Correctness/ABI-safety of this patch depends on struct
> __or1k_fpu_state being just a single unsigned long and never changing.
> Is this something it's possible to commit to? Otherwise, a much better
> idea would be making the union contain a struct __or1k_fpu_state *
> or some more general pointer type that points to an out-of-band,
> expandable data structure past the end of the ucontext_t.

Thank you for reviewing.

Yes, __or1k_fpu_state I intend to always contain just the single unsigned long.

Thinking about it, I should just change sigcontext from using struct
__or1k_fpu_state to simply 'unsigned long fpcsr' to make this more clear. I will
do this for v3.

If further expansion is needed we can add more structs after ucontext_t as you
mention.  I was considering doing that but found the unused space in oldmask.

-Stafford