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

[Martin KaFai Lau <martin.lau@linux.dev>]

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@gmail.com>
>>> ---
>>>   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?

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@gmail.com/

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: 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);
>>