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

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

On 5/29/24 8:04 AM, Kuifeng Lee wrote:
> On Tue, May 28, 2024 at 11:17 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>
>> On 5/24/24 3:30 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);
>>
>> update_mutex is needed to detach.
>>
>>> +
>>> +     map = rcu_dereference_protected(st_link->map, lockdep_is_held(&update_mutex));
>>> +     if (!map) {
>>> +             mutex_unlock(&update_mutex);
>>> +             return 0;
>>> +     }
>>> +     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,22 @@ 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.
>>> +      */
>>
>> With update_mutex held in link_create here, the parallel detach can still happen
>> before the link is fully initialized (the link->map pointer here in particular)?
>>
>>> +     RCU_INIT_POINTER(link->map, map);
>>> +
>>> +     mutex_lock(&update_mutex);
>>>        err = st_map->st_ops_desc->st_ops->reg(st_map->kvalue.data, &link->link);
>>>        if (err) {
>>> +             RCU_INIT_POINTER(link->map, NULL);
>>
>> I was hoping by holding the the update_mutex, it can avoid this link->map init
>> dance, like RCU_INIT_POINTER(link->map, map) above and then resetting here on
>> the error case.
>>
>>> +             mutex_unlock(&update_mutex);
>>>                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);
>>
>> If only init link->map once here like the existing code (and the init is
>> protected by the update_mutex), the subsystem should not be able to detach until
>> the link->map is fully initialized.
>>
>> or I am missing something obvious. Can you explain why this link->map init dance
>> is still needed?
> 
> Ok, I get what you mean.
> 
> I will move RCU_INIT_POINTER() back to its original place, and move the check
> on the value of "err" to the place after mutext_unlock().
The RCU_INIT_POINTER(link->map, map) needs to be done with update_mutex held and
it should be init after the err check, so the err check needs to be inside
update_mutex lock also.

Something like this (untested):

	mutex_lock(&update_mutex);

	err = st_map->st_ops_desc->st_ops->reg(st_map->kvalue.data, &link->link);
	if (err) {
		mutex_unlock(&update_mutex);
		bpf_link_cleanup(&link_primer);
		link = NULL;
		goto err_out;
	}
	RCU_INIT_POINTER(link->map, map);

	mutex_unlock(&update_mutex);


> 
>>
>>> +     mutex_unlock(&update_mutex);
>>