| Message ID | 20210902090925.2010528-1-jean-philippe@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [bpf-next] selftests/bpf: Fix build of task_pt_regs tests for arm64 | expand |
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> [...]
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(¤t_regs, regs, sizeof(*regs)); > - __builtin_memcpy(&ctx_regs, ctx, sizeof(*ctx)); > + bpf_copy_pt_regs(¤t_regs, regs); > + bpf_copy_pt_regs(&ctx_regs, ctx); > > /* Prove that uprobe was run */ > uprobe_res = 1; > -- > 2.33.0 >
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
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 --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(¤t_regs, regs, sizeof(*regs)); - __builtin_memcpy(&ctx_regs, ctx, sizeof(*ctx)); + bpf_copy_pt_regs(¤t_regs, regs); + bpf_copy_pt_regs(&ctx_regs, ctx); /* Prove that uprobe was run */ uprobe_res = 1;
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 -- 2.33.0