| Message ID | 20240429213609.487820-7-thinker.li@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Notify user space when a struct_ops object is detached/unregisterd | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next-PR | fail | merge-conflict |
| netdev/tree_selection | success | Clearly marked for bpf-next, async |
| netdev/apply | fail | Patch does not apply to bpf-next-0 |
On Mon, Apr 29, 2024 at 2:36 PM Kui-Feng Lee <thinker.li@gmail.com> wrote: > > Verify whether a user space program is informed through epoll with EPOLLHUP > when a struct_ops object is detached or unregistered using the function > bpf_struct_ops_kvalue_unreg() or BPF_LINK_DETACH. > > Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com> > --- > .../selftests/bpf/bpf_testmod/bpf_testmod.c | 18 ++- > .../selftests/bpf/bpf_testmod/bpf_testmod.h | 1 + > .../bpf/prog_tests/test_struct_ops_module.c | 104 ++++++++++++++++++ > .../selftests/bpf/progs/struct_ops_module.c | 7 ++ > 4 files changed, 126 insertions(+), 4 deletions(-) > [...] > diff --git a/tools/testing/selftests/bpf/progs/struct_ops_module.c b/tools/testing/selftests/bpf/progs/struct_ops_module.c > index 63b065dae002..7a697a7dd0ac 100644 > --- a/tools/testing/selftests/bpf/progs/struct_ops_module.c > +++ b/tools/testing/selftests/bpf/progs/struct_ops_module.c this file is a bit overloaded with code for various subtests, it's quite hard already to follow what's going on, I suggest adding a separate .c file for this new subtest > @@ -81,3 +81,10 @@ struct bpf_testmod_ops___incompatible testmod_incompatible = { > .test_2 = (void *)test_2, > .data = 3, > }; > + > +SEC(".struct_ops.link") > +struct bpf_testmod_ops testmod_do_unreg = { > + .test_1 = (void *)test_1, > + .test_2 = (void *)test_2, > + .do_unreg = true, > +}; > -- > 2.34.1 >
On 5/1/24 10:05, Andrii Nakryiko wrote: > On Mon, Apr 29, 2024 at 2:36 PM Kui-Feng Lee <thinker.li@gmail.com> wrote: >> >> Verify whether a user space program is informed through epoll with EPOLLHUP >> when a struct_ops object is detached or unregistered using the function >> bpf_struct_ops_kvalue_unreg() or BPF_LINK_DETACH. >> >> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com> >> --- >> .../selftests/bpf/bpf_testmod/bpf_testmod.c | 18 ++- >> .../selftests/bpf/bpf_testmod/bpf_testmod.h | 1 + >> .../bpf/prog_tests/test_struct_ops_module.c | 104 ++++++++++++++++++ >> .../selftests/bpf/progs/struct_ops_module.c | 7 ++ >> 4 files changed, 126 insertions(+), 4 deletions(-) >> > > [...] > >> diff --git a/tools/testing/selftests/bpf/progs/struct_ops_module.c b/tools/testing/selftests/bpf/progs/struct_ops_module.c >> index 63b065dae002..7a697a7dd0ac 100644 >> --- a/tools/testing/selftests/bpf/progs/struct_ops_module.c >> +++ b/tools/testing/selftests/bpf/progs/struct_ops_module.c > > this file is a bit overloaded with code for various subtests, it's > quite hard already to follow what's going on, I suggest adding a > separate .c file for this new subtest Ok! > >> @@ -81,3 +81,10 @@ struct bpf_testmod_ops___incompatible testmod_incompatible = { >> .test_2 = (void *)test_2, >> .data = 3, >> }; >> + >> +SEC(".struct_ops.link") >> +struct bpf_testmod_ops testmod_do_unreg = { >> + .test_1 = (void *)test_1, >> + .test_2 = (void *)test_2, >> + .do_unreg = true, >> +}; >> -- >> 2.34.1 >>
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. 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. > + rcu_read_unlock(); > + } > + > return 0; > }
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? > >> + rcu_read_unlock(); >> + } >> + >> return 0; >> } >
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.
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.
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. >
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c index 39ad96a18123..e526ccfad8bf 100644 --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c @@ -539,16 +539,20 @@ static int bpf_testmod_ops_init_member(const struct btf_type *t, const struct btf_member *member, void *kdata, const void *udata) { + struct bpf_testmod_ops *ops = kdata; + if (member->offset == offsetof(struct bpf_testmod_ops, data) * 8) { /* For data fields, this function has to copy it and return * 1 to indicate that the data has been handled by the * struct_ops type, or the verifier will reject the map if * the value of the data field is not zero. */ - ((struct bpf_testmod_ops *)kdata)->data = ((struct bpf_testmod_ops *)udata)->data; - return 1; - } - return 0; + ops->data = ((struct bpf_testmod_ops *)udata)->data; + } else if (member->offset == offsetof(struct bpf_testmod_ops, do_unreg) * 8) { + ops->do_unreg = ((struct bpf_testmod_ops *)udata)->do_unreg; + } else + return 0; + return 1; } static const struct btf_kfunc_id_set bpf_testmod_kfunc_set = { @@ -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); + rcu_read_unlock(); + } + return 0; } diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h index 23fa1872ee67..ee8d4a2cd187 100644 --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h @@ -43,6 +43,7 @@ struct bpf_testmod_ops { int b; } unsupported; int data; + bool do_unreg; /* The following pointers are used to test the maps having multiple * pages of trampolines. diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c index 7cf2b9ddd3e1..b4d3b29114ca 100644 --- a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c +++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c @@ -3,6 +3,8 @@ #include <test_progs.h> #include <time.h> +#include <sys/epoll.h> + #include "struct_ops_module.skel.h" static void check_map_info(struct bpf_map_info *info) @@ -160,6 +162,104 @@ static void test_struct_ops_incompatible(void) struct_ops_module__destroy(skel); } +static void test_detach_link(void) +{ + struct epoll_event ev, events[2]; + struct struct_ops_module *skel; + struct bpf_link *link = NULL; + int fd, epollfd = -1, nfds; + int err; + + skel = struct_ops_module__open_and_load(); + if (!ASSERT_OK_PTR(skel, "open_and_load")) + return; + + link = bpf_map__attach_struct_ops(skel->maps.testmod_1); + if (!ASSERT_OK_PTR(link, "attach_struct_ops")) + goto cleanup; + + fd = bpf_link__fd(link); + if (!ASSERT_GE(fd, 0, "link_fd")) + goto cleanup; + + epollfd = epoll_create1(0); + if (!ASSERT_GE(epollfd, 0, "epoll_create1")) + goto cleanup; + + ev.events = EPOLLHUP; + ev.data.fd = fd; + err = epoll_ctl(epollfd, EPOLL_CTL_ADD, fd, &ev); + if (!ASSERT_OK(err, "epoll_ctl")) + goto cleanup; + + err = bpf_link__detach(link); + if (!ASSERT_OK(err, "detach_link")) + goto cleanup; + + nfds = epoll_wait(epollfd, events, 2, 500); + if (!ASSERT_EQ(nfds, 1, "epoll_wait")) + goto cleanup; + + if (!ASSERT_EQ(events[0].data.fd, fd, "epoll_wait_fd")) + goto cleanup; + if (!ASSERT_TRUE(events[0].events & EPOLLHUP, "events[0].events")) + goto cleanup; + +cleanup: + close(epollfd); + bpf_link__destroy(link); + struct_ops_module__destroy(skel); +} + +/* Test bpf_struct_ops_kvalue_unreg() */ +static void test_do_unreg(void) +{ + struct epoll_event ev, events[2]; + struct struct_ops_module *skel; + struct bpf_link *link = NULL; + int fd, epollfd = -1, nfds; + int err; + + skel = struct_ops_module__open_and_load(); + if (!ASSERT_OK_PTR(skel, "open_and_load")) + return; + + /* bpf_testmod will unregister this map immediately through the + * function bpf_struct_ops_kvalue_unreg() since "do_unreg" is true. + */ + link = bpf_map__attach_struct_ops(skel->maps.testmod_do_unreg); + if (!ASSERT_OK_PTR(link, "attach_struct_ops")) + goto cleanup; + + fd = bpf_link__fd(link); + if (!ASSERT_GE(fd, 0, "link_fd")) + goto cleanup; + + epollfd = epoll_create1(0); + if (!ASSERT_GE(epollfd, 0, "epoll_create1")) + goto cleanup; + + ev.events = EPOLLHUP; + ev.data.fd = fd; + err = epoll_ctl(epollfd, EPOLL_CTL_ADD, fd, &ev); + if (!ASSERT_OK(err, "epoll_ctl")) + goto cleanup; + + nfds = epoll_wait(epollfd, events, 2, 500); + if (!ASSERT_EQ(nfds, 1, "epoll_wait")) + goto cleanup; + + if (!ASSERT_EQ(events[0].data.fd, fd, "epoll_wait_fd")) + goto cleanup; + if (!ASSERT_TRUE(events[0].events & EPOLLHUP, "events[0].events")) + goto cleanup; + +cleanup: + close(epollfd); + bpf_link__destroy(link); + struct_ops_module__destroy(skel); +} + void serial_test_struct_ops_module(void) { if (test__start_subtest("test_struct_ops_load")) @@ -168,5 +268,9 @@ void serial_test_struct_ops_module(void) test_struct_ops_not_zeroed(); if (test__start_subtest("test_struct_ops_incompatible")) test_struct_ops_incompatible(); + if (test__start_subtest("test_detach_link")) + test_detach_link(); + if (test__start_subtest("test_do_unreg")) + test_do_unreg(); } diff --git a/tools/testing/selftests/bpf/progs/struct_ops_module.c b/tools/testing/selftests/bpf/progs/struct_ops_module.c index 63b065dae002..7a697a7dd0ac 100644 --- a/tools/testing/selftests/bpf/progs/struct_ops_module.c +++ b/tools/testing/selftests/bpf/progs/struct_ops_module.c @@ -81,3 +81,10 @@ struct bpf_testmod_ops___incompatible testmod_incompatible = { .test_2 = (void *)test_2, .data = 3, }; + +SEC(".struct_ops.link") +struct bpf_testmod_ops testmod_do_unreg = { + .test_1 = (void *)test_1, + .test_2 = (void *)test_2, + .do_unreg = true, +};
Verify whether a user space program is informed through epoll with EPOLLHUP when a struct_ops object is detached or unregistered using the function bpf_struct_ops_kvalue_unreg() or BPF_LINK_DETACH. Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com> --- .../selftests/bpf/bpf_testmod/bpf_testmod.c | 18 ++- .../selftests/bpf/bpf_testmod/bpf_testmod.h | 1 + .../bpf/prog_tests/test_struct_ops_module.c | 104 ++++++++++++++++++ .../selftests/bpf/progs/struct_ops_module.c | 7 ++ 4 files changed, 126 insertions(+), 4 deletions(-)