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.
