Re: [PATCH bpf-next v3 6/7] selftests/bpf: detach a struct_ops link from the subsystem managing it.

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

On 5/21/24 15:56, Amery Hung wrote:
> On Thu, May 09, 2024 at 05:29:41PM -0700, Kui-Feng Lee wrote:
>> Not only a user space program can detach a struct_ops link, the subsystem
>> managing a link can also detach the link. This patch adds a kfunc to
>> simulate detaching a link by the subsystem managing it and makes sure user
>> space programs get notified through epoll.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 42 ++++++++++++
>>   .../bpf/bpf_testmod/bpf_testmod_kfunc.h       |  1 +
>>   .../bpf/prog_tests/test_struct_ops_module.c   | 67 +++++++++++++++++++
>>   .../selftests/bpf/progs/struct_ops_detach.c   |  7 ++
>>   4 files changed, 117 insertions(+)
>>
>> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>> index 1150e758e630..1f347eed6c18 100644
>> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>> @@ -741,6 +741,38 @@ __bpf_kfunc int bpf_kfunc_call_kernel_getpeername(struct addr_args *args)
>>   	return err;
>>   }
>>   
>> +static DEFINE_SPINLOCK(detach_lock);
>> +static struct bpf_link *link_to_detach;
>> +
>> +__bpf_kfunc int bpf_dummy_do_link_detach(void)
>> +{
>> +	struct bpf_link *link;
>> +	int ret = -ENOENT;
>> +
>> +	/* A subsystem must ensure that a link is valid when detaching the
>> +	 * link. In order to achieve that, the subsystem may need to obtain
>> +	 * a lock to safeguard a table that holds the pointer to the link
>> +	 * being detached. However, the subsystem cannot invoke
>> +	 * link->ops->detach() while holding the lock because other tasks
>> +	 * may be in the process of unregistering, which could lead to
>> +	 * acquiring the same lock and causing a deadlock. This is why
>> +	 * bpf_link_inc_not_zero() is used to maintain the link's validity.
>> +	 */
>> +	spin_lock(&detach_lock);
>> +	link = link_to_detach;
>> +	/* Make sure the link is still valid by increasing its refcnt */
>> +	if (link && IS_ERR(bpf_link_inc_not_zero(link)))
>> +		link = NULL;
>> +	spin_unlock(&detach_lock);
>> +
> 
> I know it probably doesn't matter in this example, but where would you set
> link_to_detach to NULL if reg and unreg can be called multiple times?

For the same link if there is, reg() can be called only once
except if unreg() has been called for the previous reg() call on the
same link. Unreg() can only be called for once after a reg() call on the
same link.

For struct_ops map with link, unreg() is called by
bpf_struct_ops_map_link_dealloc() and bpf_struct_ops_map_link_detach().
The former one is called for a link only if the refcnt of the link has
dropped to zero. The later one is called for a link only if the refcnt
is not zero, and it holds update_mutex. Once unreg() has been called,
link->map will be cleared as well. So, unreg() should not be called
twice on the same link except it is registered again.

Does that answer your question?

> 
>> +	if (link) {
>> +		ret = link->ops->detach(link);
>> +		bpf_link_put(link);
>> +	}
>> +
>> +	return ret;
>> +}
> 
> [...]