[bpf-next,v6,6/8] selftests/bpf: detach a struct_ops link from the subsystem managing it.

Commit Message

Kui-Feng Lee May 24, 2024, 10:30 p.m. UTC

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 <[e-mail]>
---
 .../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(+)

Comments

Martin KaFai Lau May 29, 2024, 9:51 p.m. UTC | #1

On 5/24/24 3:30 PM, Kui-Feng Lee wrote:
> @@ -832,11 +865,20 @@ static int bpf_dummy_reg(void *kdata, struct bpf_link *link)
>   	if (ops->test_2)
>   		ops->test_2(4, ops->data);
>   
> +	spin_lock(&detach_lock);
> +	if (!link_to_detach)
> +		link_to_detach = link;

bpf_testmod_ops is used in a few different tests now. Can you check if 
"./test_progs -j <num_of_parallel_workers>" will work considering link_to_detach 
here is the very first registered link.

> +	spin_unlock(&detach_lock);
> +
>   	return 0;
>   }

Martin KaFai Lau May 30, 2024, 5:53 p.m. UTC | #2

[ The mailing list got dropped in your reply, so CC back the list ]

On 5/29/24 11:05 PM, Kuifeng Lee wrote:
> On Wed, May 29, 2024 at 2:51 PM Martin KaFai Lau <[e-mail]> wrote:
>>
>> On 5/24/24 3:30 PM, Kui-Feng Lee wrote:
>>> @@ -832,11 +865,20 @@ static int bpf_dummy_reg(void *kdata, struct bpf_link *link)
>>>        if (ops->test_2)
>>>                ops->test_2(4, ops->data);
>>>
>>> +     spin_lock(&detach_lock);
>>> +     if (!link_to_detach)
>>> +             link_to_detach = link;
>>
>> bpf_testmod_ops is used in a few different tests now. Can you check if
>> "./test_progs -j <num_of_parallel_workers>" will work considering link_to_detach
>> here is the very first registered link.
> 
> Yes, it works.  Since the test in test_struct_ops_modules.c is serial,
> no other test will
> be run simultaneously. And its subtests are run one after another.

just did a quick search on "bpf_map__attach_struct_ops", how about the other 
tests like struct_ops_autocreate.c and test_struct_ops_multi_pages.c ?


> 
>>
>>> +     spin_unlock(&detach_lock);
>>> +
>>>        return 0;
>>>    }
>>

Kui-Feng Lee May 30, 2024, 7:34 p.m. UTC | #3

On Thu, May 30, 2024 at 10:53 AM Martin KaFai Lau <[e-mail]> wrote:
>
> [ The mailing list got dropped in your reply, so CC back the list ]
>
> On 5/29/24 11:05 PM, Kuifeng Lee wrote:
> > On Wed, May 29, 2024 at 2:51 PM Martin KaFai Lau <[e-mail]> wrote:
> >>
> >> On 5/24/24 3:30 PM, Kui-Feng Lee wrote:
> >>> @@ -832,11 +865,20 @@ static int bpf_dummy_reg(void *kdata, struct bpf_link *link)
> >>>        if (ops->test_2)
> >>>                ops->test_2(4, ops->data);
> >>>
> >>> +     spin_lock(&detach_lock);
> >>> +     if (!link_to_detach)
> >>> +             link_to_detach = link;
> >>
> >> bpf_testmod_ops is used in a few different tests now. Can you check if
> >> "./test_progs -j <num_of_parallel_workers>" will work considering link_to_detach
> >> here is the very first registered link.
> >
> > Yes, it works.  Since the test in test_struct_ops_modules.c is serial,
> > no other test will
> > be run simultaneously. And its subtests are run one after another.
>
> just did a quick search on "bpf_map__attach_struct_ops", how about the other
> tests like struct_ops_autocreate.c and test_struct_ops_multi_pages.c ?

Got it!
I will put all these test to serial. WDYT?

>
>
> >
> >>
> >>> +     spin_unlock(&detach_lock);
> >>> +
> >>>        return 0;
> >>>    }
> >>
>

Kui-Feng Lee May 30, 2024, 7:42 p.m. UTC | #4

On Thu, May 30, 2024 at 12:34 PM Kuifeng Lee <[e-mail]> wrote:
>
> On Thu, May 30, 2024 at 10:53 AM Martin KaFai Lau <[e-mail]> wrote:
> >
> > [ The mailing list got dropped in your reply, so CC back the list ]
> >
> > On 5/29/24 11:05 PM, Kuifeng Lee wrote:
> > > On Wed, May 29, 2024 at 2:51 PM Martin KaFai Lau <[e-mail]> wrote:
> > >>
> > >> On 5/24/24 3:30 PM, Kui-Feng Lee wrote:
> > >>> @@ -832,11 +865,20 @@ static int bpf_dummy_reg(void *kdata, struct bpf_link *link)
> > >>>        if (ops->test_2)
> > >>>                ops->test_2(4, ops->data);
> > >>>
> > >>> +     spin_lock(&detach_lock);
> > >>> +     if (!link_to_detach)
> > >>> +             link_to_detach = link;
> > >>
> > >> bpf_testmod_ops is used in a few different tests now. Can you check if
> > >> "./test_progs -j <num_of_parallel_workers>" will work considering link_to_detach
> > >> here is the very first registered link.
> > >
> > > Yes, it works.  Since the test in test_struct_ops_modules.c is serial,
> > > no other test will
> > > be run simultaneously. And its subtests are run one after another.
> >
> > just did a quick search on "bpf_map__attach_struct_ops", how about the other
> > tests like struct_ops_autocreate.c and test_struct_ops_multi_pages.c ?
>
> Got it!
> I will put all these test to serial. WDYT?

By the way, even without putting all these tests to serial, it still works.
The serial ones will be performed without other tests running at
the background. This test is the only test replying to the notification feature
so far.

>
> >
> >
> > >
> > >>
> > >>> +     spin_unlock(&detach_lock);
> > >>> +
> > >>>        return 0;
> > >>>    }
> > >>
> >

Martin KaFai Lau May 30, 2024, 8:19 p.m. UTC | #5

On 5/30/24 12:42 PM, Kuifeng Lee wrote:
> On Thu, May 30, 2024 at 12:34 PM Kuifeng Lee <[e-mail]> wrote:
>>
>> On Thu, May 30, 2024 at 10:53 AM Martin KaFai Lau <[e-mail]> wrote:
>>>
>>> [ The mailing list got dropped in your reply, so CC back the list ]
>>>
>>> On 5/29/24 11:05 PM, Kuifeng Lee wrote:
>>>> On Wed, May 29, 2024 at 2:51 PM Martin KaFai Lau <[e-mail]> wrote:
>>>>>
>>>>> On 5/24/24 3:30 PM, Kui-Feng Lee wrote:
>>>>>> @@ -832,11 +865,20 @@ static int bpf_dummy_reg(void *kdata, struct bpf_link *link)
>>>>>>         if (ops->test_2)
>>>>>>                 ops->test_2(4, ops->data);
>>>>>>
>>>>>> +     spin_lock(&detach_lock);
>>>>>> +     if (!link_to_detach)
>>>>>> +             link_to_detach = link;
>>>>>
>>>>> bpf_testmod_ops is used in a few different tests now. Can you check if
>>>>> "./test_progs -j <num_of_parallel_workers>" will work considering link_to_detach
>>>>> here is the very first registered link.
>>>>
>>>> Yes, it works.  Since the test in test_struct_ops_modules.c is serial,
>>>> no other test will
>>>> be run simultaneously. And its subtests are run one after another.
>>>
>>> just did a quick search on "bpf_map__attach_struct_ops", how about the other
>>> tests like struct_ops_autocreate.c and test_struct_ops_multi_pages.c ?
>>
>> Got it!
>> I will put all these test to serial. WDYT?

Other than slowing things down, the future new bpf_testmod_ops tests also need 
to remember to serialize. It is not good.

Put a flag in "struct bpf_testmod_ops" to flag it is testing the detach test?

> 
> By the way, even without putting all these tests to serial, it still works.
> The serial ones will be performed without other tests running at
> the background. This test is the only test replying to the notification feature
> so far.

The new detach test added in patch 6 here may work. How about other existing 
tests? afaik, the link_to_detach here could be the link belonging to other 
tests. I don't think those tests expect their link to be detached by the new 
kfunc bpf_dummy_do_link_detach.

> 
>>
>>>
>>>
>>>>
>>>>>
>>>>>> +     spin_unlock(&detach_lock);
>>>>>> +
>>>>>>         return 0;
>>>>>>     }
>>>>>
>>>

Patch

diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 0a09732cde4b..2b3a89609b7e 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -744,6 +744,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);
+
+	if (link) {
+		ret = link->ops->detach(link);
+		bpf_link_put(link);
+	}
+
+	return ret;
+}
+
 BTF_KFUNCS_START(bpf_testmod_check_kfunc_ids)
 BTF_ID_FLAGS(func, bpf_testmod_test_mod_kfunc)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test1)
@@ -780,6 +812,7 @@  BTF_ID_FLAGS(func, bpf_kfunc_call_kernel_sendmsg, KF_SLEEPABLE)
 BTF_ID_FLAGS(func, bpf_kfunc_call_sock_sendmsg, KF_SLEEPABLE)
 BTF_ID_FLAGS(func, bpf_kfunc_call_kernel_getsockname, KF_SLEEPABLE)
 BTF_ID_FLAGS(func, bpf_kfunc_call_kernel_getpeername, KF_SLEEPABLE)
+BTF_ID_FLAGS(func, bpf_dummy_do_link_detach)
 BTF_KFUNCS_END(bpf_testmod_check_kfunc_ids)
 
 static int bpf_testmod_ops_init(struct btf *btf)
@@ -832,11 +865,20 @@  static int bpf_dummy_reg(void *kdata, struct bpf_link *link)
 	if (ops->test_2)
 		ops->test_2(4, ops->data);
 
+	spin_lock(&detach_lock);
+	if (!link_to_detach)
+		link_to_detach = link;
+	spin_unlock(&detach_lock);
+
 	return 0;
 }
 
 static void bpf_dummy_unreg(void *kdata, struct bpf_link *link)
 {
+	spin_lock(&detach_lock);
+	if (link == link_to_detach)
+		link_to_detach = NULL;
+	spin_unlock(&detach_lock);
 }
 
 static int bpf_testmod_test_1(void)
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
index b0d586a6751f..19131baf4a9e 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
@@ -121,6 +121,7 @@  void bpf_kfunc_call_test_fail1(struct prog_test_fail1 *p);
 void bpf_kfunc_call_test_fail2(struct prog_test_fail2 *p);
 void bpf_kfunc_call_test_fail3(struct prog_test_fail3 *p);
 void bpf_kfunc_call_test_mem_len_fail1(void *mem, int len);
+int bpf_dummy_do_link_detach(void) __ksym;
 
 void bpf_kfunc_common_test(void) __ksym;
 
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 bbcf12696a6b..f4000bf04752 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
@@ -2,6 +2,7 @@ 
 /* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
 #include <test_progs.h>
 #include <time.h>
+#include <network_helpers.h>
 
 #include <sys/epoll.h>
 
@@ -297,6 +298,70 @@  static void test_detach_link(void)
 	struct_ops_detach__destroy(skel);
 }
 
+/* Detach a link from the subsystem that the link was registered to */
+static void test_subsystem_detach(void)
+{
+	LIBBPF_OPTS(bpf_test_run_opts, topts,
+		    .data_in = &pkt_v4,
+		    .data_size_in = sizeof(pkt_v4));
+	struct epoll_event ev, events[2];
+	struct struct_ops_detach *skel;
+	struct bpf_link *link = NULL;
+	int fd, epollfd = -1, nfds;
+	int prog_fd;
+	int err;
+
+	skel = struct_ops_detach__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "struct_ops_detach_open_and_load"))
+		return;
+
+	link = bpf_map__attach_struct_ops(skel->maps.testmod_do_detach);
+	if (!ASSERT_OK_PTR(link, "attach_struct_ops"))
+		goto cleanup;
+
+	fd = bpf_link__fd(link);
+	if (!ASSERT_GE(fd, 0, "link_fd"))
+		goto cleanup;
+
+	prog_fd = bpf_program__fd(skel->progs.start_detach);
+	if (!ASSERT_GE(prog_fd, 0, "start_detach_fd"))
+		goto cleanup;
+
+	/* Do detachment from the registered subsystem */
+	err = bpf_prog_test_run_opts(prog_fd, &topts);
+	if (!ASSERT_OK(err, "start_detach_run"))
+		goto cleanup;
+
+	if (!ASSERT_EQ(topts.retval, 0, "start_detach_run_retval"))
+		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;
+
+	/* Wait for EPOLLHUP */
+	nfds = epoll_wait(epollfd, events, 2, 5000);
+	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:
+	if (epollfd >= 0)
+		close(epollfd);
+	bpf_link__destroy(link);
+	struct_ops_detach__destroy(skel);
+}
+
 void serial_test_struct_ops_module(void)
 {
 	if (test__start_subtest("struct_ops_load"))
@@ -311,5 +376,7 @@  void serial_test_struct_ops_module(void)
 		test_struct_ops_forgotten_cb();
 	if (test__start_subtest("test_detach_link"))
 		test_detach_link();
+	if (test__start_subtest("test_subsystem_detach"))
+		test_subsystem_detach();
 }
 
diff --git a/tools/testing/selftests/bpf/progs/struct_ops_detach.c b/tools/testing/selftests/bpf/progs/struct_ops_detach.c
index 45eacc2ca657..5c742b0df04d 100644
--- a/tools/testing/selftests/bpf/progs/struct_ops_detach.c
+++ b/tools/testing/selftests/bpf/progs/struct_ops_detach.c
@@ -2,8 +2,15 @@ 
 /* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
 #include <vmlinux.h>
 #include "../bpf_testmod/bpf_testmod.h"
+#include "../bpf_testmod/bpf_testmod_kfunc.h"
 
 char _license[] SEC("license") = "GPL";
 
 SEC(".struct_ops.link")
 struct bpf_testmod_ops testmod_do_detach;
+
+SEC("tc")
+int start_detach(void *skb)
+{
+	return bpf_dummy_do_link_detach();
+}