Re: [PATCH bpf-next 1/3] s390/bpf: Add orig_gpr2 to user_pt_regs

["Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>]

Ilya Leoshkevich wrote:
> On Fri, 2022-02-04 at 08:21 +0000, Naveen N. Rao wrote:
>> Naveen N. Rao wrote:
>> > Hi Heiko,
>> > 
>> > Heiko Carstens wrote:
>> > > On Wed, Feb 02, 2022 at 12:41:58AM +0100, Ilya Leoshkevich wrote:
>> > > > user_pt_regs is used by eBPF in order to access userspace
>> > > > registers -
>> > > > see commit 466698e654e8 ("s390/bpf: correct broken uapi for
>> > > > BPF_PROG_TYPE_PERF_EVENT program type"). In order to access the
>> > > > first
>> > > > syscall argument from eBPF programs, we need to export
>> > > > orig_gpr2.
>> > > > 
>> > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
>> > > > ---
>> > > >  arch/s390/include/asm/ptrace.h      | 2 +-
>> > > >  arch/s390/include/uapi/asm/ptrace.h | 1 +
>> > > >  2 files changed, 2 insertions(+), 1 deletion(-)
>> > > > 
>> > > > diff --git a/arch/s390/include/asm/ptrace.h
>> > > > b/arch/s390/include/asm/ptrace.h
>> > > > index 4ffa8e7f0ed3..c8698e643904 100644
>> > > > --- a/arch/s390/include/asm/ptrace.h
>> > > > +++ b/arch/s390/include/asm/ptrace.h
>> > > > @@ -83,9 +83,9 @@ struct pt_regs {
>> > > >                         unsigned long args[1];
>> > > >                         psw_t psw;
>> > > >                         unsigned long gprs[NUM_GPRS];
>> > > > +                       unsigned long orig_gpr2;
>> > > >                 };
>> > > >         };
>> > > > -       unsigned long orig_gpr2;
>> > > >         union {
>> > > >                 struct {
>> > > >                         unsigned int int_code;
>> > > > diff --git a/arch/s390/include/uapi/asm/ptrace.h
>> > > > b/arch/s390/include/uapi/asm/ptrace.h
>> > > > index ad64d673b5e6..b3dec603f507 100644
>> > > > --- a/arch/s390/include/uapi/asm/ptrace.h
>> > > > +++ b/arch/s390/include/uapi/asm/ptrace.h
>> > > > @@ -295,6 +295,7 @@ typedef struct {
>> > > >         unsigned long args[1];
>> > > >         psw_t psw;
>> > > >         unsigned long gprs[NUM_GPRS];
>> > > > +       unsigned long orig_gpr2;
>> > > >  } user_pt_regs;
>> > > 
>> > > Isn't this broken on nearly all architectures? I just checked
>> > > powerpc,
>> > > arm64, and riscv. While powerpc seems to mirror pt_regs as
>> > > user_pt_regs,
>> > > and therefore exports orig_gpr3, the bpf macros still seem to
>> > > access the
>> > > wrong location to access the first syscall parameter(?).
>> > 
>> > On powerpc, gpr[3] continues to be valid on syscall entry (so this
>> > test 
>> > passes on powerpc), though orig_gpr3 will remain valid throughout.
>> 
>> Hmm.. we can't use orig_gpr3 since we don't use a syscall wrapper.
>> All 
>> system calls just receive the parameters directly.
>> 
>> - Naveen
> 
> Right, I ran into this yesterday as well.
> I solved it in v2
> (https://lore.kernel.org/bpf/20220204041955.1958263-1-iii@linux.ibm.com/)
> by introducing a macro that hides whether or not an arch uses a syscall
> wrapper.

Thanks. I missed your patches. I will take a look.

- Naveen
>