Re: [PATCH bpf-next 5/6] libbpf: add support for bpf_link-based cgroup attachment

On Mon, Mar 23, 2020 at 12:31 PM Toke Høiland-Jørgensen [off-list ref] wrote:

Andrii Nakryiko [off-list ref] writes:

quoted

On Mon, Mar 23, 2020 at 4:02 AM Toke Høiland-Jørgensen [off-list ref] wrote:

quoted

Andrii Nakryiko [off-list ref] writes:

quoted

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 <redacted>
---
 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