[PATCH 1/3] powerpc: Split user/kernel definitions of struct pt_regs

[Madhavan Srinivasan]

On Monday 15 October 2018 04:38 PM, Michael Ellerman wrote:
> Madhavan Srinivasan <maddy at linux.vnet.ibm.com> writes:
>
>> On Saturday 13 October 2018 04:26 PM, Michael Ellerman wrote:
>>> We use a shared definition for struct pt_regs in uapi/asm/ptrace.h.
>>> That means the layout of the structure is ABI, ie. we can't change it.
>>>
>>> That would be fine if it was only used to describe the user-visible
>>> register state of a process, but it's also the struct we use in the
>>> kernel to describe the registers saved in an interrupt frame.
>>>
>>> We'd like more flexibility in the content (and possibly layout) of the
>>> kernel version of the struct, but currently that's not possible.
>>>
>>> So split the definition into a user-visible definition which remains
>>> unchanged, and a kernel internal one.
>>>
>>> At the moment they're still identical, and we check that at build
>>> time. That's because we have code (in ptrace etc.) that assumes that
>>> they are the same. We will fix that code in future patches, and then
>>> we can break the strict symmetry between the two structs.
>> Nice and awesome. But just trying to understand. What will
>> *regs will point to in the "struct sigcontext".
> Yeah that's a bit fishy.
>
> It should always point to a user_pt_regs.
>
> So in the kernel we want:
>
>    struct sigcontext {
>    	...
>    	struct user_pt_regs	__user *regs;
>
> And in userspace we want:
>
>    struct sigcontext {
>    	...
>    	struct pt_regs	__user *regs;
>
>
> I think it's not actually broken at the moment, because it's just a
> pointer, and we don't do anything based on the sizeof() the type.

yes. This clarifies. But still perf/perf_regs.c needs changes.
Because perf support dumping user_space regs and interrupt regs.
Once again, we dont use any sizeof(), but need to handle the
user_pt_regs changes.

I will have a look at that in the morning.

Thanks for clarification.
Maddy

>
> But still we should fix it.
>
> I guess I'll do this:
>
> diff --git a/arch/powerpc/include/uapi/asm/sigcontext.h b/arch/powerpc/include/uapi/asm/sigcontext.h
> index 2fbe485acdb4..630aeda56d59 100644
> --- a/arch/powerpc/include/uapi/asm/sigcontext.h
> +++ b/arch/powerpc/include/uapi/asm/sigcontext.h
> @@ -22,7 +22,11 @@ struct sigcontext {
>   #endif
>   	unsigned long	handler;
>   	unsigned long	oldmask;
> -	struct pt_regs	__user *regs;
> +#ifdef __KERNEL__
> +	struct user_pt_regs __user *regs;
> +#else
> +	struct pt_regs	*regs;
> +#endif
>   #ifdef __powerpc64__
>   	elf_gregset_t	gp_regs;
>   	elf_fpregset_t	fp_regs;
>
>
> Thanks for the review.
>
> cheers
>