diff mbox series

[bpf-next] selftests/bpf: Fix build of task_pt_regs tests for arm64

Message ID 20210902090925.2010528-1-jean-philippe@linaro.org (mailing list archive)
State New
Headers show
Series [bpf-next] selftests/bpf: Fix build of task_pt_regs tests for arm64 | expand

Commit Message

Jean-Philippe Brucker Sept. 2, 2021, 9:09 a.m. UTC 
struct pt_regs is not exported to userspace on all archs. arm64 and s390
export "user_pt_regs" instead, which causes build failure at the moment:

  progs/test_task_pt_regs.c:8:16: error: variable has incomplete type 'struct pt_regs'
  struct pt_regs current_regs = {};

Use the multi-arch macros defined by tools/lib/bpf/bpf_tracing.h to copy
the pt_regs into a locally-defined struct.

Copying the user_pt_regs struct on arm64 wouldn't work because the
struct is too large and the compiler complains about using too much
stack.

Fixes: 576d47bb1a92 ("bpf: selftests: Add bpf_task_pt_regs() selftest")
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 .../selftests/bpf/bpf_pt_regs_helpers.h       | 30 +++++++++++++++++++
 .../selftests/bpf/prog_tests/task_pt_regs.c   |  1 +
 .../selftests/bpf/progs/test_task_pt_regs.c   | 10 ++++---
 3 files changed, 37 insertions(+), 4 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/bpf_pt_regs_helpers.h

Comments

Daniel Xu Sept. 2, 2021, 6:32 p.m. UTC | #1 
On Thu, Sep 02, 2021 at 11:09:26AM +0200, Jean-Philippe Brucker wrote:
> struct pt_regs is not exported to userspace on all archs. arm64 and s390
> export "user_pt_regs" instead, which causes build failure at the moment:
> 
>   progs/test_task_pt_regs.c:8:16: error: variable has incomplete type 'struct pt_regs'
>   struct pt_regs current_regs = {};
> 
> Use the multi-arch macros defined by tools/lib/bpf/bpf_tracing.h to copy
> the pt_regs into a locally-defined struct.
> 
> Copying the user_pt_regs struct on arm64 wouldn't work because the
> struct is too large and the compiler complains about using too much
> stack.
> 
> Fixes: 576d47bb1a92 ("bpf: selftests: Add bpf_task_pt_regs() selftest")
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  .../selftests/bpf/bpf_pt_regs_helpers.h       | 30 +++++++++++++++++++
>  .../selftests/bpf/prog_tests/task_pt_regs.c   |  1 +
>  .../selftests/bpf/progs/test_task_pt_regs.c   | 10 ++++---
>  3 files changed, 37 insertions(+), 4 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/bpf_pt_regs_helpers.h

Acked-by: Daniel Xu <dxu@dxuuu.xyz>

[...]
Alexei Starovoitov Sept. 2, 2021, 7:13 p.m. UTC | #2 
On Thu, Sep 2, 2021 at 2:08 AM Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> struct pt_regs is not exported to userspace on all archs. arm64 and s390
> export "user_pt_regs" instead, which causes build failure at the moment:
>
>   progs/test_task_pt_regs.c:8:16: error: variable has incomplete type 'struct pt_regs'
>   struct pt_regs current_regs = {};

Right, which is 'bpf_user_pt_regs_t'.
It's defined for all archs and arm64/s390/ppc/risv define it
differently from pt_regs.

>
> Use the multi-arch macros defined by tools/lib/bpf/bpf_tracing.h to copy
> the pt_regs into a locally-defined struct.
>
> Copying the user_pt_regs struct on arm64 wouldn't work because the
> struct is too large and the compiler complains about using too much
> stack.

That's a different issue.
I think the cleaner fix would be to make the test use
bpf_user_pt_regs_t instead.

> Fixes: 576d47bb1a92 ("bpf: selftests: Add bpf_task_pt_regs() selftest")
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  .../selftests/bpf/bpf_pt_regs_helpers.h       | 30 +++++++++++++++++++
>  .../selftests/bpf/prog_tests/task_pt_regs.c   |  1 +
>  .../selftests/bpf/progs/test_task_pt_regs.c   | 10 ++++---
>  3 files changed, 37 insertions(+), 4 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/bpf_pt_regs_helpers.h
>
> diff --git a/tools/testing/selftests/bpf/bpf_pt_regs_helpers.h b/tools/testing/selftests/bpf/bpf_pt_regs_helpers.h
> new file mode 100644
> index 000000000000..7531f4824ead
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/bpf_pt_regs_helpers.h
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __BPF_PT_REGS_HELPERS
> +#define __BPF_PT_REGS_HELPERS
> +
> +#include <bpf/bpf_tracing.h>
> +
> +struct bpf_pt_regs {
> +       unsigned long long parm[5];
> +       unsigned long long ret;
> +       unsigned long long fp;
> +       unsigned long long rc;
> +       unsigned long long sp;
> +       unsigned long long ip;
> +};
> +
> +static inline void bpf_copy_pt_regs(struct bpf_pt_regs *dest, struct pt_regs *src)
> +{
> +       dest->parm[0]   = PT_REGS_PARM1(src);
> +       dest->parm[1]   = PT_REGS_PARM2(src);
> +       dest->parm[2]   = PT_REGS_PARM3(src);
> +       dest->parm[3]   = PT_REGS_PARM4(src);
> +       dest->parm[4]   = PT_REGS_PARM5(src);
> +       dest->ret       = PT_REGS_RET(src);
> +       dest->fp        = PT_REGS_FP(src);
> +       dest->rc        = PT_REGS_RC(src);
> +       dest->sp        = PT_REGS_SP(src);
> +       dest->ip        = PT_REGS_IP(src);
> +}
> +
> +#endif /* __BPF_PT_REGS_HELPERS */
> diff --git a/tools/testing/selftests/bpf/prog_tests/task_pt_regs.c b/tools/testing/selftests/bpf/prog_tests/task_pt_regs.c
> index 53f0e0fa1a53..196453b75937 100644
> --- a/tools/testing/selftests/bpf/prog_tests/task_pt_regs.c
> +++ b/tools/testing/selftests/bpf/prog_tests/task_pt_regs.c
> @@ -2,6 +2,7 @@
>  #define _GNU_SOURCE
>  #include <test_progs.h>
>  #include <linux/ptrace.h>
> +#include "bpf_pt_regs_helpers.h"
>  #include "test_task_pt_regs.skel.h"
>
>  void test_task_pt_regs(void)
> diff --git a/tools/testing/selftests/bpf/progs/test_task_pt_regs.c b/tools/testing/selftests/bpf/progs/test_task_pt_regs.c
> index 6c059f1cfa1b..348da3509093 100644
> --- a/tools/testing/selftests/bpf/progs/test_task_pt_regs.c
> +++ b/tools/testing/selftests/bpf/progs/test_task_pt_regs.c
> @@ -5,8 +5,10 @@
>  #include <bpf/bpf_helpers.h>
>  #include <bpf/bpf_tracing.h>
>
> -struct pt_regs current_regs = {};
> -struct pt_regs ctx_regs = {};
> +#include "bpf_pt_regs_helpers.h"
> +
> +struct bpf_pt_regs current_regs = {};
> +struct bpf_pt_regs ctx_regs = {};
>  int uprobe_res = 0;
>
>  SEC("uprobe/trigger_func")
> @@ -17,8 +19,8 @@ int handle_uprobe(struct pt_regs *ctx)
>
>         current = bpf_get_current_task_btf();
>         regs = (struct pt_regs *) bpf_task_pt_regs(current);
> -       __builtin_memcpy(&current_regs, regs, sizeof(*regs));
> -       __builtin_memcpy(&ctx_regs, ctx, sizeof(*ctx));
> +       bpf_copy_pt_regs(&current_regs, regs);
> +       bpf_copy_pt_regs(&ctx_regs, ctx);
>
>         /* Prove that uprobe was run */
>         uprobe_res = 1;
> --
> 2.33.0
>
Jean-Philippe Brucker Sept. 3, 2021, 12:33 p.m. UTC | #3 
On Thu, Sep 02, 2021 at 12:13:40PM -0700, Alexei Starovoitov wrote:
> On Thu, Sep 2, 2021 at 2:08 AM Jean-Philippe Brucker
> <jean-philippe@linaro.org> wrote:
> >
> > struct pt_regs is not exported to userspace on all archs. arm64 and s390
> > export "user_pt_regs" instead, which causes build failure at the moment:
> >
> >   progs/test_task_pt_regs.c:8:16: error: variable has incomplete type 'struct pt_regs'
> >   struct pt_regs current_regs = {};
> 
> Right, which is 'bpf_user_pt_regs_t'.
> It's defined for all archs and arm64/s390/ppc/risv define it
> differently from pt_regs.
> 
> >
> > Use the multi-arch macros defined by tools/lib/bpf/bpf_tracing.h to copy
> > the pt_regs into a locally-defined struct.
> >
> > Copying the user_pt_regs struct on arm64 wouldn't work because the
> > struct is too large and the compiler complains about using too much
> > stack.
> 
> That's a different issue.

It does work when doing an implicit copy (current_regs = *regs) rather
than using __builtin_memcpy(). Don't know why but I'll take it.

> I think the cleaner fix would be to make the test use
> bpf_user_pt_regs_t instead.

Right, although that comes with another complication. We end up including
tools/include/uapi/asm/bpf_perf_event.h which requires the compiler
builtins "__aarch64__", "__s390__", etc. Those are not defined with
"clang -target bpf" so I have to add them to the command line.
I'll resend with your suggestion but this patch is simpler.

Thanks,
Jean
Andrii Nakryiko Sept. 3, 2021, 4:51 p.m. UTC | #4 
On Fri, Sep 3, 2021 at 5:31 AM Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> On Thu, Sep 02, 2021 at 12:13:40PM -0700, Alexei Starovoitov wrote:
> > On Thu, Sep 2, 2021 at 2:08 AM Jean-Philippe Brucker
> > <jean-philippe@linaro.org> wrote:
> > >
> > > struct pt_regs is not exported to userspace on all archs. arm64 and s390
> > > export "user_pt_regs" instead, which causes build failure at the moment:
> > >
> > >   progs/test_task_pt_regs.c:8:16: error: variable has incomplete type 'struct pt_regs'
> > >   struct pt_regs current_regs = {};
> >
> > Right, which is 'bpf_user_pt_regs_t'.
> > It's defined for all archs and arm64/s390/ppc/risv define it
> > differently from pt_regs.
> >
> > >
> > > Use the multi-arch macros defined by tools/lib/bpf/bpf_tracing.h to copy
> > > the pt_regs into a locally-defined struct.
> > >
> > > Copying the user_pt_regs struct on arm64 wouldn't work because the
> > > struct is too large and the compiler complains about using too much
> > > stack.
> >
> > That's a different issue.
>
> It does work when doing an implicit copy (current_regs = *regs) rather
> than using __builtin_memcpy(). Don't know why but I'll take it.
>
> > I think the cleaner fix would be to make the test use
> > bpf_user_pt_regs_t instead.
>
> Right, although that comes with another complication. We end up including
> tools/include/uapi/asm/bpf_perf_event.h which requires the compiler
> builtins "__aarch64__", "__s390__", etc. Those are not defined with
> "clang -target bpf" so I have to add them to the command line.
> I'll resend with your suggestion but this patch is simpler.
>

The test doesn't care about struct pt_regs type itself, it only cares
to check that contents of captured pt_regs are the same.

We can use CO-RE to check whether user_pt_regs or pt_regs exists in
the kernel. We can also use bpf_core_type_size() to know exactly how
many bytes we want to capture. And then just use
bpf_probe_read_kernel() as memcpy() equivalent to capture bytes. This
should work on all architectures.

> Thanks,
> Jean
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/bpf_pt_regs_helpers.h b/tools/testing/selftests/bpf/bpf_pt_regs_helpers.h
new file mode 100644
index 000000000000..7531f4824ead
--- /dev/null
+++ b/tools/testing/selftests/bpf/bpf_pt_regs_helpers.h
@@ -0,0 +1,30 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __BPF_PT_REGS_HELPERS
+#define __BPF_PT_REGS_HELPERS
+
+#include <bpf/bpf_tracing.h>
+
+struct bpf_pt_regs {
+	unsigned long long parm[5];
+	unsigned long long ret;
+	unsigned long long fp;
+	unsigned long long rc;
+	unsigned long long sp;
+	unsigned long long ip;
+};
+
+static inline void bpf_copy_pt_regs(struct bpf_pt_regs *dest, struct pt_regs *src)
+{
+	dest->parm[0]	= PT_REGS_PARM1(src);
+	dest->parm[1]	= PT_REGS_PARM2(src);
+	dest->parm[2]	= PT_REGS_PARM3(src);
+	dest->parm[3]	= PT_REGS_PARM4(src);
+	dest->parm[4]	= PT_REGS_PARM5(src);
+	dest->ret	= PT_REGS_RET(src);
+	dest->fp	= PT_REGS_FP(src);
+	dest->rc	= PT_REGS_RC(src);
+	dest->sp	= PT_REGS_SP(src);
+	dest->ip	= PT_REGS_IP(src);
+}
+
+#endif /* __BPF_PT_REGS_HELPERS */
diff --git a/tools/testing/selftests/bpf/prog_tests/task_pt_regs.c b/tools/testing/selftests/bpf/prog_tests/task_pt_regs.c
index 53f0e0fa1a53..196453b75937 100644
--- a/tools/testing/selftests/bpf/prog_tests/task_pt_regs.c
+++ b/tools/testing/selftests/bpf/prog_tests/task_pt_regs.c
@@ -2,6 +2,7 @@ 
 #define _GNU_SOURCE
 #include <test_progs.h>
 #include <linux/ptrace.h>
+#include "bpf_pt_regs_helpers.h"
 #include "test_task_pt_regs.skel.h"
 
 void test_task_pt_regs(void)
diff --git a/tools/testing/selftests/bpf/progs/test_task_pt_regs.c b/tools/testing/selftests/bpf/progs/test_task_pt_regs.c
index 6c059f1cfa1b..348da3509093 100644
--- a/tools/testing/selftests/bpf/progs/test_task_pt_regs.c
+++ b/tools/testing/selftests/bpf/progs/test_task_pt_regs.c
@@ -5,8 +5,10 @@ 
 #include <bpf/bpf_helpers.h>
 #include <bpf/bpf_tracing.h>
 
-struct pt_regs current_regs = {};
-struct pt_regs ctx_regs = {};
+#include "bpf_pt_regs_helpers.h"
+
+struct bpf_pt_regs current_regs = {};
+struct bpf_pt_regs ctx_regs = {};
 int uprobe_res = 0;
 
 SEC("uprobe/trigger_func")
@@ -17,8 +19,8 @@  int handle_uprobe(struct pt_regs *ctx)
 
 	current = bpf_get_current_task_btf();
 	regs = (struct pt_regs *) bpf_task_pt_regs(current);
-	__builtin_memcpy(&current_regs, regs, sizeof(*regs));
-	__builtin_memcpy(&ctx_regs, ctx, sizeof(*ctx));
+	bpf_copy_pt_regs(&current_regs, regs);
+	bpf_copy_pt_regs(&ctx_regs, ctx);
 
 	/* Prove that uprobe was run */
 	uprobe_res = 1;