Re: [PATCH v4 bpf-next 1/3] bpf: enable BPF_PROG_TEST_RUN for raw_tracepoint

On Sep 24, 2020, at 12:56 PM, Andrii Nakryiko [off-list ref] wrote:

On Wed, Sep 23, 2020 at 6:46 PM Song Liu [off-list ref] wrote:

quoted

Add .test_run for raw_tracepoint. Also, introduce a new feature that runs
the target program on a specific CPU. This is achieved by a new flag in
bpf_attr.test, BPF_F_TEST_RUN_ON_CPU. When this flag is set, the program
is triggered on cpu with id bpf_attr.test.cpu. This feature is needed for
BPF programs that handle perf_event and other percpu resources, as the
program can access these resource locally.

Acked-by: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Song Liu <redacted>
---
include/linux/bpf.h            |  3 ++
include/uapi/linux/bpf.h       |  7 +++
kernel/bpf/syscall.c           |  2 +-
kernel/trace/bpf_trace.c       |  1 +
net/bpf/test_run.c             | 89 ++++++++++++++++++++++++++++++++++
tools/include/uapi/linux/bpf.h |  7 +++
6 files changed, 108 insertions(+), 1 deletion(-)

[...]

quoted

+int bpf_prog_test_run_raw_tp(struct bpf_prog *prog,
+                            const union bpf_attr *kattr,
+                            union bpf_attr __user *uattr)
+{
+       void __user *ctx_in = u64_to_user_ptr(kattr->test.ctx_in);
+       __u32 ctx_size_in = kattr->test.ctx_size_in;
+       struct bpf_raw_tp_test_run_info info;
+       int cpu, err = 0;
+
+       /* doesn't support data_in/out, ctx_out, duration, or repeat */
+       if (kattr->test.data_in || kattr->test.data_out ||
+           kattr->test.ctx_out || kattr->test.duration ||
+           kattr->test.repeat)

duration and repeat sound generally useful (benchmarking raw_tp
programs), so it's a pity you haven't implemented them. But it can be
added later, so not a deal breaker.

quoted

+               return -EINVAL;
+
+       if (ctx_size_in < prog->aux->max_ctx_offset)
+               return -EINVAL;
+
+       if (ctx_size_in) {
+               info.ctx = kzalloc(ctx_size_in, GFP_USER);
+               if (!info.ctx)
+                       return -ENOMEM;
+               if (copy_from_user(info.ctx, ctx_in, ctx_size_in)) {
+                       err = -EFAULT;
+                       goto out;
+               }
+       } else {
+               info.ctx = NULL;
+       }
+
+       info.prog = prog;
+       cpu = kattr->test.cpu;
+
+       if ((kattr->test.flags & BPF_F_TEST_RUN_ON_CPU) == 0 ||
+           cpu == smp_processor_id()) {

should we enforce that cpu == 0 if BPF_F_TEST_RUN_ON_CPU is not set?

Added a test. 

quoted

+               __bpf_prog_test_run_raw_tp(&info);
+       } else {
+               /* smp_call_function_single() also checks cpu_online()
+                * after csd_lock(). However, since cpu_plus is from user

cpu_plus leftover in a comment

Fixed. 

quoted

+                * space, let's do an extra quick check to filter out
+                * invalid value before smp_call_function_single().
+                */
+               if (!cpu_online(cpu)) {

briefly looking at cpu_online() code, it seems like it's not checking
that cpu is < NR_CPUS. Should we add a selftest that validates that
passing unreasonable cpu index doesn't generate warning or invalid
memory access?

Good catch! We need extra "cpu >= nr_cpu_ids".

Thanks,
Song