Re: [PATCH bpf-next 5/6] libbpf: add support for bpf_link-based cgroup attachment - Toke Høiland-Jørgensen

From: "Toke Høiland-Jørgensen" <toke@redhat.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Andrii Nakryiko <andriin@fb.com>, bpf <bpf@vger.kernel.org>,
	Networking <netdev@vger.kernel.org>,
	Alexei Starovoitov <ast@fb.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH bpf-next 5/6] libbpf: add support for bpf_link-based cgroup attachment
Date: Mon, 23 Mar 2020 20:31:03 +0100	[thread overview]
Message-ID: <87blom3m2w.fsf@toke.dk> (raw)
In-Reply-To: <CAEf4Bzbt7-A+2dH0kSAM11mjwX+ZDV8JBhJZDArAU=Q9+y79mw@mail.gmail.com>

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Mon, Mar 23, 2020 at 4:02 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Andrii Nakryiko <andriin@fb.com> writes:
>>
>> > Add bpf_program__attach_cgroup(), which uses BPF_LINK_CREATE subcommand to
>> > create an FD-based kernel bpf_link. Also add low-level bpf_link_create() API.
>> >
>> > If expected_attach_type is not specified explicitly with
>> > bpf_program__set_expected_attach_type(), libbpf will try to determine proper
>> > attach type from BPF program's section definition.
>> >
>> > Also add support for bpf_link's underlying BPF program replacement:
>> >   - unconditional through high-level bpf_link__update_program() API;
>> >   - cmpxchg-like with specifying expected current BPF program through
>> >     low-level bpf_link_update() API.
>> >
>> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>> > ---
>> >  tools/include/uapi/linux/bpf.h | 12 +++++++++
>> >  tools/lib/bpf/bpf.c            | 34 +++++++++++++++++++++++++
>> >  tools/lib/bpf/bpf.h            | 19 ++++++++++++++
>> >  tools/lib/bpf/libbpf.c         | 46 ++++++++++++++++++++++++++++++++++
>> >  tools/lib/bpf/libbpf.h         |  8 +++++-
>> >  tools/lib/bpf/libbpf.map       |  4 +++
>> >  6 files changed, 122 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
>> > index fad9f79bb8f1..fa944093f9fc 100644
>> > --- a/tools/include/uapi/linux/bpf.h
>> > +++ b/tools/include/uapi/linux/bpf.h
>> > @@ -112,6 +112,7 @@ enum bpf_cmd {
>> >       BPF_MAP_UPDATE_BATCH,
>> >       BPF_MAP_DELETE_BATCH,
>> >       BPF_LINK_CREATE,
>> > +     BPF_LINK_UPDATE,
>> >  };
>> >
>> >  enum bpf_map_type {
>> > @@ -574,6 +575,17 @@ union bpf_attr {
>> >               __u32           target_fd;      /* object to attach to */
>> >               __u32           attach_type;    /* attach type */
>> >       } link_create;
>> > +
>> > +     struct { /* struct used by BPF_LINK_UPDATE command */
>> > +             __u32           link_fd;        /* link fd */
>> > +             /* new program fd to update link with */
>> > +             __u32           new_prog_fd;
>> > +             __u32           flags;          /* extra flags */
>> > +             /* expected link's program fd; is specified only if
>> > +              * BPF_F_REPLACE flag is set in flags */
>> > +             __u32           old_prog_fd;
>> > +     } link_update;
>> > +
>> >  } __attribute__((aligned(8)));
>> >
>> >  /* The description below is an attempt at providing documentation to eBPF
>> > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
>> > index c6dafe563176..35c34fc81bd0 100644
>> > --- a/tools/lib/bpf/bpf.c
>> > +++ b/tools/lib/bpf/bpf.c
>> > @@ -584,6 +584,40 @@ int bpf_prog_detach2(int prog_fd, int target_fd, enum bpf_attach_type type)
>> >       return sys_bpf(BPF_PROG_DETACH, &attr, sizeof(attr));
>> >  }
>> >
>> > +int bpf_link_create(int prog_fd, int target_fd,
>> > +                 enum bpf_attach_type attach_type,
>> > +                 const struct bpf_link_create_opts *opts)
>> > +{
>> > +     union bpf_attr attr;
>> > +
>> > +     if (!OPTS_VALID(opts, bpf_link_create_opts))
>> > +             return -EINVAL;
>> > +
>> > +     memset(&attr, 0, sizeof(attr));
>> > +     attr.link_create.prog_fd = prog_fd;
>> > +     attr.link_create.target_fd = target_fd;
>> > +     attr.link_create.attach_type = attach_type;
>> > +
>> > +     return sys_bpf(BPF_LINK_CREATE, &attr, sizeof(attr));
>> > +}
>> > +
>> > +int bpf_link_update(int link_fd, int new_prog_fd,
>> > +                 const struct bpf_link_update_opts *opts)
>> > +{
>> > +     union bpf_attr attr;
>> > +
>> > +     if (!OPTS_VALID(opts, bpf_link_update_opts))
>> > +             return -EINVAL;
>> > +
>> > +     memset(&attr, 0, sizeof(attr));
>> > +     attr.link_update.link_fd = link_fd;
>> > +     attr.link_update.new_prog_fd = new_prog_fd;
>> > +     attr.link_update.flags = OPTS_GET(opts, flags, 0);
>> > +     attr.link_update.old_prog_fd = OPTS_GET(opts, old_prog_fd, 0);
>> > +
>> > +     return sys_bpf(BPF_LINK_UPDATE, &attr, sizeof(attr));
>> > +}
>> > +
>> >  int bpf_prog_query(int target_fd, enum bpf_attach_type type, __u32 query_flags,
>> >                  __u32 *attach_flags, __u32 *prog_ids, __u32 *prog_cnt)
>> >  {
>> > diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
>> > index b976e77316cc..46d47afdd887 100644
>> > --- a/tools/lib/bpf/bpf.h
>> > +++ b/tools/lib/bpf/bpf.h
>> > @@ -168,6 +168,25 @@ LIBBPF_API int bpf_prog_detach(int attachable_fd, enum bpf_attach_type type);
>> >  LIBBPF_API int bpf_prog_detach2(int prog_fd, int attachable_fd,
>> >                               enum bpf_attach_type type);
>> >
>> > +struct bpf_link_create_opts {
>> > +     size_t sz; /* size of this struct for forward/backward compatibility */
>> > +};
>> > +#define bpf_link_create_opts__last_field sz
>> > +
>> > +LIBBPF_API int bpf_link_create(int prog_fd, int target_fd,
>> > +                            enum bpf_attach_type attach_type,
>> > +                            const struct bpf_link_create_opts *opts);
>> > +
>> > +struct bpf_link_update_opts {
>> > +     size_t sz; /* size of this struct for forward/backward compatibility */
>> > +     __u32 flags;       /* extra flags */
>> > +     __u32 old_prog_fd; /* expected old program FD */
>> > +};
>> > +#define bpf_link_update_opts__last_field old_prog_fd
>> > +
>> > +LIBBPF_API int bpf_link_update(int link_fd, int new_prog_fd,
>> > +                            const struct bpf_link_update_opts *opts);
>> > +
>> >  struct bpf_prog_test_run_attr {
>> >       int prog_fd;
>> >       int repeat;
>> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> > index 085e41f9b68e..8b23c70033d3 100644
>> > --- a/tools/lib/bpf/libbpf.c
>> > +++ b/tools/lib/bpf/libbpf.c
>> > @@ -6951,6 +6951,12 @@ struct bpf_link {
>> >       bool disconnected;
>> >  };
>> >
>> > +/* Replace link's underlying BPF program with the new one */
>> > +int bpf_link__update_program(struct bpf_link *link, struct bpf_program *prog)
>> > +{
>> > +     return bpf_link_update(bpf_link__fd(link), bpf_program__fd(prog), NULL);
>> > +}
>>
>> I would expect bpf_link to keep track of the previous program and
>> automatically fill it in with this operation. I.e., it should be
>> possible to do something like:
>>
>> link = bpf_link__open("/sys/fs/bpf/my_link");
>> prog = bpf_link__get_prog(link);
>
> I don't think libbpf is able to construct struct bpf_program from link
> info. It can get program FD, of course, but struct bpf_program is much
> more than that and not sure kernel has all the necessary info. Some
> parts of bpf_program is coming from ELF file, which is gone by this
> time.

Hmm, sure, maybe, but it could still get enough information (such as the
prog fd, and everything returned by GET_PROG_INFO) for userspace could
do something meaningful with the result. So that would turn the above
into bpf_link__get_prog_fd(), and struct bpf_link would contain the fd
of the currently-attached program so it can be supplied in any future
replacement calls.

-Toke