[PATCH bpf-next 0/3] libbpf: Fix accessing the first syscall argument on s390

[PATCH bpf-next 0/3] libbpf: Fix accessing the first syscall argument on s390

[Ilya Leoshkevich]

libbpf CI reported a bpf_syscall_macro test failure on s390 [1], which
happens because the code uses gprs[2] instead of orig_gpr2 to access
the first syscall argument. Patches 1-2 are preparations, patch 3 fixes
the issue.

@Heiko, @Vasily, @Christian, @Alexander - could you please review
patch 1, which touches arch/s390? Would it be ok to put it into
bpf-next tree?

[1] https://github.com/libbpf/libbpf/runs/5025905587

Ilya Leoshkevich (3):
  s390/bpf: Add orig_gpr2 to user_pt_regs
  selftests/bpf: Fix an endianness issue in bpf_syscall_macro test
  libbpf: Fix accessing the first syscall argument on s390

 arch/s390/include/asm/ptrace.h                        | 2 +-
 arch/s390/include/uapi/asm/ptrace.h                   | 1 +
 tools/lib/bpf/bpf_tracing.h                           | 9 +++++++++
 tools/testing/selftests/bpf/progs/bpf_syscall_macro.c | 5 ++++-
 4 files changed, 15 insertions(+), 2 deletions(-)

-- 
2.34.1

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

[Ilya Leoshkevich]

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;
 
 /*
-- 
2.34.1

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

[Vasily Gorbik]

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.

> 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;

It could be a good opportunity to get rid of that "args[1]" which is not
used for syscall parameters handling since commit baa071588c3f ("[S390]
cleanup system call parameter setup") [v2.6.37], as well as completely
unused now, and shouldn't really be exported to eBPF. And luckily eBPF
never used it.

So, how about reusing "args[1]" slot for orig_gpr2 instead?

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

[Christian Borntraeger]

Am 02.02.22 um 15:19 schrieb Vasily Gorbik:
> 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.
> 
>> 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;
> 
> It could be a good opportunity to get rid of that "args[1]" which is not
> used for syscall parameters handling since commit baa071588c3f ("[S390]
> cleanup system call parameter setup") [v2.6.37], as well as completely
> unused now, and shouldn't really be exported to eBPF. And luckily eBPF
> never used it.
> 
> So, how about reusing "args[1]" slot for orig_gpr2 instead?

Since this is uapi we certainly have to careful. Reusing this could be ok, though.

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

[Heiko Carstens]

On Wed, Feb 02, 2022 at 06:23:46PM +0100, Christian Borntraeger wrote:
> Am 02.02.22 um 15:19 schrieb Vasily Gorbik:
> > > 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;
> > 
> > It could be a good opportunity to get rid of that "args[1]" which is not
> > used for syscall parameters handling since commit baa071588c3f ("[S390]
> > cleanup system call parameter setup") [v2.6.37], as well as completely
> > unused now, and shouldn't really be exported to eBPF. And luckily eBPF
> > never used it.
> > 
> > So, how about reusing "args[1]" slot for orig_gpr2 instead?
> 
> Since this is uapi we certainly have to careful. Reusing this could be ok, though.

I agree with Vasily: let's get rid of "args[1]", rename it to orig_gpr2,
and effectively move orig_gpr2 to user_pt_regs, while at the same time
reducing the size of struct pt_regs a bit.

This will also prevent future random usages of the args member; e.g. until
recently it was used to pass the last breaking event address to the
exception handler. However that usage has also been removed already.

Ilya, could you resend with this proposed change, please?

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

[Heiko Carstens]

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(?).

For arm64 and riscv it seems that orig_x0 or orig_a0 respectively need to
be added to user_pt_regs too, and the same fix like for s390 needs to be
applied as well.

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

[Andrii Nakryiko]

On Wed, Feb 2, 2022 at 12:14 PM Heiko Carstens <hca@linux.ibm.com> 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(?).
>
> For arm64 and riscv it seems that orig_x0 or orig_a0 respectively need to
> be added to user_pt_regs too, and the same fix like for s390 needs to be
> applied as well.

We just recently added syscall-specific macros to libbpf and fixed
this issue for x86-64. It would be great if people familiar with other
architectures contribute fixes for other architectures. Thanks!

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

[Naveen N. Rao]

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.

I will submit a patch to use orig_gpr3 on powerpc.


Thanks!
- Naveen

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

[Naveen N. Rao]

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

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

[Ilya Leoshkevich]

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.

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

[Naveen N. Rao]

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
> 

[PATCH bpf-next 2/3] selftests/bpf: Fix an endianness issue in bpf_syscall_macro test

[Ilya Leoshkevich]

bpf_syscall_macro reads a long argument into an int variable, which
produces a wrong value on big-endian systems. Fix by reading the
argument into an intermediate long variable first.

Fixes: 77fc0330dfe5 ("selftests/bpf: Add a test to confirm PT_REGS_PARM4_SYSCALL")
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/bpf/progs/bpf_syscall_macro.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c b/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c
index c8e60220cda8..f5c6ef2ff6d1 100644
--- a/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c
+++ b/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c
@@ -28,6 +28,7 @@ int BPF_KPROBE(handle_sys_prctl)
 {
 	struct pt_regs *real_regs;
 	pid_t pid = bpf_get_current_pid_tgid() >> 32;
+	unsigned long tmp;
 
 	if (pid != filter_pid)
 		return 0;
@@ -35,7 +36,9 @@ int BPF_KPROBE(handle_sys_prctl)
 	real_regs = (struct pt_regs *)PT_REGS_PARM1(ctx);
 
 	/* test for PT_REGS_PARM */
-	bpf_probe_read_kernel(&arg1, sizeof(arg1), &PT_REGS_PARM1_SYSCALL(real_regs));
+
+	bpf_probe_read_kernel(&tmp, sizeof(tmp), &PT_REGS_PARM1_SYSCALL(real_regs));
+	arg1 = tmp;
 	bpf_probe_read_kernel(&arg2, sizeof(arg2), &PT_REGS_PARM2_SYSCALL(real_regs));
 	bpf_probe_read_kernel(&arg3, sizeof(arg3), &PT_REGS_PARM3_SYSCALL(real_regs));
 	bpf_probe_read_kernel(&arg4_cx, sizeof(arg4_cx), &PT_REGS_PARM4(real_regs));
-- 
2.34.1

[PATCH bpf-next 3/3] libbpf: Fix accessing the first syscall argument on s390

[Ilya Leoshkevich]

On s390, the first syscall argument should be accessed via orig_gpr2
(see arch/s390/include/asm/syscall.h). Currently gpr[2] is used
instead, leading to bpf_syscall_macro test failure. Fix by adding
__PT_PARM1_REG_SYSCALL macro, similar to the existing
__PT_PARM4_REG_SYSCALL.

Fixes: d084df3b7a4c ("libbpf: Fix the incorrect register read for syscalls on x86_64")
Reported-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/lib/bpf/bpf_tracing.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
index 032ba809f3e5..9d6ff24c7abd 100644
--- a/tools/lib/bpf/bpf_tracing.h
+++ b/tools/lib/bpf/bpf_tracing.h
@@ -117,6 +117,7 @@
 /* s390 provides user_pt_regs instead of struct pt_regs to userspace */
 #define __PT_REGS_CAST(x) ((const user_pt_regs *)(x))
 #define __PT_PARM1_REG gprs[2]
+#define __PT_PARM1_REG_SYSCALL orig_gpr2
 #define __PT_PARM2_REG gprs[3]
 #define __PT_PARM3_REG gprs[4]
 #define __PT_PARM4_REG gprs[5]
@@ -265,7 +266,11 @@ struct pt_regs;
 
 #endif
 
+#ifdef __PT_PARM1_REG_SYSCALL
+#define PT_REGS_PARM1_SYSCALL(x) (__PT_REGS_CAST(x)->__PT_PARM1_REG_SYSCALL)
+#else /* __PT_PARM1_REG_SYSCALL */
 #define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x)
+#endif
 #define PT_REGS_PARM2_SYSCALL(x) PT_REGS_PARM2(x)
 #define PT_REGS_PARM3_SYSCALL(x) PT_REGS_PARM3(x)
 #ifdef __PT_PARM4_REG_SYSCALL
@@ -275,7 +280,11 @@ struct pt_regs;
 #endif
 #define PT_REGS_PARM5_SYSCALL(x) PT_REGS_PARM5(x)
 
+#ifdef __PT_PARM1_REG_SYSCALL
+#define PT_REGS_PARM1_CORE_SYSCALL(x) BPF_CORE_READ(__PT_REGS_CAST(x), __PT_PARM1_REG_SYSCALL)
+#else /* __PT_PARM1_REG_SYSCALL */
 #define PT_REGS_PARM1_CORE_SYSCALL(x) PT_REGS_PARM1_CORE(x)
+#endif
 #define PT_REGS_PARM2_CORE_SYSCALL(x) PT_REGS_PARM2_CORE(x)
 #define PT_REGS_PARM3_CORE_SYSCALL(x) PT_REGS_PARM3_CORE(x)
 #ifdef __PT_PARM4_REG_SYSCALL
-- 
2.34.1