| Message ID | 20220208051635.2160304-9-iii@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Fix accessing syscall arguments | expand |
On Mon, Feb 7, 2022 at 9:16 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote: > > Andrii says: "... with CO-RE and vmlinux.h it would be more reliable > and straightforward to just stick to kernel-internal struct pt_regs > everywhere ...". > > Actually, if vmlinux.h is available, then it's ok to do so for both > CO-RE and non-CO-RE cases, since the beginning of struct pt_regs must > match (struct) user_pt_regs, which must never change. > > Implement this by not defining __PT_REGS_CAST if the user included > vmlinux.h. > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > --- If we are using CO-RE we don't have to assume vmlinux.h, we can define our own definition of pt_regs with custom "flavor": struct pt_regs___s390x { long gprs[10]; long orig_gpr2; /* whatever the right types and names, but order doesn't matter */ } __attribute__((preserve_access_index)); And then use `struct pt_regs__s390x` for s390x macros. That way we don't assume any specific included header, we have minimal definition we need (and it can be different for each architecture. It's still CO-RE, still relocatable, and we don't need all these ugly #if defined() checks. > 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 7a015ee8fb11..07e291d77e83 100644 > --- a/tools/lib/bpf/bpf_tracing.h > +++ b/tools/lib/bpf/bpf_tracing.h > @@ -118,8 +118,11 @@ > > #define __BPF_ARCH_HAS_SYSCALL_WRAPPER > > +#if !defined(__KERNEL__) && !defined(__VMLINUX_H__) > /* s390 provides user_pt_regs instead of struct pt_regs to userspace */ > #define __PT_REGS_CAST(x) ((const user_pt_regs *)(x)) > +#endif > + > #define __PT_PARM1_REG gprs[2] > #define __PT_PARM2_REG gprs[3] > #define __PT_PARM3_REG gprs[4] > @@ -148,8 +151,11 @@ > > #define __BPF_ARCH_HAS_SYSCALL_WRAPPER > > +#if !defined(__KERNEL__) && !defined(__VMLINUX_H__) > /* arm64 provides struct user_pt_regs instead of struct pt_regs to userspace */ > #define __PT_REGS_CAST(x) ((const struct user_pt_regs *)(x)) > +#endif > + > #define __PT_PARM1_REG regs[0] > #define __PT_PARM2_REG regs[1] > #define __PT_PARM3_REG regs[2] > @@ -207,7 +213,10 @@ > > #elif defined(bpf_target_riscv) > > +#if !defined(__KERNEL__) && !defined(__VMLINUX_H__) > #define __PT_REGS_CAST(x) ((const struct user_regs_struct *)(x)) > +#endif > + > #define __PT_PARM1_REG a0 > #define __PT_PARM2_REG a1 > #define __PT_PARM3_REG a2 > -- > 2.34.1 >
On Tue, 2022-02-08 at 14:12 -0800, Andrii Nakryiko wrote: > On Mon, Feb 7, 2022 at 9:16 PM Ilya Leoshkevich <iii@linux.ibm.com> > wrote: > > > > Andrii says: "... with CO-RE and vmlinux.h it would be more > > reliable > > and straightforward to just stick to kernel-internal struct pt_regs > > everywhere ...". > > > > Actually, if vmlinux.h is available, then it's ok to do so for both > > CO-RE and non-CO-RE cases, since the beginning of struct pt_regs > > must > > match (struct) user_pt_regs, which must never change. > > > > Implement this by not defining __PT_REGS_CAST if the user included > > vmlinux.h. > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > --- > > If we are using CO-RE we don't have to assume vmlinux.h, we can > define > our own definition of pt_regs with custom "flavor": > > struct pt_regs___s390x { > long gprs[10]; > long orig_gpr2; /* whatever the right types and names, but order > doesn't matter */ > } __attribute__((preserve_access_index)); > > > And then use `struct pt_regs__s390x` for s390x macros. That way we > don't assume any specific included header, we have minimal definition > we need (and it can be different for each architecture. It's still > CO-RE, still relocatable, and we don't need all these ugly #if > defined() checks. I haven't considered using flavors - this will indeed improve the CO-RE side of things, thanks! > > > 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 7a015ee8fb11..07e291d77e83 100644 > > --- a/tools/lib/bpf/bpf_tracing.h > > +++ b/tools/lib/bpf/bpf_tracing.h > > @@ -118,8 +118,11 @@ > > > > #define __BPF_ARCH_HAS_SYSCALL_WRAPPER > > > > +#if !defined(__KERNEL__) && !defined(__VMLINUX_H__) > > /* s390 provides user_pt_regs instead of struct pt_regs to > > userspace */ > > #define __PT_REGS_CAST(x) ((const user_pt_regs *)(x)) > > +#endif > > + > > #define __PT_PARM1_REG gprs[2] > > #define __PT_PARM2_REG gprs[3] > > #define __PT_PARM3_REG gprs[4] > > @@ -148,8 +151,11 @@ > > > > #define __BPF_ARCH_HAS_SYSCALL_WRAPPER > > > > +#if !defined(__KERNEL__) && !defined(__VMLINUX_H__) > > /* arm64 provides struct user_pt_regs instead of struct pt_regs to > > userspace */ > > #define __PT_REGS_CAST(x) ((const struct user_pt_regs *)(x)) > > +#endif > > + > > #define __PT_PARM1_REG regs[0] > > #define __PT_PARM2_REG regs[1] > > #define __PT_PARM3_REG regs[2] > > @@ -207,7 +213,10 @@ > > > > #elif defined(bpf_target_riscv) > > > > +#if !defined(__KERNEL__) && !defined(__VMLINUX_H__) > > #define __PT_REGS_CAST(x) ((const struct user_regs_struct *)(x)) > > +#endif > > + > > #define __PT_PARM1_REG a0 > > #define __PT_PARM2_REG a1 > > #define __PT_PARM3_REG a2 > > -- > > 2.34.1 > >
diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h index 7a015ee8fb11..07e291d77e83 100644 --- a/tools/lib/bpf/bpf_tracing.h +++ b/tools/lib/bpf/bpf_tracing.h @@ -118,8 +118,11 @@ #define __BPF_ARCH_HAS_SYSCALL_WRAPPER +#if !defined(__KERNEL__) && !defined(__VMLINUX_H__) /* s390 provides user_pt_regs instead of struct pt_regs to userspace */ #define __PT_REGS_CAST(x) ((const user_pt_regs *)(x)) +#endif + #define __PT_PARM1_REG gprs[2] #define __PT_PARM2_REG gprs[3] #define __PT_PARM3_REG gprs[4] @@ -148,8 +151,11 @@ #define __BPF_ARCH_HAS_SYSCALL_WRAPPER +#if !defined(__KERNEL__) && !defined(__VMLINUX_H__) /* arm64 provides struct user_pt_regs instead of struct pt_regs to userspace */ #define __PT_REGS_CAST(x) ((const struct user_pt_regs *)(x)) +#endif + #define __PT_PARM1_REG regs[0] #define __PT_PARM2_REG regs[1] #define __PT_PARM3_REG regs[2] @@ -207,7 +213,10 @@ #elif defined(bpf_target_riscv) +#if !defined(__KERNEL__) && !defined(__VMLINUX_H__) #define __PT_REGS_CAST(x) ((const struct user_regs_struct *)(x)) +#endif + #define __PT_PARM1_REG a0 #define __PT_PARM2_REG a1 #define __PT_PARM3_REG a2
Andrii says: "... with CO-RE and vmlinux.h it would be more reliable and straightforward to just stick to kernel-internal struct pt_regs everywhere ...". Actually, if vmlinux.h is available, then it's ok to do so for both CO-RE and non-CO-RE cases, since the beginning of struct pt_regs must match (struct) user_pt_regs, which must never change. Implement this by not defining __PT_REGS_CAST if the user included vmlinux.h. Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> --- tools/lib/bpf/bpf_tracing.h | 9 +++++++++ 1 file changed, 9 insertions(+)