Re: [PATCH v4 bpf-next 2/3] bpf: introduce helper bpf_get_branch_snapshot

[Andrii Nakryiko]

On Tue, Aug 31, 2021 at 7:01 PM Song Liu <songliubraving@xxxxxx> wrote:
>
> Introduce bpf_get_branch_snapshot(), which allows tracing pogram to get
> branch trace from hardware (e.g. Intel LBR). To use the feature, the
> user need to create perf_event with proper branch_record filtering
> on each cpu, and then calls bpf_get_branch_snapshot in the bpf function.
> On Intel CPUs, VLBR event (raw event 0x1b00) can be use for this.
>
> Signed-off-by: Song Liu <songliubraving@xxxxxx>
> ---
>  include/uapi/linux/bpf.h       | 22 +++++++++++++++++++
>  kernel/bpf/trampoline.c        |  3 ++-
>  kernel/trace/bpf_trace.c       | 40 ++++++++++++++++++++++++++++++++++
>  tools/include/uapi/linux/bpf.h | 22 +++++++++++++++++++
>  4 files changed, 86 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 791f31dd0abee..c986e6fad5bc0 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -4877,6 +4877,27 @@ union bpf_attr {
>   *             Get the struct pt_regs associated with **task**.
>   *     Return
>   *             A pointer to struct pt_regs.
> + *
> + * long bpf_get_branch_snapshot(void *entries, u32 size, u64 flags)
> + *     Description
> + *             Get branch trace from hardware engines like Intel LBR. The
> + *             branch trace is taken soon after the trigger point of the
> + *             BPF program, so it may contain some entries after the
> + *             trigger point. The user need to filter these entries
> + *             accordingly.
> + *
> + *             The data is stored as struct perf_branch_entry into output
> + *             buffer *entries*. *size* is the size of *entries* in bytes.
> + *             *flags* is reserved for now and must be zero.
> + *
> + *     Return
> + *             On success, number of bytes written to *buf*. On error, a
> + *             negative value.
> + *
> + *             **-EINVAL** if arguments invalid or **size** not a multiple
> + *             of **sizeof**\ (**struct perf_branch_entry**\ ).
> + *
> + *             **-ENOENT** if architecture does not support branch records.
>   */
>  #define __BPF_FUNC_MAPPER(FN)          \
>         FN(unspec),                     \
> @@ -5055,6 +5076,7 @@ union bpf_attr {
>         FN(get_func_ip),                \
>         FN(get_attach_cookie),          \
>         FN(task_pt_regs),               \
> +       FN(get_branch_snapshot),        \
>         /* */
>
>  /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index fe1e857324e66..39eaaff81953d 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -10,6 +10,7 @@
>  #include <linux/rcupdate_trace.h>
>  #include <linux/rcupdate_wait.h>
>  #include <linux/module.h>
> +#include <linux/static_call.h>
>
>  /* dummy _ops. The verifier will operate on target program's ops. */
>  const struct bpf_verifier_ops bpf_extension_verifier_ops = {
> @@ -526,7 +527,7 @@ void bpf_trampoline_put(struct bpf_trampoline *tr)
>  }
>
>  #define NO_START_TIME 1
> -static u64 notrace bpf_prog_start_time(void)
> +static __always_inline u64 notrace bpf_prog_start_time(void)
>  {
>         u64 start = NO_START_TIME;
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 8e2eb950aa829..a8ec3634a3329 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1017,6 +1017,44 @@ static const struct bpf_func_proto bpf_get_attach_cookie_proto_pe = {
>         .arg1_type      = ARG_PTR_TO_CTX,
>  };
>
> +static DEFINE_PER_CPU(struct perf_branch_snapshot, bpf_perf_branch_snapshot);
> +
> +BPF_CALL_3(bpf_get_branch_snapshot, void *, buf, u32, size, u64, flags)
> +{
> +#ifndef CONFIG_X86
> +       return -ENOENT;

nit: -EOPNOTSUPP probably makes more sense for this?

> +#else
> +       static const u32 br_entry_size = sizeof(struct perf_branch_entry);
> +       u32 to_copy;
> +
> +       if (unlikely(flags))
> +               return -EINVAL;
> +
> +       if (!buf || (size % br_entry_size != 0))
> +               return -EINVAL;
> +
> +       static_call(perf_snapshot_branch_stack)(this_cpu_ptr(&bpf_perf_branch_snapshot));

First, you have four this_cpu_ptr(&bpf_perf_branch_snapshot)
invocations in this function, probably cleaner to store the pointer in
local variable?

But second, this still has the reentrancy problem, right? And further,
we copy the same LBR data twice (to per-cpu buffer and into
user-provided destination).

What if we change perf_snapshot_branch_stack signature to this:

int perf_snapshot_branch_stack(struct perf_branch_entry *entries, int
max_nr_entries);

with the semantics that it will copy only min(max_nr_entreis,
PERF_MAX_BRANCH_RECORDS) * sizeof(struct perf_branch_entry) bytes.
That way we can copy directly into a user-provided buffer with no
per-cpu storage. Of course, perf_snapshot_branch_stack will return
number of entries copied, either as return result, or if static calls
don't support that, as another int *nr_entries output argument.


> +
> +       if (this_cpu_ptr(&bpf_perf_branch_snapshot)->nr == 0)
> +               return -ENOENT;
> +
> +       to_copy = this_cpu_ptr(&bpf_perf_branch_snapshot)->nr *
> +               sizeof(struct perf_branch_entry);
> +       to_copy = min_t(u32, size, to_copy);
> +       memcpy(buf, this_cpu_ptr(&bpf_perf_branch_snapshot)->entries, to_copy);
> +
> +       return to_copy;
> +#endif
> +}
> +

[...]