Re: [PATCH bpf-next v3 2/8] bpf: Create links for BPF struct_ops maps.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 3/6/23 12:23, Stanislav Fomichev wrote:
On 03/02, Kui-Feng Lee wrote:
BPF struct_ops maps are employed directly to register TCP Congestion
Control algorithms. Unlike other BPF programs that terminate when
their links gone. The link of a BPF struct_ops map provides a uniform
experience akin to other types of BPF programs.

bpf_links are responsible for registering their associated
struct_ops. You can only use a struct_ops that has the BPF_F_LINK flag
set to create a bpf_link, while a structs without this flag behaves in
the same manner as before and is registered upon updating its value.

The BPF_LINK_TYPE_STRUCT_OPS serves a dual purpose. Not only is it
used to craft the links for BPF struct_ops programs, but also to
create links for BPF struct_ops them-self.  Since the links of BPF
struct_ops programs are only used to create trampolines internally,
they are never seen in other contexts. Thus, they can be reused for
struct_ops themself.

Signed-off-by: Kui-Feng Lee <kuifeng@xxxxxxxx>
---
  include/linux/bpf.h            |  11 +++
  include/uapi/linux/bpf.h       |  12 +++-
  kernel/bpf/bpf_struct_ops.c    | 118 +++++++++++++++++++++++++++++++--
  kernel/bpf/syscall.c           |  26 +++++---
  tools/include/uapi/linux/bpf.h |  12 +++-
  5 files changed, 164 insertions(+), 15 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index cb837f42b99d..b845be719422 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1396,6 +1396,11 @@ struct bpf_link {
      struct work_struct work;
  };

+struct bpf_struct_ops_link {
+    struct bpf_link link;
+    struct bpf_map __rcu *map;
+};
+
  struct bpf_link_ops {
      void (*release)(struct bpf_link *link);
      void (*dealloc)(struct bpf_link *link);
@@ -1964,6 +1969,7 @@ int bpf_link_new_fd(struct bpf_link *link);
  struct file *bpf_link_new_file(struct bpf_link *link, int *reserved_fd);
  struct bpf_link *bpf_link_get_from_fd(u32 ufd);
  struct bpf_link *bpf_link_get_curr_or_next(u32 *id);
+int bpf_struct_ops_link_create(union bpf_attr *attr);

  int bpf_obj_pin_user(u32 ufd, const char __user *pathname);
  int bpf_obj_get_user(const char __user *pathname, int flags);
@@ -2308,6 +2314,11 @@ static inline void bpf_link_put(struct bpf_link *link)
  {
  }

+static inline int bpf_struct_ops_link_create(union bpf_attr *attr)
+{
+    return -EOPNOTSUPP;
+}
+
  static inline int bpf_obj_get_user(const char __user *pathname, int flags)
  {
      return -EOPNOTSUPP;
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 17afd2b35ee5..cd0ff39981e8 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1033,6 +1033,7 @@ enum bpf_attach_type {
      BPF_PERF_EVENT,
      BPF_TRACE_KPROBE_MULTI,
      BPF_LSM_CGROUP,
+    BPF_STRUCT_OPS,
      __MAX_BPF_ATTACH_TYPE
  };

@@ -1266,6 +1267,9 @@ enum {

  /* Create a map that is suitable to be an inner map with dynamic max entries */
      BPF_F_INNER_MAP        = (1U << 12),
+
+/* Create a map that will be registered/unregesitered by the backed bpf_link */
+    BPF_F_LINK        = (1U << 13),
  };

  /* Flags for BPF_PROG_QUERY. */
@@ -1507,7 +1511,10 @@ union bpf_attr {
      } task_fd_query;

      struct { /* struct used by BPF_LINK_CREATE command */
-        __u32        prog_fd;    /* eBPF program to attach */
+        union {
+            __u32        prog_fd;    /* eBPF program to attach */
+            __u32        map_fd;        /* eBPF struct_ops to attach */
+        };
          union {
              __u32        target_fd;    /* object to attach to */
              __u32        target_ifindex; /* target ifindex */
@@ -6354,6 +6361,9 @@ struct bpf_link_info {
          struct {
              __u32 ifindex;
          } xdp;
+        struct {
+            __u32 map_id;
+        } struct_ops;
      };
  } __attribute__((aligned(8)));

diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index bba03b6b010b..9ec675576d97 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -14,6 +14,7 @@

  enum bpf_struct_ops_state {
      BPF_STRUCT_OPS_STATE_INIT,
+    BPF_STRUCT_OPS_STATE_READY,
      BPF_STRUCT_OPS_STATE_INUSE,
      BPF_STRUCT_OPS_STATE_TOBEFREE,
  };
@@ -494,11 +495,19 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
          *(unsigned long *)(udata + moff) = prog->aux->id;
      }

-    bpf_map_inc(map);
-
      set_memory_rox((long)st_map->image, 1);
+    if (st_map->map.map_flags & BPF_F_LINK) {
+        /* Let bpf_link handle registration & unregistration.
+         *
+         * Pair with smp_load_acquire() during lookup_elem().
+         */
+        smp_store_release(&kvalue->state, BPF_STRUCT_OPS_STATE_READY);

Any reason not to create a link here and return to the user? (without
extra link_create API)


We want the ability to substitute an existing struct_ops with a different one. If we establish a link here, that implies we can't load multiple struct_ops and switch between them since the struct_ops will be registered instantly. Likewise, bpf_map_update_elem() is not supposed to generate a new FD.

+        goto unlock;
+    }
+
      err = st_ops->reg(kdata);
      if (likely(!err)) {
+        bpf_map_inc(map);
          /* Pair with smp_load_acquire() during lookup_elem().
           * It ensures the above udata updates (e.g. prog->aux->id)
           * can be seen once BPF_STRUCT_OPS_STATE_INUSE is set.
@@ -514,7 +523,6 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
       */
      set_memory_nx((long)st_map->image, 1);
      set_memory_rw((long)st_map->image, 1);
-    bpf_map_put(map);

  reset_unlock:
      bpf_struct_ops_map_put_progs(st_map);
@@ -532,10 +540,15 @@ static int bpf_struct_ops_map_delete_elem(struct bpf_map *map, void *key)
      struct bpf_struct_ops_map *st_map;

      st_map = (struct bpf_struct_ops_map *)map;
+    if (st_map->map.map_flags & BPF_F_LINK)
+        return -EOPNOTSUPP;
+
      prev_state = cmpxchg(&st_map->kvalue.state,
                   BPF_STRUCT_OPS_STATE_INUSE,
                   BPF_STRUCT_OPS_STATE_TOBEFREE);
      switch (prev_state) {
+    case BPF_STRUCT_OPS_STATE_READY:
+        return -EOPNOTSUPP;
      case BPF_STRUCT_OPS_STATE_INUSE:
          st_map->st_ops->unreg(&st_map->kvalue.data);
          bpf_map_put(map);
@@ -618,7 +631,7 @@ static void bpf_struct_ops_map_free_rcu(struct bpf_map *map)
  static int bpf_struct_ops_map_alloc_check(union bpf_attr *attr)
  {
      if (attr->key_size != sizeof(unsigned int) || attr->max_entries != 1 ||
-        attr->map_flags || !attr->btf_vmlinux_value_type_id)
+        (attr->map_flags & ~BPF_F_LINK) || !attr->btf_vmlinux_value_type_id)
          return -EINVAL;
      return 0;
  }
@@ -714,3 +727,100 @@ void bpf_struct_ops_put(const void *kdata)

      bpf_map_put(&st_map->map);
  }
+
+static void bpf_struct_ops_map_link_dealloc(struct bpf_link *link)
+{
+    struct bpf_struct_ops_link *st_link;
+    struct bpf_struct_ops_map *st_map;
+
+    st_link = container_of(link, struct bpf_struct_ops_link, link);
+    if (st_link->map) {
+        st_map = (struct bpf_struct_ops_map *)st_link->map;
+        st_map->st_ops->unreg(&st_map->kvalue.data);
+        bpf_map_put(st_link->map);
+    }
+    kfree(st_link);
+}
+
+static void bpf_struct_ops_map_link_show_fdinfo(const struct bpf_link *link,
+                        struct seq_file *seq)
+{
+    struct bpf_struct_ops_link *st_link;
+    struct bpf_map *map;
+
+    st_link = container_of(link, struct bpf_struct_ops_link, link);
+    rcu_read_lock_trace();
+    map = rcu_dereference(st_link->map);
+    if (map)
+        seq_printf(seq, "map_id:\t%d\n", map->id);
+    rcu_read_unlock_trace();
+}
+
+static int bpf_struct_ops_map_link_fill_link_info(const struct bpf_link *link,
+                           struct bpf_link_info *info)
+{
+    struct bpf_struct_ops_link *st_link;
+    struct bpf_map *map;
+
+    st_link = container_of(link, struct bpf_struct_ops_link, link);
+    rcu_read_lock_trace();
+    map = rcu_dereference(st_link->map);
+    if (map)
+        info->struct_ops.map_id = map->id;
+    rcu_read_unlock_trace();
+    return 0;
+}
+
+static const struct bpf_link_ops bpf_struct_ops_map_lops = {
+    .dealloc = bpf_struct_ops_map_link_dealloc,
+    .show_fdinfo = bpf_struct_ops_map_link_show_fdinfo,
+    .fill_link_info = bpf_struct_ops_map_link_fill_link_info,
+};
+
+int bpf_struct_ops_link_create(union bpf_attr *attr)
+{
+    struct bpf_struct_ops_link *link = NULL;
+    struct bpf_link_primer link_primer;
+    struct bpf_struct_ops_map *st_map;
+    struct bpf_map *map;
+    int err;
+
+    map = bpf_map_get(attr->link_create.map_fd);
+    if (!map)
+        return -EINVAL;
+
+    st_map = (struct bpf_struct_ops_map *)map;
+
+    if (map->map_type != BPF_MAP_TYPE_STRUCT_OPS || !(map->map_flags & BPF_F_LINK) ||
+        /* Pair with smp_store_release() during map_update */
+        smp_load_acquire(&st_map->kvalue.state) != BPF_STRUCT_OPS_STATE_READY) {
+        err = -EINVAL;
+        goto err_out;
+    }
+
+    link = kzalloc(sizeof(*link), GFP_USER);
+    if (!link) {
+        err = -ENOMEM;
+        goto err_out;
+    }
+    bpf_link_init(&link->link, BPF_LINK_TYPE_STRUCT_OPS, &bpf_struct_ops_map_lops, NULL);
+    link->map = map;
+
+    err = bpf_link_prime(&link->link, &link_primer);
+    if (err)
+        goto err_out;
+
+    err = st_map->st_ops->reg(st_map->kvalue.data);
+    if (err) {
+        bpf_link_cleanup(&link_primer);
+        goto err_out;
+    }
+
+    return bpf_link_settle(&link_primer);
+
+err_out:
+    bpf_map_put(map);
+    kfree(link);
+    return err;
+}
+
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 358a0e40555e..3db4938212d6 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2743,10 +2743,11 @@ void bpf_link_inc(struct bpf_link *link)
  static void bpf_link_free(struct bpf_link *link)
  {
      bpf_link_free_id(link->id);
+    /* detach BPF program, clean up used resources */
      if (link->prog) {
-        /* detach BPF program, clean up used resources */
          link->ops->release(link);
          bpf_prog_put(link->prog);
+        /* The struct_ops links clean up map by them-selves. */
      }
      /* free bpf_link and its containing memory */
      link->ops->dealloc(link);
@@ -2802,16 +2803,19 @@ static void bpf_link_show_fdinfo(struct seq_file *m, struct file *filp)
      const struct bpf_prog *prog = link->prog;
      char prog_tag[sizeof(prog->tag) * 2 + 1] = { };

-    bin2hex(prog_tag, prog->tag, sizeof(prog->tag));
      seq_printf(m,
             "link_type:\t%s\n"
-           "link_id:\t%u\n"
-           "prog_tag:\t%s\n"
-           "prog_id:\t%u\n",
+           "link_id:\t%u\n",
             bpf_link_type_strs[link->type],
-           link->id,
-           prog_tag,
-           prog->aux->id);
+           link->id);
+    if (prog) {
+        bin2hex(prog_tag, prog->tag, sizeof(prog->tag));
+        seq_printf(m,
+               "prog_tag:\t%s\n"
+               "prog_id:\t%u\n",
+               prog_tag,
+               prog->aux->id);
+    }
      if (link->ops->show_fdinfo)
          link->ops->show_fdinfo(link, m);
  }
@@ -4286,7 +4290,8 @@ static int bpf_link_get_info_by_fd(struct file *file,

      info.type = link->type;
      info.id = link->id;
-    info.prog_id = link->prog->aux->id;
+    if (link->prog)
+        info.prog_id = link->prog->aux->id;

      if (link->ops->fill_link_info) {
          err = link->ops->fill_link_info(link, &info);
@@ -4549,6 +4554,9 @@ static int link_create(union bpf_attr *attr, bpfptr_t uattr)
      if (CHECK_ATTR(BPF_LINK_CREATE))
          return -EINVAL;

+    if (attr->link_create.attach_type == BPF_STRUCT_OPS)
+        return bpf_struct_ops_link_create(attr);
+
      prog = bpf_prog_get(attr->link_create.prog_fd);
      if (IS_ERR(prog))
          return PTR_ERR(prog);
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 17afd2b35ee5..cd0ff39981e8 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1033,6 +1033,7 @@ enum bpf_attach_type {
      BPF_PERF_EVENT,
      BPF_TRACE_KPROBE_MULTI,
      BPF_LSM_CGROUP,
+    BPF_STRUCT_OPS,
      __MAX_BPF_ATTACH_TYPE
  };

@@ -1266,6 +1267,9 @@ enum {

  /* Create a map that is suitable to be an inner map with dynamic max entries */
      BPF_F_INNER_MAP        = (1U << 12),
+
+/* Create a map that will be registered/unregesitered by the backed bpf_link */
+    BPF_F_LINK        = (1U << 13),
  };

  /* Flags for BPF_PROG_QUERY. */
@@ -1507,7 +1511,10 @@ union bpf_attr {
      } task_fd_query;

      struct { /* struct used by BPF_LINK_CREATE command */
-        __u32        prog_fd;    /* eBPF program to attach */
+        union {
+            __u32        prog_fd;    /* eBPF program to attach */
+            __u32        map_fd;        /* eBPF struct_ops to attach */
+        };
          union {
              __u32        target_fd;    /* object to attach to */
              __u32        target_ifindex; /* target ifindex */
@@ -6354,6 +6361,9 @@ struct bpf_link_info {
          struct {
              __u32 ifindex;
          } xdp;
+        struct {
+            __u32 map_id;
+        } struct_ops;
      };
  } __attribute__((aligned(8)));

--
2.30.2




[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux