* [PATCH v2 bpf-next 0/2] Allow attaching to bare tracepoints @ 2021-01-16 18:21 Qais Yousef 2021-01-16 18:21 ` [PATCH v2 bpf-next 1/2] trace: bpf: Allow bpf to attach " Qais Yousef 2021-01-16 18:21 ` [PATCH v2 bpf-next 2/2] selftests: bpf: Add a new test for " Qais Yousef 0 siblings, 2 replies; 8+ messages in thread From: Qais Yousef @ 2021-01-16 18:21 UTC (permalink / raw) To: netdev, bpf Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, Yonghong Song, Steven Rostedt, Peter Zijlstra (Intel), linux-kernel, Qais Yousef Changes in v2: * Fix compilation error. * Make the new test use write() instead of read() Add some missing glue logic to teach bpf about bare tracepoints - tracepoints without any trace event associated with them. Bare tracepoints are declare with DECLARE_TRACE(). Full tracepoints are declare with TRACE_EVENT(). BPF can attach to these tracepoints as RAW_TRACEPOINT() only as there're no events in tracefs created with them. Qais Yousef (2): trace: bpf: Allow bpf to attach to bare tracepoints selftests: bpf: Add a new test for bare tracepoints Documentation/bpf/bpf_design_QA.rst | 6 +++++ include/trace/bpf_probe.h | 12 +++++++-- .../bpf/bpf_testmod/bpf_testmod-events.h | 6 +++++ .../selftests/bpf/bpf_testmod/bpf_testmod.c | 21 ++++++++++++++- .../selftests/bpf/bpf_testmod/bpf_testmod.h | 6 +++++ .../selftests/bpf/prog_tests/module_attach.c | 27 +++++++++++++++++++ .../selftests/bpf/progs/test_module_attach.c | 10 +++++++ 7 files changed, 85 insertions(+), 3 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 bpf-next 1/2] trace: bpf: Allow bpf to attach to bare tracepoints 2021-01-16 18:21 [PATCH v2 bpf-next 0/2] Allow attaching to bare tracepoints Qais Yousef @ 2021-01-16 18:21 ` Qais Yousef 2021-01-17 2:03 ` Yonghong Song 2021-01-16 18:21 ` [PATCH v2 bpf-next 2/2] selftests: bpf: Add a new test for " Qais Yousef 1 sibling, 1 reply; 8+ messages in thread From: Qais Yousef @ 2021-01-16 18:21 UTC (permalink / raw) To: netdev, bpf Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, Yonghong Song, Steven Rostedt, Peter Zijlstra (Intel), linux-kernel, Qais Yousef Some subsystems only have bare tracepoints (a tracepoint with no associated trace event) to avoid the problem of trace events being an ABI that can't be changed. From bpf presepective, bare tracepoints are what it calls RAW_TRACEPOINT(). Since bpf assumed there's 1:1 mapping, it relied on hooking to DEFINE_EVENT() macro to create bpf mapping of the tracepoints. Since bare tracepoints use DECLARE_TRACE() to create the tracepoint, bpf had no knowledge about their existence. By teaching bpf_probe.h to parse DECLARE_TRACE() in a similar fashion to DEFINE_EVENT(), bpf can find and attach to the new raw tracepoints. Enabling that comes with the contract that changes to raw tracepoints don't constitute a regression if they break existing bpf programs. We need the ability to continue to morph and modify these raw tracepoints without worrying about any ABI. Update Documentation/bpf/bpf_design_QA.rst to document this contract. Signed-off-by: Qais Yousef <qais.yousef@arm.com> --- Documentation/bpf/bpf_design_QA.rst | 6 ++++++ include/trace/bpf_probe.h | 12 ++++++++++-- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/Documentation/bpf/bpf_design_QA.rst b/Documentation/bpf/bpf_design_QA.rst index 2df7b067ab93..0e15f9b05c9d 100644 --- a/Documentation/bpf/bpf_design_QA.rst +++ b/Documentation/bpf/bpf_design_QA.rst @@ -208,6 +208,12 @@ data structures and compile with kernel internal headers. Both of these kernel internals are subject to change and can break with newer kernels such that the program needs to be adapted accordingly. +Q: Are tracepoints part of the stable ABI? +------------------------------------------ +A: NO. Tracepoints are tied to internal implementation details hence they are +subject to change and can break with newer kernels. BPF programs need to change +accordingly when this happens. + Q: How much stack space a BPF program uses? ------------------------------------------- A: Currently all program types are limited to 512 bytes of stack diff --git a/include/trace/bpf_probe.h b/include/trace/bpf_probe.h index cd74bffed5c6..a23be89119aa 100644 --- a/include/trace/bpf_probe.h +++ b/include/trace/bpf_probe.h @@ -55,8 +55,7 @@ /* tracepoints with more than 12 arguments will hit build error */ #define CAST_TO_U64(...) CONCATENATE(__CAST, COUNT_ARGS(__VA_ARGS__))(__VA_ARGS__) -#undef DECLARE_EVENT_CLASS -#define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print) \ +#define __BPF_DECLARE_TRACE(call, proto, args) \ static notrace void \ __bpf_trace_##call(void *__data, proto) \ { \ @@ -64,6 +63,10 @@ __bpf_trace_##call(void *__data, proto) \ CONCATENATE(bpf_trace_run, COUNT_ARGS(args))(prog, CAST_TO_U64(args)); \ } +#undef DECLARE_EVENT_CLASS +#define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print) \ + __BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args)) + /* * This part is compiled out, it is only here as a build time check * to make sure that if the tracepoint handling changes, the @@ -111,6 +114,11 @@ __DEFINE_EVENT(template, call, PARAMS(proto), PARAMS(args), size) #define DEFINE_EVENT_PRINT(template, name, proto, args, print) \ DEFINE_EVENT(template, name, PARAMS(proto), PARAMS(args)) +#undef DECLARE_TRACE +#define DECLARE_TRACE(call, proto, args) \ + __BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args)) \ + __DEFINE_EVENT(call, call, PARAMS(proto), PARAMS(args), 0) + #include TRACE_INCLUDE(TRACE_INCLUDE_FILE) #undef DEFINE_EVENT_WRITABLE -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 bpf-next 1/2] trace: bpf: Allow bpf to attach to bare tracepoints 2021-01-16 18:21 ` [PATCH v2 bpf-next 1/2] trace: bpf: Allow bpf to attach " Qais Yousef @ 2021-01-17 2:03 ` Yonghong Song 0 siblings, 0 replies; 8+ messages in thread From: Yonghong Song @ 2021-01-17 2:03 UTC (permalink / raw) To: Qais Yousef, netdev, bpf Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, Steven Rostedt, Peter Zijlstra (Intel), linux-kernel On 1/16/21 10:21 AM, Qais Yousef wrote: > Some subsystems only have bare tracepoints (a tracepoint with no > associated trace event) to avoid the problem of trace events being an > ABI that can't be changed. > > From bpf presepective, bare tracepoints are what it calls > RAW_TRACEPOINT(). > > Since bpf assumed there's 1:1 mapping, it relied on hooking to > DEFINE_EVENT() macro to create bpf mapping of the tracepoints. Since > bare tracepoints use DECLARE_TRACE() to create the tracepoint, bpf had > no knowledge about their existence. > > By teaching bpf_probe.h to parse DECLARE_TRACE() in a similar fashion to > DEFINE_EVENT(), bpf can find and attach to the new raw tracepoints. > > Enabling that comes with the contract that changes to raw tracepoints > don't constitute a regression if they break existing bpf programs. > We need the ability to continue to morph and modify these raw > tracepoints without worrying about any ABI. > > Update Documentation/bpf/bpf_design_QA.rst to document this contract. > > Signed-off-by: Qais Yousef <qais.yousef@arm.com> Acked-by: Yonghong Song <yhs@fb.com> ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 bpf-next 2/2] selftests: bpf: Add a new test for bare tracepoints 2021-01-16 18:21 [PATCH v2 bpf-next 0/2] Allow attaching to bare tracepoints Qais Yousef 2021-01-16 18:21 ` [PATCH v2 bpf-next 1/2] trace: bpf: Allow bpf to attach " Qais Yousef @ 2021-01-16 18:21 ` Qais Yousef 2021-01-17 2:11 ` Yonghong Song 1 sibling, 1 reply; 8+ messages in thread From: Qais Yousef @ 2021-01-16 18:21 UTC (permalink / raw) To: netdev, bpf Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, Yonghong Song, Steven Rostedt, Peter Zijlstra (Intel), linux-kernel, Qais Yousef Reuse module_attach infrastructure to add a new bare tracepoint to check we can attach to it as a raw tracepoint. Signed-off-by: Qais Yousef <qais.yousef@arm.com> --- .../bpf/bpf_testmod/bpf_testmod-events.h | 6 +++++ .../selftests/bpf/bpf_testmod/bpf_testmod.c | 21 ++++++++++++++- .../selftests/bpf/bpf_testmod/bpf_testmod.h | 6 +++++ .../selftests/bpf/prog_tests/module_attach.c | 27 +++++++++++++++++++ .../selftests/bpf/progs/test_module_attach.c | 10 +++++++ 5 files changed, 69 insertions(+), 1 deletion(-) diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h index b83ea448bc79..89c6d58e5dd6 100644 --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h @@ -28,6 +28,12 @@ TRACE_EVENT(bpf_testmod_test_read, __entry->pid, __entry->comm, __entry->off, __entry->len) ); +/* A bare tracepoint with no event associated with it */ +DECLARE_TRACE(bpf_testmod_test_write_bare, + TP_PROTO(struct task_struct *task, struct bpf_testmod_test_write_ctx *ctx), + TP_ARGS(task, ctx) +); + #endif /* _BPF_TESTMOD_EVENTS_H */ #undef TRACE_INCLUDE_PATH diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c index 2df19d73ca49..e900adad2276 100644 --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c @@ -28,9 +28,28 @@ bpf_testmod_test_read(struct file *file, struct kobject *kobj, EXPORT_SYMBOL(bpf_testmod_test_read); ALLOW_ERROR_INJECTION(bpf_testmod_test_read, ERRNO); +noinline ssize_t +bpf_testmod_test_write(struct file *file, struct kobject *kobj, + struct bin_attribute *bin_attr, + char *buf, loff_t off, size_t len) +{ + struct bpf_testmod_test_write_ctx ctx = { + .buf = buf, + .off = off, + .len = len, + }; + + trace_bpf_testmod_test_write_bare(current, &ctx); + + return -EIO; /* always fail */ +} +EXPORT_SYMBOL(bpf_testmod_test_write); +ALLOW_ERROR_INJECTION(bpf_testmod_test_write, ERRNO); + static struct bin_attribute bin_attr_bpf_testmod_file __ro_after_init = { - .attr = { .name = "bpf_testmod", .mode = 0444, }, + .attr = { .name = "bpf_testmod", .mode = 0666, }, .read = bpf_testmod_test_read, + .write = bpf_testmod_test_write, }; static int bpf_testmod_init(void) diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h index b81adfedb4f6..b3892dc40111 100644 --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h @@ -11,4 +11,10 @@ struct bpf_testmod_test_read_ctx { size_t len; }; +struct bpf_testmod_test_write_ctx { + char *buf; + loff_t off; + size_t len; +}; + #endif /* _BPF_TESTMOD_H */ diff --git a/tools/testing/selftests/bpf/prog_tests/module_attach.c b/tools/testing/selftests/bpf/prog_tests/module_attach.c index 50796b651f72..e4605c0b5af1 100644 --- a/tools/testing/selftests/bpf/prog_tests/module_attach.c +++ b/tools/testing/selftests/bpf/prog_tests/module_attach.c @@ -21,9 +21,34 @@ static int trigger_module_test_read(int read_sz) return 0; } +static int trigger_module_test_write(int write_sz) +{ + int fd, err; + char *buf = malloc(write_sz); + + if (!buf) + return -ENOMEM; + + memset(buf, 'a', write_sz); + buf[write_sz-1] = '\0'; + + fd = open("/sys/kernel/bpf_testmod", O_WRONLY); + err = -errno; + if (CHECK(fd < 0, "testmod_file_open", "failed: %d\n", err)) + goto out; + + write(fd, buf, write_sz); + close(fd); +out: + free(buf); + + return 0; +} + void test_module_attach(void) { const int READ_SZ = 456; + const int WRITE_SZ = 457; struct test_module_attach* skel; struct test_module_attach__bss *bss; int err; @@ -48,8 +73,10 @@ void test_module_attach(void) /* trigger tracepoint */ ASSERT_OK(trigger_module_test_read(READ_SZ), "trigger_read"); + ASSERT_OK(trigger_module_test_write(WRITE_SZ), "trigger_write"); ASSERT_EQ(bss->raw_tp_read_sz, READ_SZ, "raw_tp"); + ASSERT_EQ(bss->raw_tp_bare_write_sz, WRITE_SZ, "raw_tp_bare"); ASSERT_EQ(bss->tp_btf_read_sz, READ_SZ, "tp_btf"); ASSERT_EQ(bss->fentry_read_sz, READ_SZ, "fentry"); ASSERT_EQ(bss->fentry_manual_read_sz, READ_SZ, "fentry_manual"); diff --git a/tools/testing/selftests/bpf/progs/test_module_attach.c b/tools/testing/selftests/bpf/progs/test_module_attach.c index efd1e287ac17..bd37ceec5587 100644 --- a/tools/testing/selftests/bpf/progs/test_module_attach.c +++ b/tools/testing/selftests/bpf/progs/test_module_attach.c @@ -17,6 +17,16 @@ int BPF_PROG(handle_raw_tp, return 0; } +__u32 raw_tp_bare_write_sz = 0; + +SEC("raw_tp/bpf_testmod_test_write_bare") +int BPF_PROG(handle_raw_tp_bare, + struct task_struct *task, struct bpf_testmod_test_write_ctx *write_ctx) +{ + raw_tp_bare_write_sz = BPF_CORE_READ(write_ctx, len); + return 0; +} + __u32 tp_btf_read_sz = 0; SEC("tp_btf/bpf_testmod_test_read") -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 bpf-next 2/2] selftests: bpf: Add a new test for bare tracepoints 2021-01-16 18:21 ` [PATCH v2 bpf-next 2/2] selftests: bpf: Add a new test for " Qais Yousef @ 2021-01-17 2:11 ` Yonghong Song 2021-01-18 12:18 ` Qais Yousef 0 siblings, 1 reply; 8+ messages in thread From: Yonghong Song @ 2021-01-17 2:11 UTC (permalink / raw) To: Qais Yousef, netdev, bpf Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, Steven Rostedt, Peter Zijlstra (Intel), linux-kernel On 1/16/21 10:21 AM, Qais Yousef wrote: > Reuse module_attach infrastructure to add a new bare tracepoint to check > we can attach to it as a raw tracepoint. > > Signed-off-by: Qais Yousef <qais.yousef@arm.com> > --- > .../bpf/bpf_testmod/bpf_testmod-events.h | 6 +++++ > .../selftests/bpf/bpf_testmod/bpf_testmod.c | 21 ++++++++++++++- > .../selftests/bpf/bpf_testmod/bpf_testmod.h | 6 +++++ > .../selftests/bpf/prog_tests/module_attach.c | 27 +++++++++++++++++++ > .../selftests/bpf/progs/test_module_attach.c | 10 +++++++ > 5 files changed, 69 insertions(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h > index b83ea448bc79..89c6d58e5dd6 100644 > --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h > +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h > @@ -28,6 +28,12 @@ TRACE_EVENT(bpf_testmod_test_read, > __entry->pid, __entry->comm, __entry->off, __entry->len) > ); > > +/* A bare tracepoint with no event associated with it */ > +DECLARE_TRACE(bpf_testmod_test_write_bare, > + TP_PROTO(struct task_struct *task, struct bpf_testmod_test_write_ctx *ctx), > + TP_ARGS(task, ctx) > +); > + > #endif /* _BPF_TESTMOD_EVENTS_H */ > > #undef TRACE_INCLUDE_PATH > diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > index 2df19d73ca49..e900adad2276 100644 > --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > @@ -28,9 +28,28 @@ bpf_testmod_test_read(struct file *file, struct kobject *kobj, > EXPORT_SYMBOL(bpf_testmod_test_read); > ALLOW_ERROR_INJECTION(bpf_testmod_test_read, ERRNO); > > +noinline ssize_t > +bpf_testmod_test_write(struct file *file, struct kobject *kobj, > + struct bin_attribute *bin_attr, > + char *buf, loff_t off, size_t len) > +{ > + struct bpf_testmod_test_write_ctx ctx = { > + .buf = buf, > + .off = off, > + .len = len, > + }; > + > + trace_bpf_testmod_test_write_bare(current, &ctx); > + > + return -EIO; /* always fail */ > +} > +EXPORT_SYMBOL(bpf_testmod_test_write); > +ALLOW_ERROR_INJECTION(bpf_testmod_test_write, ERRNO); > + > static struct bin_attribute bin_attr_bpf_testmod_file __ro_after_init = { Do we need to remove __ro_after_init? > - .attr = { .name = "bpf_testmod", .mode = 0444, }, > + .attr = { .name = "bpf_testmod", .mode = 0666, }, > .read = bpf_testmod_test_read, > + .write = bpf_testmod_test_write, > }; > > static int bpf_testmod_init(void) > diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h > index b81adfedb4f6..b3892dc40111 100644 > --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h > +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h > @@ -11,4 +11,10 @@ struct bpf_testmod_test_read_ctx { > size_t len; > }; > > +struct bpf_testmod_test_write_ctx { > + char *buf; > + loff_t off; > + size_t len; > +}; > + > #endif /* _BPF_TESTMOD_H */ > diff --git a/tools/testing/selftests/bpf/prog_tests/module_attach.c b/tools/testing/selftests/bpf/prog_tests/module_attach.c > index 50796b651f72..e4605c0b5af1 100644 > --- a/tools/testing/selftests/bpf/prog_tests/module_attach.c > +++ b/tools/testing/selftests/bpf/prog_tests/module_attach.c > @@ -21,9 +21,34 @@ static int trigger_module_test_read(int read_sz) > return 0; > } > > +static int trigger_module_test_write(int write_sz) > +{ > + int fd, err; Init err = 0? > + char *buf = malloc(write_sz); > + > + if (!buf) > + return -ENOMEM; Looks like we already non-negative value, so return ENOMEM? > + > + memset(buf, 'a', write_sz); > + buf[write_sz-1] = '\0'; > + > + fd = open("/sys/kernel/bpf_testmod", O_WRONLY); > + err = -errno; > + if (CHECK(fd < 0, "testmod_file_open", "failed: %d\n", err)) > + goto out; Change the above to fd = open("/sys/kernel/bpf_testmod", O_WRONLY); if (CHECK(fd < 0, "testmod_file_open", "failed: %d\n", errno)) { err = -errno; goto out; } > + > + write(fd, buf, write_sz); > + close(fd); > +out: > + free(buf); > + No need for extra line here. > + return 0; return err. > +} > + > void test_module_attach(void) > { > const int READ_SZ = 456; > + const int WRITE_SZ = 457; > struct test_module_attach* skel; > struct test_module_attach__bss *bss; > int err; > @@ -48,8 +73,10 @@ void test_module_attach(void) > > /* trigger tracepoint */ > ASSERT_OK(trigger_module_test_read(READ_SZ), "trigger_read"); > + ASSERT_OK(trigger_module_test_write(WRITE_SZ), "trigger_write"); > > ASSERT_EQ(bss->raw_tp_read_sz, READ_SZ, "raw_tp"); > + ASSERT_EQ(bss->raw_tp_bare_write_sz, WRITE_SZ, "raw_tp_bare"); > ASSERT_EQ(bss->tp_btf_read_sz, READ_SZ, "tp_btf"); > ASSERT_EQ(bss->fentry_read_sz, READ_SZ, "fentry"); > ASSERT_EQ(bss->fentry_manual_read_sz, READ_SZ, "fentry_manual"); > diff --git a/tools/testing/selftests/bpf/progs/test_module_attach.c b/tools/testing/selftests/bpf/progs/test_module_attach.c > index efd1e287ac17..bd37ceec5587 100644 > --- a/tools/testing/selftests/bpf/progs/test_module_attach.c > +++ b/tools/testing/selftests/bpf/progs/test_module_attach.c > @@ -17,6 +17,16 @@ int BPF_PROG(handle_raw_tp, > return 0; > } > > +__u32 raw_tp_bare_write_sz = 0; > + > +SEC("raw_tp/bpf_testmod_test_write_bare") > +int BPF_PROG(handle_raw_tp_bare, > + struct task_struct *task, struct bpf_testmod_test_write_ctx *write_ctx) > +{ > + raw_tp_bare_write_sz = BPF_CORE_READ(write_ctx, len); > + return 0; > +} > + > __u32 tp_btf_read_sz = 0; > > SEC("tp_btf/bpf_testmod_test_read") > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 bpf-next 2/2] selftests: bpf: Add a new test for bare tracepoints 2021-01-17 2:11 ` Yonghong Song @ 2021-01-18 12:18 ` Qais Yousef 2021-01-18 17:48 ` Yonghong Song 0 siblings, 1 reply; 8+ messages in thread From: Qais Yousef @ 2021-01-18 12:18 UTC (permalink / raw) To: Yonghong Song Cc: netdev, bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, Steven Rostedt, Peter Zijlstra (Intel), linux-kernel On 01/16/21 18:11, Yonghong Song wrote: > > > On 1/16/21 10:21 AM, Qais Yousef wrote: > > Reuse module_attach infrastructure to add a new bare tracepoint to check > > we can attach to it as a raw tracepoint. > > > > Signed-off-by: Qais Yousef <qais.yousef@arm.com> > > --- > > .../bpf/bpf_testmod/bpf_testmod-events.h | 6 +++++ > > .../selftests/bpf/bpf_testmod/bpf_testmod.c | 21 ++++++++++++++- > > .../selftests/bpf/bpf_testmod/bpf_testmod.h | 6 +++++ > > .../selftests/bpf/prog_tests/module_attach.c | 27 +++++++++++++++++++ > > .../selftests/bpf/progs/test_module_attach.c | 10 +++++++ > > 5 files changed, 69 insertions(+), 1 deletion(-) > > > > diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h > > index b83ea448bc79..89c6d58e5dd6 100644 > > --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h > > +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h > > @@ -28,6 +28,12 @@ TRACE_EVENT(bpf_testmod_test_read, > > __entry->pid, __entry->comm, __entry->off, __entry->len) > > ); > > +/* A bare tracepoint with no event associated with it */ > > +DECLARE_TRACE(bpf_testmod_test_write_bare, > > + TP_PROTO(struct task_struct *task, struct bpf_testmod_test_write_ctx *ctx), > > + TP_ARGS(task, ctx) > > +); > > + > > #endif /* _BPF_TESTMOD_EVENTS_H */ > > #undef TRACE_INCLUDE_PATH > > diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > > index 2df19d73ca49..e900adad2276 100644 > > --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > > +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > > @@ -28,9 +28,28 @@ bpf_testmod_test_read(struct file *file, struct kobject *kobj, > > EXPORT_SYMBOL(bpf_testmod_test_read); > > ALLOW_ERROR_INJECTION(bpf_testmod_test_read, ERRNO); > > +noinline ssize_t > > +bpf_testmod_test_write(struct file *file, struct kobject *kobj, > > + struct bin_attribute *bin_attr, > > + char *buf, loff_t off, size_t len) > > +{ > > + struct bpf_testmod_test_write_ctx ctx = { > > + .buf = buf, > > + .off = off, > > + .len = len, > > + }; > > + > > + trace_bpf_testmod_test_write_bare(current, &ctx); > > + > > + return -EIO; /* always fail */ > > +} > > +EXPORT_SYMBOL(bpf_testmod_test_write); > > +ALLOW_ERROR_INJECTION(bpf_testmod_test_write, ERRNO); > > + > > static struct bin_attribute bin_attr_bpf_testmod_file __ro_after_init = { > > Do we need to remove __ro_after_init? I don't think so. The structure should still remain RO AFAIU. > > > - .attr = { .name = "bpf_testmod", .mode = 0444, }, > > + .attr = { .name = "bpf_testmod", .mode = 0666, }, > > .read = bpf_testmod_test_read, > > + .write = bpf_testmod_test_write, > > }; > > static int bpf_testmod_init(void) > > diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h > > index b81adfedb4f6..b3892dc40111 100644 > > --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h > > +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h > > @@ -11,4 +11,10 @@ struct bpf_testmod_test_read_ctx { > > size_t len; > > }; > > +struct bpf_testmod_test_write_ctx { > > + char *buf; > > + loff_t off; > > + size_t len; > > +}; > > + > > #endif /* _BPF_TESTMOD_H */ > > diff --git a/tools/testing/selftests/bpf/prog_tests/module_attach.c b/tools/testing/selftests/bpf/prog_tests/module_attach.c > > index 50796b651f72..e4605c0b5af1 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/module_attach.c > > +++ b/tools/testing/selftests/bpf/prog_tests/module_attach.c > > @@ -21,9 +21,34 @@ static int trigger_module_test_read(int read_sz) > > return 0; > > } > > +static int trigger_module_test_write(int write_sz) > > +{ > > + int fd, err; > > Init err = 0? I don't see what difference this makes. > > > + char *buf = malloc(write_sz); > > + > > + if (!buf) > > + return -ENOMEM; > > Looks like we already non-negative value, so return ENOMEM? We already set err=-errno. So shouldn't we return negative too? > > > + > > + memset(buf, 'a', write_sz); > > + buf[write_sz-1] = '\0'; > > + > > + fd = open("/sys/kernel/bpf_testmod", O_WRONLY); > > + err = -errno; > > + if (CHECK(fd < 0, "testmod_file_open", "failed: %d\n", err)) > > + goto out; > > Change the above to > fd = open("/sys/kernel/bpf_testmod", O_WRONLY); > if (CHECK(fd < 0, "testmod_file_open", "failed: %d\n", errno)) { > err = -errno; > goto out; > } I kept the code consistent with the definition of trigger_module_test_read(). I'll leave it up to the maintainer to pick up the style changes if they prefer it this way. Thanks for the ack and for the review. Cheers -- Qais Yousef ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 bpf-next 2/2] selftests: bpf: Add a new test for bare tracepoints 2021-01-18 12:18 ` Qais Yousef @ 2021-01-18 17:48 ` Yonghong Song 2021-01-19 9:57 ` Qais Yousef 0 siblings, 1 reply; 8+ messages in thread From: Yonghong Song @ 2021-01-18 17:48 UTC (permalink / raw) To: Qais Yousef Cc: netdev, bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, Steven Rostedt, Peter Zijlstra (Intel), linux-kernel On 1/18/21 4:18 AM, Qais Yousef wrote: > On 01/16/21 18:11, Yonghong Song wrote: >> >> >> On 1/16/21 10:21 AM, Qais Yousef wrote: >>> Reuse module_attach infrastructure to add a new bare tracepoint to check >>> we can attach to it as a raw tracepoint. >>> >>> Signed-off-by: Qais Yousef <qais.yousef@arm.com> >>> --- >>> .../bpf/bpf_testmod/bpf_testmod-events.h | 6 +++++ >>> .../selftests/bpf/bpf_testmod/bpf_testmod.c | 21 ++++++++++++++- >>> .../selftests/bpf/bpf_testmod/bpf_testmod.h | 6 +++++ >>> .../selftests/bpf/prog_tests/module_attach.c | 27 +++++++++++++++++++ >>> .../selftests/bpf/progs/test_module_attach.c | 10 +++++++ >>> 5 files changed, 69 insertions(+), 1 deletion(-) >>> >>> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h >>> index b83ea448bc79..89c6d58e5dd6 100644 >>> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h >>> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h >>> @@ -28,6 +28,12 @@ TRACE_EVENT(bpf_testmod_test_read, >>> __entry->pid, __entry->comm, __entry->off, __entry->len) >>> ); >>> +/* A bare tracepoint with no event associated with it */ >>> +DECLARE_TRACE(bpf_testmod_test_write_bare, >>> + TP_PROTO(struct task_struct *task, struct bpf_testmod_test_write_ctx *ctx), >>> + TP_ARGS(task, ctx) >>> +); >>> + >>> #endif /* _BPF_TESTMOD_EVENTS_H */ >>> #undef TRACE_INCLUDE_PATH >>> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c >>> index 2df19d73ca49..e900adad2276 100644 >>> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c >>> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c >>> @@ -28,9 +28,28 @@ bpf_testmod_test_read(struct file *file, struct kobject *kobj, >>> EXPORT_SYMBOL(bpf_testmod_test_read); >>> ALLOW_ERROR_INJECTION(bpf_testmod_test_read, ERRNO); >>> +noinline ssize_t >>> +bpf_testmod_test_write(struct file *file, struct kobject *kobj, >>> + struct bin_attribute *bin_attr, >>> + char *buf, loff_t off, size_t len) >>> +{ >>> + struct bpf_testmod_test_write_ctx ctx = { >>> + .buf = buf, >>> + .off = off, >>> + .len = len, >>> + }; >>> + >>> + trace_bpf_testmod_test_write_bare(current, &ctx); >>> + >>> + return -EIO; /* always fail */ >>> +} >>> +EXPORT_SYMBOL(bpf_testmod_test_write); >>> +ALLOW_ERROR_INJECTION(bpf_testmod_test_write, ERRNO); >>> + >>> static struct bin_attribute bin_attr_bpf_testmod_file __ro_after_init = { >> >> Do we need to remove __ro_after_init? > > I don't think so. The structure should still remain RO AFAIU. okay. > >> >>> - .attr = { .name = "bpf_testmod", .mode = 0444, }, >>> + .attr = { .name = "bpf_testmod", .mode = 0666, }, >>> .read = bpf_testmod_test_read, >>> + .write = bpf_testmod_test_write, >>> }; >>> static int bpf_testmod_init(void) >>> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h >>> index b81adfedb4f6..b3892dc40111 100644 >>> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h >>> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h >>> @@ -11,4 +11,10 @@ struct bpf_testmod_test_read_ctx { >>> size_t len; >>> }; >>> +struct bpf_testmod_test_write_ctx { >>> + char *buf; >>> + loff_t off; >>> + size_t len; >>> +}; >>> + >>> #endif /* _BPF_TESTMOD_H */ >>> diff --git a/tools/testing/selftests/bpf/prog_tests/module_attach.c b/tools/testing/selftests/bpf/prog_tests/module_attach.c >>> index 50796b651f72..e4605c0b5af1 100644 >>> --- a/tools/testing/selftests/bpf/prog_tests/module_attach.c >>> +++ b/tools/testing/selftests/bpf/prog_tests/module_attach.c >>> @@ -21,9 +21,34 @@ static int trigger_module_test_read(int read_sz) >>> return 0; >>> } >>> +static int trigger_module_test_write(int write_sz) >>> +{ >>> + int fd, err; >> >> Init err = 0? > > I don't see what difference this makes. > >> >>> + char *buf = malloc(write_sz); >>> + >>> + if (!buf) >>> + return -ENOMEM; >> >> Looks like we already non-negative value, so return ENOMEM? > > We already set err=-errno. So shouldn't we return negative too? Oh, yes, return -ENOMEM sounds right here. > >> >>> + >>> + memset(buf, 'a', write_sz); >>> + buf[write_sz-1] = '\0'; >>> + >>> + fd = open("/sys/kernel/bpf_testmod", O_WRONLY); >>> + err = -errno; >>> + if (CHECK(fd < 0, "testmod_file_open", "failed: %d\n", err)) >>> + goto out; >> >> Change the above to >> fd = open("/sys/kernel/bpf_testmod", O_WRONLY); >> if (CHECK(fd < 0, "testmod_file_open", "failed: %d\n", errno)) { Here it should be ... "failed: %d\n", -errno. >> err = -errno; >> goto out; >> } > > I kept the code consistent with the definition of trigger_module_test_read(). The original patch code: +static int trigger_module_test_write(int write_sz) +{ + int fd, err; + char *buf = malloc(write_sz); + + if (!buf) + return -ENOMEM; + + memset(buf, 'a', write_sz); + buf[write_sz-1] = '\0'; + + fd = open("/sys/kernel/bpf_testmod", O_WRONLY); + err = -errno; + if (CHECK(fd < 0, "testmod_file_open", "failed: %d\n", err)) + goto out; + + write(fd, buf, write_sz); + close(fd); +out: + free(buf); + + return 0; +} Even for "fd < 0" case, it "goto out" and "return 0". We should return error code here instead of 0. Second, "err = -errno" is set before checking fd < 0. If fd >= 0, err might inherit an postive errno from previous failure. In trigger_module_test_write(), it is okay since the err is only used when fd < 0: err = -errno; if (CHECK(fd < 0, "testmod_file_open", "failed: %d\n", err)) return err; My above rewrite intends to use "err" during final "return" statement, so I put assignment of "err = -errno" inside the CHECK branch. But there are different ways to implement this properly. > > I'll leave it up to the maintainer to pick up the style changes if they prefer > it this way. > > Thanks for the ack and for the review. No problem. > > Cheers > > -- > Qais Yousef > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 bpf-next 2/2] selftests: bpf: Add a new test for bare tracepoints 2021-01-18 17:48 ` Yonghong Song @ 2021-01-19 9:57 ` Qais Yousef 0 siblings, 0 replies; 8+ messages in thread From: Qais Yousef @ 2021-01-19 9:57 UTC (permalink / raw) To: Yonghong Song Cc: netdev, bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, Steven Rostedt, Peter Zijlstra (Intel), linux-kernel Hi Yonghong On 01/18/21 09:48, Yonghong Song wrote: > The original patch code: > > +static int trigger_module_test_write(int write_sz) > +{ > + int fd, err; > + char *buf = malloc(write_sz); > + > + if (!buf) > + return -ENOMEM; > + > + memset(buf, 'a', write_sz); > + buf[write_sz-1] = '\0'; > + > + fd = open("/sys/kernel/bpf_testmod", O_WRONLY); > + err = -errno; > + if (CHECK(fd < 0, "testmod_file_open", "failed: %d\n", err)) > + goto out; > + > + write(fd, buf, write_sz); > + close(fd); > +out: > + free(buf); > + > + return 0; > +} > > Even for "fd < 0" case, it "goto out" and "return 0". We should return > error code here instead of 0. > > Second, "err = -errno" is set before checking fd < 0. If fd >= 0, err might > inherit an postive errno from previous failure. > In trigger_module_test_write(), it is okay since the err is only used > when fd < 0: > err = -errno; > if (CHECK(fd < 0, "testmod_file_open", "failed: %d\n", err)) > return err; > > My above rewrite intends to use "err" during final "return" statement, > so I put assignment of "err = -errno" inside the CHECK branch. > But there are different ways to implement this properly. Okay I see now. Sorry I missed your point initially. I will fix and send v3. Thanks -- Qais Yousef ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-01-19 14:46 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-16 18:21 [PATCH v2 bpf-next 0/2] Allow attaching to bare tracepoints Qais Yousef 2021-01-16 18:21 ` [PATCH v2 bpf-next 1/2] trace: bpf: Allow bpf to attach " Qais Yousef 2021-01-17 2:03 ` Yonghong Song 2021-01-16 18:21 ` [PATCH v2 bpf-next 2/2] selftests: bpf: Add a new test for " Qais Yousef 2021-01-17 2:11 ` Yonghong Song 2021-01-18 12:18 ` Qais Yousef 2021-01-18 17:48 ` Yonghong Song 2021-01-19 9:57 ` Qais Yousef
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).