On Mon, Mar 23, 2020 at 12:31 PM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> writes:
>
> > On Mon, Mar 23, 2020 at 4:02 AM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote:
> >>
> >> Andrii Nakryiko <andriin@xxxxxx> 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@xxxxxx>
> >> > ---
> >> > 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.
Yes, at that will probably be implementation if we go with "expected
always required" as a default. But I'm still not sure that's the right
default.
>
> -Toke
>
