Re: [PATCH bpf-next v2 2/6] bpf: enable detaching links of struct_ops objects.

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

 





On 5/8/24 17:14, Kui-Feng Lee wrote:


On 5/8/24 16:22, Martin KaFai Lau wrote:
On 5/6/24 10:55 PM, Kui-Feng Lee wrote:
Implement the detach callback in bpf_link_ops for struct_ops. The
subsystems that struct_ops objects are registered to can use this callback
to detach the links being passed to them.

The user space can also use the detach. The subsystem is merely reusing the similar detach callback if it stores the link during reg().

Sure!



Signed-off-by: Kui-Feng Lee <thinker.li@xxxxxxxxx>
---
  kernel/bpf/bpf_struct_ops.c | 50 ++++++++++++++++++++++++++++++++-----
  1 file changed, 44 insertions(+), 6 deletions(-)

diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 390f8c155135..bd2602982e4d 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -1057,9 +1057,6 @@ static void bpf_struct_ops_map_link_dealloc(struct bpf_link *link)
      st_map = (struct bpf_struct_ops_map *)
          rcu_dereference_protected(st_link->map, true);
      if (st_map) {
-        /* st_link->map can be NULL if
-         * bpf_struct_ops_link_create() fails to register.
-         */
          st_map->st_ops_desc->st_ops->unreg(&st_map->kvalue.data, st_link);
          bpf_map_put(&st_map->map);
      }
@@ -1075,7 +1072,8 @@ static void bpf_struct_ops_map_link_show_fdinfo(const struct bpf_link *link,
      st_link = container_of(link, struct bpf_struct_ops_link, link);
      rcu_read_lock();
      map = rcu_dereference(st_link->map);
-    seq_printf(seq, "map_id:\t%d\n", map->id);
+    if (map)
+        seq_printf(seq, "map_id:\t%d\n", map->id);
      rcu_read_unlock();
  }
@@ -1088,7 +1086,8 @@ static int bpf_struct_ops_map_link_fill_link_info(const struct bpf_link *link,
      st_link = container_of(link, struct bpf_struct_ops_link, link);
      rcu_read_lock();
      map = rcu_dereference(st_link->map);
-    info->struct_ops.map_id = map->id;
+    if (map)
+        info->struct_ops.map_id = map->id;
      rcu_read_unlock();
      return 0;
  }
@@ -1113,6 +1112,10 @@ static int bpf_struct_ops_map_link_update(struct bpf_link *link, struct bpf_map
      mutex_lock(&update_mutex);
      old_map = rcu_dereference_protected(st_link->map, lockdep_is_held(&update_mutex));
+    if (!old_map) {
+        err = -EINVAL;
+        goto err_out;
+    }
      if (expected_old_map && old_map != expected_old_map) {
          err = -EPERM;
          goto err_out;
@@ -1139,8 +1142,37 @@ static int bpf_struct_ops_map_link_update(struct bpf_link *link, struct bpf_map
      return err;
  }
+static int bpf_struct_ops_map_link_detach(struct bpf_link *link)
+{
+    struct bpf_struct_ops_link *st_link = container_of(link, struct bpf_struct_ops_link, link);
+    struct bpf_struct_ops_map *st_map;
+    struct bpf_map *map;
+
+    mutex_lock(&update_mutex);
+
+    map = rcu_dereference_protected(st_link->map, true);

nit. s/true/lockdep_is_held(&update_mutex)/


I thought it is protected by the refcount holding by the caller.
WDYT?



+    if (!map) {
+        mutex_unlock(&update_mutex);
+        return -EINVAL;
+    }
+    st_map = container_of(map, struct bpf_struct_ops_map, map);
+
+    st_map->st_ops_desc->st_ops->unreg(&st_map->kvalue.data, link);
+
+    rcu_assign_pointer(st_link->map, NULL);
+    /* Pair with bpf_map_get() in bpf_struct_ops_link_create() or
+     * bpf_map_inc() in bpf_struct_ops_map_link_update().
+     */
+    bpf_map_put(&st_map->map);
+
+    mutex_unlock(&update_mutex);
+
+    return 0;
+}
+
  static const struct bpf_link_ops bpf_struct_ops_map_lops = {
      .dealloc = bpf_struct_ops_map_link_dealloc,
+    .detach = bpf_struct_ops_map_link_detach,
      .show_fdinfo = bpf_struct_ops_map_link_show_fdinfo,
      .fill_link_info = bpf_struct_ops_map_link_fill_link_info,
      .update_map = bpf_struct_ops_map_link_update,
@@ -1176,13 +1208,19 @@ int bpf_struct_ops_link_create(union bpf_attr *attr)
      if (err)
          goto err_out;
+    /* Init link->map before calling reg() in case being detached
+     * immediately.
+     */
+    RCU_INIT_POINTER(link->map, map);
+
      err = st_map->st_ops_desc->st_ops->reg(st_map->kvalue.data, &link->link);
      if (err) {
+        rcu_assign_pointer(link->map, NULL);

nit. RCU_INIT_POINTER(link->map, NULL) is fine.

Got it!


There is a merge conflict with patch 4 also.

What do you mean here? Do you mean the patch 4 can not be applied on top
of the patch 2?

I have seen it after rebasing my local repository.



pw-bot: cr

          bpf_link_cleanup(&link_primer);
+        /* The link has been free by bpf_link_cleanup() */
          link = NULL;
          goto err_out;
      }
-    RCU_INIT_POINTER(link->map, map);
      return bpf_link_settle(&link_primer);





[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