| Message ID | 20220201234200.1836443-2-iii@linux.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | libbpf: Fix accessing the first syscall argument on s390 | expand |
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?
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.
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.
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!
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?
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
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
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.
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 >
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; /*
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(-)