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

[Rich Felker]

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.

Rich