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

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

On Fri, Jul 30, 2021 at 2:34 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 7/30/21 10:48 AM, Andrii Nakryiko wrote:
> > On Thu, Jul 29, 2021 at 10:49 PM Yonghong Song <yhs@fb.com> wrote:
> >>
> >>
> >>
> >> 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.
> >
> > I'd love to do it because it is indeed generic and common field, like
> > target_fd. But I'm not sure what you are proposing below. Where
> > exactly that user_ctx (now called bpf_cookie) goes in your example? I
> > see few possible options that allow preserving ABI backwards
> > compatibility. Let's see if you and everyone else likes any of those
> > better. I'll use the full LINK_CREATE sub-struct definition from
> > bpf_attr to make it clear. And to demonstrate how this can be extended
> > to bpf_iter in the future, please note this part as this is an
> > important aspect.
> >
> > 1. Full backwards compatibility and per-link type sections (my current
> > approach):
> >
> >          struct { /* struct used by BPF_LINK_CREATE command */
> >                  __u32           prog_fd;
> >                  union {
> >                          __u32           target_fd;
> >                          __u32           target_ifindex;
> >                  };
> >                  __u32           attach_type;
> >                  __u32           flags;
> >                  union {
> >                          __u32           target_btf_id;
> >                          struct {
> >                                  __aligned_u64   iter_info;
> >                                  __u32           iter_info_len;
> >                          };
> >                          struct {
> >                                  __u64           bpf_cookie;
> >                          } perf_event;
> >                          struct {
> >                                  __aligned_u64   info;
> >                                  __u32           info_len;
> >                                  __aligned_u64   bpf_cookie;
> >                          } iter;
> >                 };
> >          } link_create;
> >
> > The good property here is that we can keep easily extending link
> > type-specific sections with extra fields where needed. For common
> > stuff like bpf_cookie it's suboptimal because we'll need to duplicate
> > field definition in each struct inside that union, but I think that's
> > fine. From end-user point of view, they will know which type of link
> > they are creating, so the use will be straightforward. This is why I
> > went with this approach. But let's consider alternatives.
> >
> > 2. Non-backwards compatible layout but extra flag to specify that new
> > field layout is used.
> >
> >          struct { /* struct used by BPF_LINK_CREATE command */
> >                  __u32           prog_fd;
> >                  union {
> >                          __u32           target_fd;
> >                          __u32           target_ifindex;
> >                  };
> >                  __u32           attach_type;
> >                  __u32           flags; /* this will start supporting
> > some new flag like BPF_F_LINK_CREATE_NEW */
> >                  __u64           bpf_cookie; /* common field now */
> >                  union { /* this parts is effectively deprecated now */
> >                          __u32           target_btf_id;
> >                          struct {
> >                                  __aligned_u64   iter_info;
> >                                  __u32           iter_info_len;
> >                          };
> >                          struct { /* this is new layout, but needs
> > BPF_F_LINK_CREATE_NEW, at least for ext/ and bpf_iter/ programs */
> >                              __u64       bpf_cookie;
> >                              union {
> >                                  struct {
> >                                      __u32     target_btf_id;
> >                                  } ext;
> >                                  struct {
> >                                      __aligned_u64 info;
> >                                      __u32         info_len;
> >                                  } iter;
> >                              }
> >                          }
> >                  };
> >          } link_create;
> >
> > This makes bpf_cookie a common field, but at least for EXT (freplace/)
> > and ITER (bpf_iter/) links we need to specify extra flag to specify
> > that we are not using iter_info/iter_info_len/target_btf_id. bpf_iter
> > then will use iter.info and iter.info_len, and can use plain
> > bpf_cookie.
> >
> > IMO, this is way too confusing and a maintainability nightmare.
> >
> > I'm trying to guess what you are proposing, I can read it two ways,
> > but let me know if I missed something.
> >
> > 3. Just add bpf_cookie field before link type-specific section.
> >
> >          struct { /* struct used by BPF_LINK_CREATE command */
> >                  __u32           prog_fd;
> >                  union {
> >                          __u32           target_fd;
> >                          __u32           target_ifindex;
> >                  };
> >                  __u32           attach_type;
> >                  __u32           flags;
> >                  __u64           bpf_cookie;  // <<<<<<<<<< HERE
> >                  union {
> >                          __u32           target_btf_id;
> >                          struct {
> >                                  __aligned_u64   iter_info;
> >                                  __u32           iter_info_len;
> >                          };
> >                  };
> >          } link_create;
> >
> > This looks really nice and would be great, but that changes offsets
> > for target_btf_id/iter_info/iter_info_len, so a no go. The only way to
> > rectify this is what proposal #2 above does with an extra flag.
> >
> > 4. Add bpf_cookie after link-type specific part:
> >
> >          struct { /* struct used by BPF_LINK_CREATE command */
> >                  __u32           prog_fd;
> >                  union {
> >                          __u32           target_fd;
> >                          __u32           target_ifindex;
> >                  };
> >                  __u32           attach_type;
> >                  __u32           flags;
> >                  union {
> >                          __u32           target_btf_id;
> >                          struct {
> >                                  __aligned_u64   iter_info;
> >                                  __u32           iter_info_len;
> >                          };
> >                          struct {
> >                  };
> >                  __u64           bpf_cookie; // <<<<<<<<<<<<<<<<<< HERE
> >          } link_create;
> >
> > This could work. But we are wasting 16 bytes currently used for
> > target_btf_id/iter_info/iter_info_len. If we later need to do
> > something link type-specific, we can add it to the existing union if
> > we need <= 16 bytes, otherwise we'll need to start another union after
> > bpf_cookie, splitting this into two link type-specific sections.
> >
> > Overall, this might work, especially assuming we won't need to extend
> > iter-specific portions. But I really hate that we didn't do named
> > structs inside that union (i.e., ext.target_btf_id and
> > iter.info/iter.info_len) and I'd like to rectify that in the follow up
> > patches with named structs duplicating existing field layout, but with
> > proper naming. But splitting this LINK_CREATE bpf_attr part into two
> > unions would make it hard and awkward in the future.
> >
> > So, thoughts? Did you have something else in mind that I missed?
>
> What I proposed is your option 4. Yes, in the future if there is there
> are something we want to add to bpf iter, we can add to iter_info, so
> it should not be an issue. Any other new link_type may utilized the same
> union with
>     struct {
>        __aligned_u64  new_type_info;
>        __u32          new_type_info_len;
>     };
> and this will put extensibility into new_type_info.
> I know this may be a little bit hassle but it should work.
>

I see what you mean. With this extra pointer we shouldn't need more
than 16 bytes per link type. That's unnecessary complication for a lot
of simpler types of links, unfortunately, though definitely an option.

We could have also done approach #4 but maybe leave 16-32 bytes before
bpf_cookie for the union, so that it's much less likely that we'll run
out of space there. Not very clean either, so I don't know.

I'll keep it here for discussion for now, let's see if anyone has
strong preferences and opinions.

> Your option 1 should work too, which is what I proposed in the beginning
> to put into the union and we can feel free to add bpf_cookie for each
> individual link type. This is actually cleaner.

Oh, you did? I must have misunderstood then. If you like approach #1,
then it's what I'm doing right now, so let's keep it as is and let's
see if anyone else has preferences.

>
> >
> >
> >> 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;
> >>>>>
> >>>> [...]