* [PATCH bpf-next 0/3] libbpf: Fix accessing the first syscall argument on s390
@ 2022-02-01 23:41 Ilya Leoshkevich
2022-02-01 23:41 ` [PATCH bpf-next 1/3] s390/bpf: Add orig_gpr2 to user_pt_regs Ilya Leoshkevich
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Ilya Leoshkevich @ 2022-02-01 23:41 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
Alexander Gordeev
Cc: bpf, 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH bpf-next 1/3] s390/bpf: Add orig_gpr2 to user_pt_regs
2022-02-01 23:41 [PATCH bpf-next 0/3] libbpf: Fix accessing the first syscall argument on s390 Ilya Leoshkevich
@ 2022-02-01 23:41 ` Ilya Leoshkevich
2022-02-02 14:19 ` Vasily Gorbik
2022-02-02 20:14 ` Heiko Carstens
2022-02-01 23:41 ` [PATCH bpf-next 2/3] selftests/bpf: Fix an endianness issue in bpf_syscall_macro test Ilya Leoshkevich
2022-02-01 23:42 ` [PATCH bpf-next 3/3] libbpf: Fix accessing the first syscall argument on s390 Ilya Leoshkevich
2 siblings, 2 replies; 13+ messages in thread
From: Ilya Leoshkevich @ 2022-02-01 23:41 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
Alexander Gordeev
Cc: bpf, 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH bpf-next 2/3] selftests/bpf: Fix an endianness issue in bpf_syscall_macro test
2022-02-01 23:41 [PATCH bpf-next 0/3] libbpf: Fix accessing the first syscall argument on s390 Ilya Leoshkevich
2022-02-01 23:41 ` [PATCH bpf-next 1/3] s390/bpf: Add orig_gpr2 to user_pt_regs Ilya Leoshkevich
@ 2022-02-01 23:41 ` Ilya Leoshkevich
2022-02-01 23:42 ` [PATCH bpf-next 3/3] libbpf: Fix accessing the first syscall argument on s390 Ilya Leoshkevich
2 siblings, 0 replies; 13+ messages in thread
From: Ilya Leoshkevich @ 2022-02-01 23:41 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
Alexander Gordeev
Cc: bpf, 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH bpf-next 3/3] libbpf: Fix accessing the first syscall argument on s390
2022-02-01 23:41 [PATCH bpf-next 0/3] libbpf: Fix accessing the first syscall argument on s390 Ilya Leoshkevich
2022-02-01 23:41 ` [PATCH bpf-next 1/3] s390/bpf: Add orig_gpr2 to user_pt_regs Ilya Leoshkevich
2022-02-01 23:41 ` [PATCH bpf-next 2/3] selftests/bpf: Fix an endianness issue in bpf_syscall_macro test Ilya Leoshkevich
@ 2022-02-01 23:42 ` Ilya Leoshkevich
2 siblings, 0 replies; 13+ messages in thread
From: Ilya Leoshkevich @ 2022-02-01 23:42 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
Alexander Gordeev
Cc: bpf, Ilya Leoshkevich, Andrii Nakryiko
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 1/3] s390/bpf: Add orig_gpr2 to user_pt_regs
2022-02-01 23:41 ` [PATCH bpf-next 1/3] s390/bpf: Add orig_gpr2 to user_pt_regs Ilya Leoshkevich
@ 2022-02-02 14:19 ` Vasily Gorbik
2022-02-02 17:23 ` Christian Borntraeger
2022-02-02 20:14 ` Heiko Carstens
1 sibling, 1 reply; 13+ messages in thread
From: Vasily Gorbik @ 2022-02-02 14:19 UTC (permalink / raw)
To: Ilya Leoshkevich
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Heiko Carstens, Christian Borntraeger, Alexander Gordeev, bpf,
Sven Schnelle
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?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 1/3] s390/bpf: Add orig_gpr2 to user_pt_regs
2022-02-02 14:19 ` Vasily Gorbik
@ 2022-02-02 17:23 ` Christian Borntraeger
2022-02-03 9:40 ` Heiko Carstens
0 siblings, 1 reply; 13+ messages in thread
From: Christian Borntraeger @ 2022-02-02 17:23 UTC (permalink / raw)
To: Vasily Gorbik, Ilya Leoshkevich
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Heiko Carstens, Alexander Gordeev, bpf, Sven Schnelle
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 1/3] s390/bpf: Add orig_gpr2 to user_pt_regs
2022-02-01 23:41 ` [PATCH bpf-next 1/3] s390/bpf: Add orig_gpr2 to user_pt_regs Ilya Leoshkevich
2022-02-02 14:19 ` Vasily Gorbik
@ 2022-02-02 20:14 ` Heiko Carstens
2022-02-02 22:49 ` Andrii Nakryiko
2022-02-04 6:07 ` Naveen N. Rao
1 sibling, 2 replies; 13+ messages in thread
From: Heiko Carstens @ 2022-02-02 20:14 UTC (permalink / raw)
To: Ilya Leoshkevich
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Vasily Gorbik, Christian Borntraeger, Alexander Gordeev, bpf,
Naveen N. Rao, Björn Töpel, Luke Nelson,
Michael Ellerman, Paul Walmsley, Palmer Dabbelt, Catalin Marinas,
Will Deacon
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 1/3] s390/bpf: Add orig_gpr2 to user_pt_regs
2022-02-02 20:14 ` Heiko Carstens
@ 2022-02-02 22:49 ` Andrii Nakryiko
2022-02-04 6:07 ` Naveen N. Rao
1 sibling, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2022-02-02 22:49 UTC (permalink / raw)
To: Heiko Carstens
Cc: Ilya Leoshkevich, Alexei Starovoitov, Daniel Borkmann,
Vasily Gorbik, Christian Borntraeger, Alexander Gordeev, bpf,
Naveen N. Rao, Björn Töpel, Luke Nelson,
Michael Ellerman, Paul Walmsley, Palmer Dabbelt, Catalin Marinas,
Will Deacon
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!
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 1/3] s390/bpf: Add orig_gpr2 to user_pt_regs
2022-02-02 17:23 ` Christian Borntraeger
@ 2022-02-03 9:40 ` Heiko Carstens
0 siblings, 0 replies; 13+ messages in thread
From: Heiko Carstens @ 2022-02-03 9:40 UTC (permalink / raw)
To: Christian Borntraeger
Cc: Vasily Gorbik, Ilya Leoshkevich, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Alexander Gordeev, bpf,
Sven Schnelle
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?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 1/3] s390/bpf: Add orig_gpr2 to user_pt_regs
2022-02-02 20:14 ` Heiko Carstens
2022-02-02 22:49 ` Andrii Nakryiko
@ 2022-02-04 6:07 ` Naveen N. Rao
2022-02-04 8:21 ` Naveen N. Rao
1 sibling, 1 reply; 13+ messages in thread
From: Naveen N. Rao @ 2022-02-04 6:07 UTC (permalink / raw)
To: Heiko Carstens, Ilya Leoshkevich
Cc: Alexander Gordeev, Andrii Nakryiko, Alexei Starovoitov,
Björn Töpel, Christian Borntraeger, bpf,
Catalin Marinas, Daniel Borkmann, Vasily Gorbik, Luke Nelson,
Michael Ellerman, Palmer Dabbelt, Paul Walmsley, Will Deacon
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 1/3] s390/bpf: Add orig_gpr2 to user_pt_regs
2022-02-04 6:07 ` Naveen N. Rao
@ 2022-02-04 8:21 ` Naveen N. Rao
2022-02-04 12:20 ` Ilya Leoshkevich
0 siblings, 1 reply; 13+ messages in thread
From: Naveen N. Rao @ 2022-02-04 8:21 UTC (permalink / raw)
To: Heiko Carstens, Ilya Leoshkevich
Cc: Alexander Gordeev, Andrii Nakryiko, Alexei Starovoitov,
Björn Töpel, Christian Borntraeger, bpf,
Catalin Marinas, Daniel Borkmann, Vasily Gorbik, Luke Nelson,
Michael Ellerman, Palmer Dabbelt, Paul Walmsley, Will Deacon
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 1/3] s390/bpf: Add orig_gpr2 to user_pt_regs
2022-02-04 8:21 ` Naveen N. Rao
@ 2022-02-04 12:20 ` Ilya Leoshkevich
2022-02-04 13:49 ` Naveen N. Rao
0 siblings, 1 reply; 13+ messages in thread
From: Ilya Leoshkevich @ 2022-02-04 12:20 UTC (permalink / raw)
To: Naveen N. Rao, Heiko Carstens
Cc: Alexander Gordeev, Andrii Nakryiko, Alexei Starovoitov,
Björn Töpel, Christian Borntraeger, bpf,
Catalin Marinas, Daniel Borkmann, Vasily Gorbik, Luke Nelson,
Michael Ellerman, Palmer Dabbelt, Paul Walmsley, Will Deacon
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 1/3] s390/bpf: Add orig_gpr2 to user_pt_regs
2022-02-04 12:20 ` Ilya Leoshkevich
@ 2022-02-04 13:49 ` Naveen N. Rao
0 siblings, 0 replies; 13+ messages in thread
From: Naveen N. Rao @ 2022-02-04 13:49 UTC (permalink / raw)
To: Heiko Carstens, Ilya Leoshkevich
Cc: Alexander Gordeev, Andrii Nakryiko, Alexei Starovoitov,
Björn Töpel, Christian Borntraeger, bpf,
Catalin Marinas, Daniel Borkmann, Vasily Gorbik, Luke Nelson,
Michael Ellerman, Palmer Dabbelt, Paul Walmsley, Will Deacon
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
>
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-02-04 13:50 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-01 23:41 [PATCH bpf-next 0/3] libbpf: Fix accessing the first syscall argument on s390 Ilya Leoshkevich
2022-02-01 23:41 ` [PATCH bpf-next 1/3] s390/bpf: Add orig_gpr2 to user_pt_regs Ilya Leoshkevich
2022-02-02 14:19 ` Vasily Gorbik
2022-02-02 17:23 ` Christian Borntraeger
2022-02-03 9:40 ` Heiko Carstens
2022-02-02 20:14 ` Heiko Carstens
2022-02-02 22:49 ` Andrii Nakryiko
2022-02-04 6:07 ` Naveen N. Rao
2022-02-04 8:21 ` Naveen N. Rao
2022-02-04 12:20 ` Ilya Leoshkevich
2022-02-04 13:49 ` Naveen N. Rao
2022-02-01 23:41 ` [PATCH bpf-next 2/3] selftests/bpf: Fix an endianness issue in bpf_syscall_macro test Ilya Leoshkevich
2022-02-01 23:42 ` [PATCH bpf-next 3/3] libbpf: Fix accessing the first syscall argument on s390 Ilya Leoshkevich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.