Re: [PATCH v2 bpf-next 06/14] bpf: add bpf_get_user_ctx() BPF helper to access user_ctx value

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Thu, Jul 29, 2021 at 11:17 AM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 7/26/21 9:12 AM, Andrii Nakryiko wrote:
> > Add new BPF helper, bpf_get_user_ctx(), which can be used by BPF programs to
> > get access to the user_ctx value, specified during BPF program attachment (BPF
> > link creation) time.
> >
> > Currently all perf_event-backed BPF program types support bpf_get_user_ctx()
> > helper. Follow-up patches will add support for fentry/fexit programs as well.
> >
> > While at it, mark bpf_tracing_func_proto() as static to make it obvious that
> > it's only used from within the kernel/trace/bpf_trace.c.
> >
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> >   include/linux/bpf.h            |  3 ---
> >   include/uapi/linux/bpf.h       | 16 ++++++++++++++++
> >   kernel/trace/bpf_trace.c       | 35 +++++++++++++++++++++++++++++++++-
> >   tools/include/uapi/linux/bpf.h | 16 ++++++++++++++++
> >   4 files changed, 66 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 74b35faf0b73..94ebedc1e13a 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -2110,9 +2110,6 @@ extern const struct bpf_func_proto bpf_btf_find_by_name_kind_proto;
> >   extern const struct bpf_func_proto bpf_sk_setsockopt_proto;
> >   extern const struct bpf_func_proto bpf_sk_getsockopt_proto;
> >
> > -const struct bpf_func_proto *bpf_tracing_func_proto(
> > -     enum bpf_func_id func_id, const struct bpf_prog *prog);
> > -
> >   const struct bpf_func_proto *tracing_prog_func_proto(
> >     enum bpf_func_id func_id, const struct bpf_prog *prog);
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index bc1fd54a8f58..96afeced3467 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -4856,6 +4856,21 @@ union bpf_attr {
> >    *          Get address of the traced function (for tracing and kprobe programs).
> >    *  Return
> >    *          Address of the traced function.
> > + *
> > + * u64 bpf_get_user_ctx(void *ctx)
> > + *   Description
> > + *           Get user_ctx value provided (optionally) during the program
> > + *           attachment. It might be different for each individual
> > + *           attachment, even if BPF program itself is the same.
> > + *           Expects BPF program context *ctx* as a first argument.
> > + *
> > + *           Supported for the following program types:
> > + *                   - kprobe/uprobe;
> > + *                   - tracepoint;
> > + *                   - perf_event.
>
> I think it is possible in the future we may need to support more
> program types with user_ctx, not just u64 but more than 64bit value.
> Should we may make this helper extensible like
>      long bpf_get_user_ctx(void *ctx, void *user_ctx, u32 user_ctx_len)
>
> The return value will 0 to be good and a negative indicating an error.
> What do you think?

I explicitly wanted to keep this user_ctx/bpf_cookie to a small fixed
size. __u64 is perfect because it's small enough to not require
dynamic memory allocation, but big enough to store any kind of index
into an array *or* user-space pointer. So if user needs more storage
than 8 bytes, they will be able to have a bigger array where
user_ctx/bpf_cookie is just an integer index or some sort of key into
hashmap, whichever is more convenient.

So I'd like to keep it lean and simple. It is already powerful enough
to support any scenario, IMO.

>
> > + *   Return
> > + *           Value specified by user at BPF link creation/attachment time
> > + *           or 0, if it was not specified.
> >    */
> >   #define __BPF_FUNC_MAPPER(FN)               \
> >       FN(unspec),                     \
> > @@ -5032,6 +5047,7 @@ union bpf_attr {
> >       FN(timer_start),                \
> >       FN(timer_cancel),               \
> >       FN(get_func_ip),                \
> > +     FN(get_user_ctx),               \
> >       /* */
> >
> >   /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index c9cf6a0d0fb3..b14978b3f6fb 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -975,7 +975,34 @@ static const struct bpf_func_proto bpf_get_func_ip_proto_kprobe = {
> >       .arg1_type      = ARG_PTR_TO_CTX,
> >   };
> >
> > -const struct bpf_func_proto *
> > +BPF_CALL_1(bpf_get_user_ctx_trace, void *, ctx)
> > +{
> > +     struct bpf_trace_run_ctx *run_ctx;
> > +
> > +     run_ctx = container_of(current->bpf_ctx, struct bpf_trace_run_ctx, run_ctx);
> > +     return run_ctx->user_ctx;
> > +}
> > +
> > +static const struct bpf_func_proto bpf_get_user_ctx_proto_trace = {
> > +     .func           = bpf_get_user_ctx_trace,
> > +     .gpl_only       = false,
> > +     .ret_type       = RET_INTEGER,
> > +     .arg1_type      = ARG_PTR_TO_CTX,
> > +};
> > +
> [...]