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

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

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;
>> }
>>
>>