diff mbox series

[bpf-next,v4,06/14] libbpf: Add PT_REGS_SYSCALL_REGS macro

Message ID 20220208051635.2160304-7-iii@linux.ibm.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series Fix accessing syscall arguments | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR fail PR summary
netdev/tree_selection success Clearly marked for bpf-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 7 maintainers not CCed: andrii@kernel.org kpsingh@kernel.org john.fastabend@gmail.com kafai@fb.com songliubraving@fb.com yhs@fb.com netdev@vger.kernel.org
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 41 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next fail VM_Test

Commit Message

Ilya Leoshkevich Feb. 8, 2022, 5:16 a.m. UTC 
Depending on whether or not an arch has ARCH_HAS_SYSCALL_WRAPPER,
syscall arguments must be accessed through a different set of
registers. Provide PT_REGS_SYSCALL_REGS macro to abstract away
that difference.

Reported-by: Heiko Carstens <hca@linux.ibm.com>
Co-developed-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/lib/bpf/bpf_tracing.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Comments

Andrii Nakryiko Feb. 8, 2022, 10:08 p.m. UTC | #1 
On Mon, Feb 7, 2022 at 9:16 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> Depending on whether or not an arch has ARCH_HAS_SYSCALL_WRAPPER,
> syscall arguments must be accessed through a different set of
> registers. Provide PT_REGS_SYSCALL_REGS macro to abstract away
> that difference.
>
> Reported-by: Heiko Carstens <hca@linux.ibm.com>
> Co-developed-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---

Again, there was nothing wrong with the way you did it in v3, please
revert to that one.

>  tools/lib/bpf/bpf_tracing.h | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
> index 82f1e935d549..7a015ee8fb11 100644
> --- a/tools/lib/bpf/bpf_tracing.h
> +++ b/tools/lib/bpf/bpf_tracing.h
> @@ -64,6 +64,8 @@
>
>  #if defined(bpf_target_x86)
>
> +#define __BPF_ARCH_HAS_SYSCALL_WRAPPER
> +
>  #if defined(__KERNEL__) || defined(__VMLINUX_H__)
>
>  #define __PT_PARM1_REG di
> @@ -114,6 +116,8 @@
>
>  #elif defined(bpf_target_s390)
>
> +#define __BPF_ARCH_HAS_SYSCALL_WRAPPER
> +
>  /* 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]
> @@ -142,6 +146,8 @@
>
>  #elif defined(bpf_target_arm64)
>
> +#define __BPF_ARCH_HAS_SYSCALL_WRAPPER
> +
>  /* arm64 provides struct user_pt_regs instead of struct pt_regs to userspace */
>  #define __PT_REGS_CAST(x) ((const struct user_pt_regs *)(x))
>  #define __PT_PARM1_REG regs[0]
> @@ -344,6 +350,17 @@ struct pt_regs;
>
>  #endif /* defined(bpf_target_defined) */
>
> +/*
> + * When invoked from a syscall handler BPF_KPROBE, returns a pointer to a
> + * struct pt_regs containing syscall arguments, that is suitable for passing to
> + * PT_REGS_PARMn_SYSCALL() and PT_REGS_PARMn_CORE_SYSCALL().
> + */
> +#ifdef __BPF_ARCH_HAS_SYSCALL_WRAPPER
> +#define PT_REGS_SYSCALL_REGS(ctx) ((struct pt_regs *)PT_REGS_PARM1(ctx))
> +#else
> +#define PT_REGS_SYSCALL_REGS(ctx) ctx
> +#endif
> +
>  #ifndef ___bpf_concat
>  #define ___bpf_concat(a, b) a ## b
>  #endif
> --
> 2.34.1
>
Ilya Leoshkevich Feb. 8, 2022, 11:26 p.m. UTC | #2 
On Tue, 2022-02-08 at 14:08 -0800, Andrii Nakryiko wrote:
> On Mon, Feb 7, 2022 at 9:16 PM Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
> > 
> > Depending on whether or not an arch has ARCH_HAS_SYSCALL_WRAPPER,
> > syscall arguments must be accessed through a different set of
> > registers. Provide PT_REGS_SYSCALL_REGS macro to abstract away
> > that difference.
> > 
> > Reported-by: Heiko Carstens <hca@linux.ibm.com>
> > Co-developed-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> 
> Again, there was nothing wrong with the way you did it in v3, please
> revert to that one.

I've realized that, even though fully correct, v3 looked somewhat
ad-hoc: it defined PT_REGS_SYSCALL_REGS for different architectures
without explaining why this particular arch has this parciular way to
access syscall arguments.

So I've decided to switch to the existing terminology, as Naveen
proposed [1]:

- arches that select ARCH_HAS_SYSCALL_WRAPPER in Kconfig get a
  __BPF_ARCH_HAS_SYSCALL_WRAPPER in bpf_tracing.h

- syscall handler calling convention is (at least partially) determined
  by whether or not an arch has a sycall wrapper as described in
  ARCH_HAS_SYSCALL_WRAPPER help text

I can, of course, switch back to v3 - both patches look functionally
identical - but this one seems to be a bit easier to understand.

[1]
https://lore.kernel.org/bpf/1643991537.bfyv1b2oym.naveen@linux.ibm.com/#t

> 
> >  tools/lib/bpf/bpf_tracing.h | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/tools/lib/bpf/bpf_tracing.h
> > b/tools/lib/bpf/bpf_tracing.h
> > index 82f1e935d549..7a015ee8fb11 100644
> > --- a/tools/lib/bpf/bpf_tracing.h
> > +++ b/tools/lib/bpf/bpf_tracing.h
> > @@ -64,6 +64,8 @@
> > 
> >  #if defined(bpf_target_x86)
> > 
> > +#define __BPF_ARCH_HAS_SYSCALL_WRAPPER
> > +
> >  #if defined(__KERNEL__) || defined(__VMLINUX_H__)
> > 
> >  #define __PT_PARM1_REG di
> > @@ -114,6 +116,8 @@
> > 
> >  #elif defined(bpf_target_s390)
> > 
> > +#define __BPF_ARCH_HAS_SYSCALL_WRAPPER
> > +
> >  /* 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]
> > @@ -142,6 +146,8 @@
> > 
> >  #elif defined(bpf_target_arm64)
> > 
> > +#define __BPF_ARCH_HAS_SYSCALL_WRAPPER
> > +
> >  /* arm64 provides struct user_pt_regs instead of struct pt_regs to
> > userspace */
> >  #define __PT_REGS_CAST(x) ((const struct user_pt_regs *)(x))
> >  #define __PT_PARM1_REG regs[0]
> > @@ -344,6 +350,17 @@ struct pt_regs;
> > 
> >  #endif /* defined(bpf_target_defined) */
> > 
> > +/*
> > + * When invoked from a syscall handler BPF_KPROBE, returns a
> > pointer to a
> > + * struct pt_regs containing syscall arguments, that is suitable
> > for passing to
> > + * PT_REGS_PARMn_SYSCALL() and PT_REGS_PARMn_CORE_SYSCALL().
> > + */
> > +#ifdef __BPF_ARCH_HAS_SYSCALL_WRAPPER
> > +#define PT_REGS_SYSCALL_REGS(ctx) ((struct pt_regs
> > *)PT_REGS_PARM1(ctx))
> > +#else
> > +#define PT_REGS_SYSCALL_REGS(ctx) ctx
> > +#endif
> > +
> >  #ifndef ___bpf_concat
> >  #define ___bpf_concat(a, b) a ## b
> >  #endif
> > --
> > 2.34.1
> >
Andrii Nakryiko Feb. 8, 2022, 11:43 p.m. UTC | #3 
On Tue, Feb 8, 2022 at 3:26 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> On Tue, 2022-02-08 at 14:08 -0800, Andrii Nakryiko wrote:
> > On Mon, Feb 7, 2022 at 9:16 PM Ilya Leoshkevich <iii@linux.ibm.com>
> > wrote:
> > >
> > > Depending on whether or not an arch has ARCH_HAS_SYSCALL_WRAPPER,
> > > syscall arguments must be accessed through a different set of
> > > registers. Provide PT_REGS_SYSCALL_REGS macro to abstract away
> > > that difference.
> > >
> > > Reported-by: Heiko Carstens <hca@linux.ibm.com>
> > > Co-developed-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > ---
> >
> > Again, there was nothing wrong with the way you did it in v3, please
> > revert to that one.
>
> I've realized that, even though fully correct, v3 looked somewhat
> ad-hoc: it defined PT_REGS_SYSCALL_REGS for different architectures
> without explaining why this particular arch has this parciular way to
> access syscall arguments.
>
> So I've decided to switch to the existing terminology, as Naveen
> proposed [1]:
>
> - arches that select ARCH_HAS_SYSCALL_WRAPPER in Kconfig get a
>   __BPF_ARCH_HAS_SYSCALL_WRAPPER in bpf_tracing.h
>
> - syscall handler calling convention is (at least partially) determined
>   by whether or not an arch has a sycall wrapper as described in
>   ARCH_HAS_SYSCALL_WRAPPER help text
>
> I can, of course, switch back to v3 - both patches look functionally
> identical - but this one seems to be a bit easier to understand.
>
> [1]
> https://lore.kernel.org/bpf/1643991537.bfyv1b2oym.naveen@linux.ibm.com/#t
>
> >
> > >  tools/lib/bpf/bpf_tracing.h | 17 +++++++++++++++++
> > >  1 file changed, 17 insertions(+)
> > >
> > > diff --git a/tools/lib/bpf/bpf_tracing.h
> > > b/tools/lib/bpf/bpf_tracing.h
> > > index 82f1e935d549..7a015ee8fb11 100644
> > > --- a/tools/lib/bpf/bpf_tracing.h
> > > +++ b/tools/lib/bpf/bpf_tracing.h
> > > @@ -64,6 +64,8 @@
> > >
> > >  #if defined(bpf_target_x86)
> > >
> > > +#define __BPF_ARCH_HAS_SYSCALL_WRAPPER
> > > +
> > >  #if defined(__KERNEL__) || defined(__VMLINUX_H__)
> > >
> > >  #define __PT_PARM1_REG di
> > > @@ -114,6 +116,8 @@
> > >
> > >  #elif defined(bpf_target_s390)
> > >
> > > +#define __BPF_ARCH_HAS_SYSCALL_WRAPPER
> > > +
> > >  /* 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]
> > > @@ -142,6 +146,8 @@
> > >
> > >  #elif defined(bpf_target_arm64)
> > >
> > > +#define __BPF_ARCH_HAS_SYSCALL_WRAPPER
> > > +
> > >  /* arm64 provides struct user_pt_regs instead of struct pt_regs to
> > > userspace */
> > >  #define __PT_REGS_CAST(x) ((const struct user_pt_regs *)(x))
> > >  #define __PT_PARM1_REG regs[0]
> > > @@ -344,6 +350,17 @@ struct pt_regs;
> > >
> > >  #endif /* defined(bpf_target_defined) */
> > >
> > > +/*
> > > + * When invoked from a syscall handler BPF_KPROBE, returns a
> > > pointer to a
> > > + * struct pt_regs containing syscall arguments, that is suitable
> > > for passing to
> > > + * PT_REGS_PARMn_SYSCALL() and PT_REGS_PARMn_CORE_SYSCALL().

You can mention ARCH_HAS_SYSCALL_WRAPPER here for documentation
purposes. I like the previous approach because it clearly shows which
architectures deviate from "common" behavior (whatever "common" we
chose as the default). With __BPF_ARCH_HAS_SYSCALL_WRAPPER I'll go and
start grepping what else depends on that, etc. Also,
ARCH_HAS_SYSCALL_WRAPPER can change over time, so it depends on kernel
version just as much as architecture (which with CO-RE we should be
able to handle transparently, btw).

Anyways, the previous one looks cleaner and easier to follow to me,
please use the previous version.

> > > + */
> > > +#ifdef __BPF_ARCH_HAS_SYSCALL_WRAPPER
> > > +#define PT_REGS_SYSCALL_REGS(ctx) ((struct pt_regs
> > > *)PT_REGS_PARM1(ctx))
> > > +#else
> > > +#define PT_REGS_SYSCALL_REGS(ctx) ctx
> > > +#endif
> > > +
> > >  #ifndef ___bpf_concat
> > >  #define ___bpf_concat(a, b) a ## b
> > >  #endif
> > > --
> > > 2.34.1
> > >
>
diff mbox series

Patch

diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
index 82f1e935d549..7a015ee8fb11 100644
--- a/tools/lib/bpf/bpf_tracing.h
+++ b/tools/lib/bpf/bpf_tracing.h
@@ -64,6 +64,8 @@ 
 
 #if defined(bpf_target_x86)
 
+#define __BPF_ARCH_HAS_SYSCALL_WRAPPER
+
 #if defined(__KERNEL__) || defined(__VMLINUX_H__)
 
 #define __PT_PARM1_REG di
@@ -114,6 +116,8 @@ 
 
 #elif defined(bpf_target_s390)
 
+#define __BPF_ARCH_HAS_SYSCALL_WRAPPER
+
 /* 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]
@@ -142,6 +146,8 @@ 
 
 #elif defined(bpf_target_arm64)
 
+#define __BPF_ARCH_HAS_SYSCALL_WRAPPER
+
 /* arm64 provides struct user_pt_regs instead of struct pt_regs to userspace */
 #define __PT_REGS_CAST(x) ((const struct user_pt_regs *)(x))
 #define __PT_PARM1_REG regs[0]
@@ -344,6 +350,17 @@  struct pt_regs;
 
 #endif /* defined(bpf_target_defined) */
 
+/*
+ * When invoked from a syscall handler BPF_KPROBE, returns a pointer to a
+ * struct pt_regs containing syscall arguments, that is suitable for passing to
+ * PT_REGS_PARMn_SYSCALL() and PT_REGS_PARMn_CORE_SYSCALL().
+ */
+#ifdef __BPF_ARCH_HAS_SYSCALL_WRAPPER
+#define PT_REGS_SYSCALL_REGS(ctx) ((struct pt_regs *)PT_REGS_PARM1(ctx))
+#else
+#define PT_REGS_SYSCALL_REGS(ctx) ctx
+#endif
+
 #ifndef ___bpf_concat
 #define ___bpf_concat(a, b) a ## b
 #endif