Re: [PATCH v2 bpf-next 05/14] bpf: allow to specify user-provided context value for BPF perf links

[Yonghong Song <yhs@fb.com>]

On 7/29/21 9:31 PM, Andrii Nakryiko wrote:
> On Thu, Jul 29, 2021 at 11:00 AM Yonghong Song <yhs@fb.com> wrote:
>>
>>
>>
>> On 7/26/21 9:12 AM, Andrii Nakryiko wrote:
>>> Add ability for users to specify custom u64 value when creating BPF link for
>>> perf_event-backed BPF programs (kprobe/uprobe, perf_event, tracepoints).
>>>
>>> This is useful for cases when the same BPF program is used for attaching and
>>> processing invocation of different tracepoints/kprobes/uprobes in a generic
>>> fashion, but such that each invocation is distinguished from each other (e.g.,
>>> BPF program can look up additional information associated with a specific
>>> kernel function without having to rely on function IP lookups). This enables
>>> new use cases to be implemented simply and efficiently that previously were
>>> possible only through code generation (and thus multiple instances of almost
>>> identical BPF program) or compilation at runtime (BCC-style) on target hosts
>>> (even more expensive resource-wise). For uprobes it is not even possible in
>>> some cases to know function IP before hand (e.g., when attaching to shared
>>> library without PID filtering, in which case base load address is not known
>>> for a library).
>>>
>>> This is done by storing u64 user_ctx in struct bpf_prog_array_item,
>>> corresponding to each attached and run BPF program. Given cgroup BPF programs
>>> already use 2 8-byte pointers for their needs and cgroup BPF programs don't
>>> have (yet?) support for user_ctx, reuse that space through union of
>>> cgroup_storage and new user_ctx field.
>>>
>>> Make it available to kprobe/tracepoint BPF programs through bpf_trace_run_ctx.
>>> This is set by BPF_PROG_RUN_ARRAY, used by kprobe/uprobe/tracepoint BPF
>>> program execution code, which luckily is now also split from
>>> BPF_PROG_RUN_ARRAY_CG. This run context will be utilized by a new BPF helper
>>> giving access to this user context value from inside a BPF program. Generic
>>> perf_event BPF programs will access this value from perf_event itself through
>>> passed in BPF program context.
>>>
>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
>>> ---
>>>    drivers/media/rc/bpf-lirc.c    |  4 ++--
>>>    include/linux/bpf.h            | 16 +++++++++++++++-
>>>    include/linux/perf_event.h     |  1 +
>>>    include/linux/trace_events.h   |  6 +++---
>>>    include/uapi/linux/bpf.h       |  7 +++++++
>>>    kernel/bpf/core.c              | 29 ++++++++++++++++++-----------
>>>    kernel/bpf/syscall.c           |  2 +-
>>>    kernel/events/core.c           | 21 ++++++++++++++-------
>>>    kernel/trace/bpf_trace.c       |  8 +++++---
>>>    tools/include/uapi/linux/bpf.h |  7 +++++++
>>>    10 files changed, 73 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/drivers/media/rc/bpf-lirc.c b/drivers/media/rc/bpf-lirc.c
>>> index afae0afe3f81..7490494273e4 100644
>>> --- a/drivers/media/rc/bpf-lirc.c
>>> +++ b/drivers/media/rc/bpf-lirc.c
>>> @@ -160,7 +160,7 @@ static int lirc_bpf_attach(struct rc_dev *rcdev, struct bpf_prog *prog)
>>>                goto unlock;
>>>        }
>>>
>>> -     ret = bpf_prog_array_copy(old_array, NULL, prog, &new_array);
>>> +     ret = bpf_prog_array_copy(old_array, NULL, prog, 0, &new_array);
>>>        if (ret < 0)
>>>                goto unlock;
>>>
>> [...]
>>>    void bpf_trace_run1(struct bpf_prog *prog, u64 arg1);
>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>>> index 00b1267ab4f0..bc1fd54a8f58 100644
>>> --- a/include/uapi/linux/bpf.h
>>> +++ b/include/uapi/linux/bpf.h
>>> @@ -1448,6 +1448,13 @@ union bpf_attr {
>>>                                __aligned_u64   iter_info;      /* extra bpf_iter_link_info */
>>>                                __u32           iter_info_len;  /* iter_info length */
>>>                        };
>>> +                     struct {
>>> +                             /* black box user-provided value passed through
>>> +                              * to BPF program at the execution time and
>>> +                              * accessible through bpf_get_user_ctx() BPF helper
>>> +                              */
>>> +                             __u64           user_ctx;
>>> +                     } perf_event;
>>
>> Is it possible to fold this field into previous union?
>>
>>                   union {
>>                           __u32           target_btf_id;  /* btf_id of
>> target to attach to */
>>                           struct {
>>                                   __aligned_u64   iter_info;      /*
>> extra bpf_iter_link_info */
>>                                   __u32           iter_info_len;  /*
>> iter_info length */
>>                           };
>>                   };
>>
>>
> 
> I didn't want to do it, because different types of BPF links will
> accept this user_ctx (or now bpf_cookie). And then we'll have to have
> different locations of that field for different types of links.
> 
> For example, when/if we add this user_ctx to BPF iterator programs,
> having __u64 user_ctx in the same anonymous union will make it overlap
> with iter_info, which is a problem. So I want to have a link
> type-specific sections in LINK_CREATE command section, to allow the
> same field name at different locations.
> 
> I actually think that we should put iter_info/iter_info_len into a
> named field, like this (also added user_ctx for bpf_iter link as a
> demonstration):
> 
> struct {
>      __aligned_u64 info;
>      __u32         info_len;
>      __aligned_u64 user_ctx;  /* see how it's at a different offset
> than perf_event.user_ctx */
> } iter;
> struct {
>      __u64         user_ctx;
> } perf_event;
> 
> (of course keeping already existing fields in anonymous struct for
> backwards compatibility)

Okay, then since user_ctx may be used by many link types. How
about just with the field "user_ctx" without struct perf_event.
Sometime like

__u64	user_ctx;

instead of

struct {
	__u64	user_ctx;
} perf_event;

> 
> I decided to not do that in this patch set, though, to not distract
> from the main goal. But I think we should avoid this shared field
> "namespace" across different link types going forward.
> 
> 
>>>                };
>>>        } link_create;
>>>
>> [...]