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

[Michael Ellerman]

Madhavan Srinivasan <ma...@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.

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