Re: [PATCH bpf-next 6/6] selftests/bpf: test detaching struct_ops links.

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

On 5/3/24 12:15, Martin KaFai Lau wrote:
> On 5/3/24 11:34 AM, Kui-Feng Lee wrote:
>>
>>
>> On 5/2/24 11:15, Martin KaFai Lau wrote:
>>> On 4/29/24 2:36 PM, Kui-Feng Lee wrote:
>>>> @@ -572,6 +576,12 @@ static int bpf_dummy_reg(void *kdata)
>>>>       if (ops->test_2)
>>>>           ops->test_2(4, ops->data);
>>>> +    if (ops->do_unreg) {
>>>> +        rcu_read_lock();
>>>> +        bpf_struct_ops_kvalue_unreg(kdata);
>>>
>>> Instead of unreg() immediately before the reg() has returned, the 
>>> test should reflect more on how the subsystem can use it in practice. 
>>> The subsystem does not do unreg() during reg().
>>>
>>> It also needs to test a case when the link is created and 
>>> successfully registered to the subsystem. The user space does 
>>> BPF_LINK_DETACH first and  >> then the subsystem does 
>>> link->ops->detach() by itself later.
> 
>>
>> agree
>>
>>>
>>> It can create a kfunc in bpf_testmod.c to trigger the subsystem to do 
>>> link->ops->detach(). The kfunc can be called by a SEC("syscall") bpf 
>>> prog which is run by bpf_prog_test_run_opts(). The test_progs can 
>>> then decide on the timing when to do link->ops->detach() to test 
>>> different cases.
>>
>> What is the purpose of this part?
>> If it goes through link->ops->detach(), it should work just like to call
>> bpf_link_detach() twice on the same link from the user space. Do you
>> want to make sure detaching a link twice work?
> 
> It is not quite what I meant and apparently link detach twice on the 
> same valid (i.e. refcnt non zero) link won't work.
> 
> Anyhow, the idea is to show how the racing case may work in patch 3 
> (when userspace tries to detach and the subsystem tries to detach/unreg 
> itself also). I was suggesting the kfunc idea such that the test_progs 
> can have better control on the timing on when to ask the subsystem to 
> unreg/detach itself instead of having to do the unreg() during the reg() 
> as in patch 6 here. If kfunc does not make sense and there is a better 
> way to do this, feel free to ignore.
> 

Ok! I think the case you are talking more like to happen when the link
is destroyed, but bpf_struct_ops_map_link_dealloc() has not finished
yet. Calling link->ops->detach() at this point may cause a racing since
bpf_struct_ops_map_link_dealloc() doesn't acquire update_mutex.

Calling link->ops->detach() immediately after BPF_LINK_DETACH would not
cause any racing since bpf_struct_ops_map_link_detach() always acquires
update_mutex. They will be executed sequentially, and call
st_map->ops->reg() sequentially as well.

I will add a test case to call link->ops->detach() after close the fd of
the link.