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

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

On 5/3/24 2:34 PM, Kui-Feng Lee wrote:
> 
> 
> 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.

Yes, adding link_dealloc() (i.e. close the link) in between will be a good test too.

With or without link_dealloc()/close(), the idea is to test this race (user 
space detach and/or dealloc vs subsystem detach/unreg) or at least show how the 
subsystem should do it. I was merely suggesting to use kfunc (may be there is a 
better way and feel free to ignore). The details of the testing steps could be 
adjusted based on how patch 3 will look like.

> 
> 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 didn't meant the detach() itself is racy or not. That part is fine. It is more 
about the link that the subsystem is holding. I feel how patch 3 will look like 
may be something different from my current thinking. If this test does not make 
sense based on how patch 3 will look like, feel free to ignore also.

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