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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4Bzasv2wJZ32G0K9aohZN=s7nys5LMcM4MywyMxBW7baOsQ@mail.gmail.com>
Date:   Thu, 24 Sep 2020 12:56:46 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Song Liu <songliubraving@...com>
Cc:     Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Kernel Team <kernel-team@...com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        john fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>
Subject: Re: [PATCH v4 bpf-next 1/3] bpf: enable BPF_PROG_TEST_RUN for raw_tracepoint

On Wed, Sep 23, 2020 at 6:46 PM Song Liu <songliubraving@...com> 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@...il.com>
> Signed-off-by: Song Liu <songliubraving@...com>
> ---
>  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?


> +               __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

> +                * 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?

> +                       err = -ENXIO;
> +                       goto out;
> +               }
> +
> +               err = smp_call_function_single(cpu, __bpf_prog_test_run_raw_tp,
> +                                              &info, 1);
> +               if (err)
> +                       goto out;
> +       }
> +

[...]