> On Sep 24, 2020, at 12:56 PM, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Wed, Sep 23, 2020 at 6:46 PM Song Liu <songliubraving@xxxxxx> wrote:
>>
>> 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@xxxxxxxxx>
>> Signed-off-by: Song Liu <songliubraving@xxxxxx>
>> ---
>> 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(-)
>>
>
> [...]
>
>> +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.
>
>> + 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.
>
>
>> + __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.
>
>> + * 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
