Re: [PATCH bpf-next 2/4] libbpf: Add helpers for pinning bpf prog through bpf object skeleton

[Yafang Shao <laoar.shao@gmail.com>]

On Mon, Apr 25, 2022 at 9:57 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 4/23/22 4:00 PM, Yafang Shao wrote:
> > Currently there're helpers for allowing to open/load/attach BPF object
> > through BPF object skeleton. Let's also add helpers for pinning through
> > BPF object skeleton. It could simplify BPF userspace code which wants to
> > pin the progs into bpffs.
>
> Please elaborate some more on your use case/rationale for the commit message,
> do you have orchestration code that will rely on these specifically?
>

We have a bpf manager on our production environment to maintain the
bpf programs, some of which need to be pinned in bpffs, for example
tracing bpf programs, perf_event programs and other bpf hooks added by
ourselves for performance tuning.  These bpf programs don't need a
user agent, while they really work like a kernel module, that is why
we pin them. For these kinds of bpf programs, the bpf manager can help
to simplify the development and deployment.  Take the improvement on
development for example,  the user doesn't need to write userspace
code while he focuses on the kernel side only, and then bpf manager
will do all the other things. Below is a simple example,
   Step1, gen the skeleton for the user provided bpf object file,
              $ bpftool gen skeleton  test.bpf.o > simple.skel.h
   Step2, Compile the bpf object file into a runnable binary
              #include "simple.skel.h"

              #define SIMPLE_BPF_PIN(name, path)  \
              ({                                                              \
                  struct name##_bpf *obj;                      \
                  int err = 0;                                            \
                                                                              \
                  obj = name##_bpf__open();                \
                   if (!obj) {                                              \
                       err = -errno;                                    \
                       goto cleanup;                                 \
                    }                                                         \
                                                                              \
                    err = name##_bpf__load(obj);           \
                    if (err)                                                 \
                        goto cleanup;                                 \
                                                                               \
                     err = name##_bpf__attach(obj);       \
                     if (err)                                                \
                         goto cleanup;                                \
                                                                               \
                     err = name##_bpf__pin_prog(obj, path);      \
                     if (err)                                                \
                         goto cleanup;                                \
                                                                              \
                      goto end;                                         \
                                                                              \
                  cleanup:                                              \
                      name##_bpf__destroy(obj);            \
                  end:                                                     \
                      err;                                                  \
                   })

                   SIMPLE_BPF_PIN(test, "/sys/fs/bpf");

               As the userspace code of FD-based bpf objects are all
the same,  so we can abstract them as above.  The pathset means to add
the non-exist "name##_bpf__pin_prog(obj, path)" for it.

> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > ---
> >   tools/lib/bpf/libbpf.c   | 59 ++++++++++++++++++++++++++++++++++++++++
> >   tools/lib/bpf/libbpf.h   |  4 +++
> >   tools/lib/bpf/libbpf.map |  2 ++
> >   3 files changed, 65 insertions(+)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 13fcf91e9e0e..e7ed6c53c525 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -12726,6 +12726,65 @@ void bpf_object__detach_skeleton(struct bpf_object_skeleton *s)
> >       }
> >   }
> >
> > +int bpf_object__pin_skeleton_prog(struct bpf_object_skeleton *s,
> > +                               const char *path)
>
> These pin the link, not the prog itself, so the func name is a bit misleading? Also,
> what if is this needs to be more customized in future? It doesn't seem very generic.
>

Ah, it should be bpf_object__pin_skeleton_link().
I'm not sure if it can be extended to work for a non-auto-detachable
bpf program, but I know it is hard for pinning tc-bpf programs, which
is also running on our production environment, in this way.

> > +{
> > +     struct bpf_link *link;
> > +     int err;
> > +     int i;
> > +
> > +     if (!s->prog_cnt)
> > +             return libbpf_err(-EINVAL);
> > +
> > +     if (!path)
> > +             path = DEFAULT_BPFFS;
> > +
> > +     for (i = 0; i < s->prog_cnt; i++) {
> > +             char buf[PATH_MAX];
> > +             int len;
> > +
> > +             len = snprintf(buf, PATH_MAX, "%s/%s", path, s->progs[i].name);
> > +             if (len < 0) {
> > +                     err = -EINVAL;
> > +                     goto err_unpin_prog;
> > +             } else if (len >= PATH_MAX) {
> > +                     err = -ENAMETOOLONG;
> > +                     goto err_unpin_prog;
> > +             }
> > +
> > +             link = *s->progs[i].link;
> > +             if (!link) {
> > +                     err = -EINVAL;
> > +                     goto err_unpin_prog;
> > +             }
> > +
> > +             err = bpf_link__pin(link, buf);
> > +             if (err)
> > +                     goto err_unpin_prog;
> > +     }
> > +
> > +     return 0;
> > +
> > +err_unpin_prog:
> > +     bpf_object__unpin_skeleton_prog(s);
> > +
> > +     return libbpf_err(err);
> > +}
> > +
> > +void bpf_object__unpin_skeleton_prog(struct bpf_object_skeleton *s)
> > +{
> > +     struct bpf_link *link;
> > +     int i;
> > +
> > +     for (i = 0; i < s->prog_cnt; i++) {
> > +             link = *s->progs[i].link;
> > +             if (!link || !link->pin_path)
> > +                     continue;
> > +
> > +             bpf_link__unpin(link);
> > +     }
> > +}
> > +
> >   void bpf_object__destroy_skeleton(struct bpf_object_skeleton *s)
> >   {
> >       if (!s)
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index 3784867811a4..af44b0968cca 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -1427,6 +1427,10 @@ bpf_object__open_skeleton(struct bpf_object_skeleton *s,
> >   LIBBPF_API int bpf_object__load_skeleton(struct bpf_object_skeleton *s);
> >   LIBBPF_API int bpf_object__attach_skeleton(struct bpf_object_skeleton *s);
> >   LIBBPF_API void bpf_object__detach_skeleton(struct bpf_object_skeleton *s);
> > +LIBBPF_API int
> > +bpf_object__pin_skeleton_prog(struct bpf_object_skeleton *s, const char *path);
> > +LIBBPF_API void
> > +bpf_object__unpin_skeleton_prog(struct bpf_object_skeleton *s);
> >   LIBBPF_API void bpf_object__destroy_skeleton(struct bpf_object_skeleton *s);
>
> Please also add API documentation.
>

Sure.

> >   struct bpf_var_skeleton {
> > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > index 82f6d62176dd..4e3e37b84b3a 100644
> > --- a/tools/lib/bpf/libbpf.map
> > +++ b/tools/lib/bpf/libbpf.map
> > @@ -55,6 +55,8 @@ LIBBPF_0.0.1 {
> >               bpf_object__unload;
> >               bpf_object__unpin_maps;
> >               bpf_object__unpin_programs;
> > +             bpf_object__pin_skeleton_prog;
> > +             bpf_object__unpin_skeleton_prog;
>
> This would have to go under LIBBPF_0.8.0 if so.
>

Thanks, I will change it.

> >               bpf_perf_event_read_simple;
> >               bpf_prog_attach;
> >               bpf_prog_detach;
> >
>

-- 
Regards
Yafang