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

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

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.

new_prog = enhance_prog(prog);
err = bpf_link__update_program(link, new_prog);

and have atomic replacement "just work". This obviously implies that
bpf_link__open() should use that BPF_LINK_QUERY operation I was
requesting in my comment to the previous patch :)

This will depend on which way we go with mandatory/default expected
program FD vs optional.

-Toke