On 5/8/24 5:14 PM, 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_mapmutex_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_mapreturn 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?
st_link->map is the one with __rcu tag and "!map" is tested next. I don't see how these imply the map pointer is protected by refcount. Can you explain?
+ 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?
Please monitor the bpf CI report.bpf CI complains: https://patchwork.kernel.org/project/netdevbpf/patch/20240507055600.2382627-2-thinker.li@xxxxxxxxx/
snippet of the error: Applying: bpf: enable detaching links of struct_ops objects. Applying: bpf: support epoll from bpf struct_ops links. Applying: selftests/bpf: test struct_ops with epoll Patch failed at 0004 selftests/bpf: test struct_ops with epoll
pw-bot: crbpf_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);
