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

[Kui-Feng Lee <sinquersw@gmail.com>]

On 5/8/24 17:36, Martin KaFai Lau wrote:
> 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?
> 
Ok! You are right. I confused links with maps.

>>
>>
>>>
>>>> +    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

Yes! I found it when I rebased 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);
>>>
>