On 5/21/24 12:30 AM, Kui-Feng Lee wrote:
On 5/20/24 18:22, Martin KaFai Lau wrote:
On 5/9/24 5:29 PM, Kui-Feng Lee wrote:
+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,
lockdep_is_held(&update_mutex));
+ 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.
+ */
It is not obvious in the patch how this (immediate detach by subsystem after
reg) may work without race, so I think it is easier to ask.
[ I put back the err_out context at the end ]
+ RCU_INIT_POINTER(link->map, map);
+
err = st_map->st_ops_desc->st_ops->reg(st_map->kvalue.data, &link->link);
if (err) {
+ RCU_INIT_POINTER(link->map, NULL);
In the bpf_struct_ops_map_link_detach() above, the update to link->map is
protected by the update_mutex. Could you explain how the link->map update to
NULL is safe here without holding the update_mutex?
If err is not zero, it means the subsystem rejects the pair of the
object and the link passing in. So, it has no reasonable to call
bpf_struct_ops_map_link_detach() for this link.
Does it make sense to you?
bpf_link_cleanup(&link_primer);
+ /* The link has been free by bpf_link_cleanup() */
link = NULL;
goto err_out;
}
At this point, we don't change the content of the link anymore except
changing link->fd in bpf_link_settle(). So, it should be safe to call
bpf_struct_ops_map_link_detach() from the subsystem.
Should I explain it in a comment if you think it makes sense to you?
It makes sense and will be useful to get this details in the comment.
- RCU_INIT_POINTER(link->map, map);
return bpf_link_settle(&link_primer);
err_out:
bpf_map_put(map);
kfree(link);
return err;
}
