[bpf-next,2/2] Added selftests to check deadlocks in queue and stack map

Commit Message

Siddharth Chintamaneni April 29, 2024, 4:56 p.m. UTC

From: Siddharth Chintamaneni <[e-mail]>

 Added selftests to check for nested deadlocks in queue
 and stack maps.

Signed-off-by: Siddharth Chintamaneni <[e-mail]>
---
 .../prog_tests/test_queue_stack_nested_map.c  | 48 ++++++++++++++
 .../bpf/progs/test_queue_stack_nested_map.c   | 62 +++++++++++++++++++
 2 files changed, 110 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/test_queue_stack_nested_map.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_queue_stack_nested_map.c

Comments

Kumar Kartikeya Dwivedi April 29, 2024, 5:54 p.m. UTC | #1

On Mon, 29 Apr 2024 at 18:57, Siddharth Chintamaneni
<[e-mail]> wrote:
>
> From: Siddharth Chintamaneni <[e-mail]>
>
>  Added selftests to check for nested deadlocks in queue
>  and stack maps.
>
> Signed-off-by: Siddharth Chintamaneni <[e-mail]>
> ---

Forgot to remind in the previous reply, but:
For the patch subject, use 'bpf:' prefix for kernel patches, and
'selftests/bpf:' for selftests, see the commit logs in bpf-next for
examples.

>  .../prog_tests/test_queue_stack_nested_map.c  | 48 ++++++++++++++
>  .../bpf/progs/test_queue_stack_nested_map.c   | 62 +++++++++++++++++++
>  2 files changed, 110 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/test_queue_stack_nested_map.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_queue_stack_nested_map.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/test_queue_stack_nested_map.c b/tools/testing/selftests/bpf/prog_tests/test_queue_stack_nested_map.c
> new file mode 100644
> index 000000000000..731e958419eb
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/test_queue_stack_nested_map.c
> @@ -0,0 +1,48 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <test_progs.h>
> +#include <network_helpers.h>
> +
> +#include "test_queue_stack_nested_map.skel.h"
> +
> +
> +static void test_map_queue_stack_nesting_success(bool is_map_queue)
> +{
> +       struct test_queue_stack_nested_map *skel;
> +       int err;
> +       int prog_fd;
> +
> +       LIBBPF_OPTS(bpf_test_run_opts, ropts);
> +
> +       skel = test_queue_stack_nested_map__open_and_load();
> +       if (!ASSERT_OK_PTR(skel, "test_queue_stack_nested_map__open_and_load"))
> +               goto out;
> +
> +       err = test_queue_stack_nested_map__attach(skel);
> +       if (!ASSERT_OK(err, "test_queue_stack_nested_map__attach"))
> +               goto out;
> +
> +       if (is_map_queue) {
> +               prog_fd = bpf_program__fd(skel->progs.test_queue_nesting);
> +               err = bpf_prog_test_run_opts(prog_fd, &ropts);
> +               ASSERT_OK(err, "test_nested_queue_map_run");

Maybe you can also check the ropts.optval to ensure we get -EBUSY?
I.e. return the value of map push/pop from the program and then check
it here?
It can be set in a global variable from the program triggering the
deadlock, and then you could return the value back as the return value
of the program.
If fentry has restrictions on the return code, you could try other
program types which work in test_run_opts (like SEC("tc")), there are
many examples in selftests using such programs.

> [...]
>
>

Siddharth Chintamaneni April 29, 2024, 6:07 p.m. UTC | #2

On Mon, 29 Apr 2024 at 13:55, Kumar Kartikeya Dwivedi <[e-mail]> wrote:
>
> On Mon, 29 Apr 2024 at 18:57, Siddharth Chintamaneni
> <[e-mail]> wrote:
> >
> > From: Siddharth Chintamaneni <[e-mail]>
> >
> >  Added selftests to check for nested deadlocks in queue
> >  and stack maps.
> >
> > Signed-off-by: Siddharth Chintamaneni <[e-mail]>
> > ---
>
> Forgot to remind in the previous reply, but:
> For the patch subject, use 'bpf:' prefix for kernel patches, and
> 'selftests/bpf:' for selftests, see the commit logs in bpf-next for
> examples.

Thanks, I will do that in the revision.

>
> >  .../prog_tests/test_queue_stack_nested_map.c  | 48 ++++++++++++++
> >  .../bpf/progs/test_queue_stack_nested_map.c   | 62 +++++++++++++++++++
> >  2 files changed, 110 insertions(+)
> >  create mode 100644 tools/testing/selftests/bpf/prog_tests/test_queue_stack_nested_map.c
> >  create mode 100644 tools/testing/selftests/bpf/progs/test_queue_stack_nested_map.c
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/test_queue_stack_nested_map.c b/tools/testing/selftests/bpf/prog_tests/test_queue_stack_nested_map.c
> > new file mode 100644
> > index 000000000000..731e958419eb
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/test_queue_stack_nested_map.c
> > @@ -0,0 +1,48 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <test_progs.h>
> > +#include <network_helpers.h>
> > +
> > +#include "test_queue_stack_nested_map.skel.h"
> > +
> > +
> > +static void test_map_queue_stack_nesting_success(bool is_map_queue)
> > +{
> > +       struct test_queue_stack_nested_map *skel;
> > +       int err;
> > +       int prog_fd;
> > +
> > +       LIBBPF_OPTS(bpf_test_run_opts, ropts);
> > +
> > +       skel = test_queue_stack_nested_map__open_and_load();
> > +       if (!ASSERT_OK_PTR(skel, "test_queue_stack_nested_map__open_and_load"))
> > +               goto out;
> > +
> > +       err = test_queue_stack_nested_map__attach(skel);
> > +       if (!ASSERT_OK(err, "test_queue_stack_nested_map__attach"))
> > +               goto out;
> > +
> > +       if (is_map_queue) {
> > +               prog_fd = bpf_program__fd(skel->progs.test_queue_nesting);
> > +               err = bpf_prog_test_run_opts(prog_fd, &ropts);
> > +               ASSERT_OK(err, "test_nested_queue_map_run");
>
> Maybe you can also check the ropts.optval to ensure we get -EBUSY?
> I.e. return the value of map push/pop from the program and then check
> it here?

make sense.I will do that.

> It can be set in a global variable from the program triggering the
> deadlock, and then you could return the value back as the return value
> of the program.
> If fentry has restrictions on the return code, you could try other
> program types which work in test_run_opts (like SEC("tc")), there are
> many examples in selftests using such programs.
>
> > [...]
> >
> >

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/test_queue_stack_nested_map.c b/tools/testing/selftests/bpf/prog_tests/test_queue_stack_nested_map.c
new file mode 100644
index 000000000000..731e958419eb
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/test_queue_stack_nested_map.c
@@ -0,0 +1,48 @@ 
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+#include <network_helpers.h>
+
+#include "test_queue_stack_nested_map.skel.h"
+
+
+static void test_map_queue_stack_nesting_success(bool is_map_queue)
+{
+	struct test_queue_stack_nested_map *skel;
+	int err;
+	int prog_fd;
+
+	LIBBPF_OPTS(bpf_test_run_opts, ropts);
+
+	skel = test_queue_stack_nested_map__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "test_queue_stack_nested_map__open_and_load"))
+		goto out;
+
+	err = test_queue_stack_nested_map__attach(skel);
+	if (!ASSERT_OK(err, "test_queue_stack_nested_map__attach"))
+		goto out;
+
+	if (is_map_queue) {
+		prog_fd = bpf_program__fd(skel->progs.test_queue_nesting);
+		err = bpf_prog_test_run_opts(prog_fd, &ropts);
+		ASSERT_OK(err, "test_nested_queue_map_run");
+	} else {
+		prog_fd = bpf_program__fd(skel->progs.test_stack_nesting);
+		err = bpf_prog_test_run_opts(prog_fd, &ropts);
+		ASSERT_OK(err, "test_nested_stack_map_run");
+	}
+
+
+
+out:
+	test_queue_stack_nested_map__destroy(skel);
+}
+
+void test_test_queue_stack_nested_map(void)
+{
+	if (test__start_subtest("map_queue_nesting"))
+		test_map_queue_stack_nesting_success(true);
+	if (test__start_subtest("map_stack_nesting"))
+		test_map_queue_stack_nesting_success(false);
+
+}
+
diff --git a/tools/testing/selftests/bpf/progs/test_queue_stack_nested_map.c b/tools/testing/selftests/bpf/progs/test_queue_stack_nested_map.c
new file mode 100644
index 000000000000..6d22016b1709
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_queue_stack_nested_map.c
@@ -0,0 +1,62 @@ 
+// SPDX-License-Identifier: GPL-2.0
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+struct {
+	__uint(type, BPF_MAP_TYPE_STACK);
+	__uint(max_entries, 32);
+	__uint(map_flags, 0);
+	__uint(key_size, 0);
+	__uint(value_size, sizeof(__u32));
+} map_stack SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_QUEUE);
+	__uint(max_entries, 32);
+	__uint(map_flags, 0);
+	__uint(key_size, 0);
+	__uint(value_size, sizeof(__u32));
+} map_queue SEC(".maps");
+
+SEC("fentry/_raw_spin_unlock_irqrestore")
+int BPF_PROG(test_stack_nesting2, raw_spinlock_t *lock, unsigned long flags)
+{
+	__u32 value = 1;
+
+	bpf_map_push_elem(&map_stack, &value, 0);
+
+	return 0;
+}
+
+SEC("fentry/bpf_fentry_test1")
+int BPF_PROG(test_stack_nesting, int a)
+{
+	__u32 value = 1;
+
+	bpf_map_push_elem(&map_stack, &value, 0);
+
+	return 0;
+}
+
+SEC("fentry/_raw_spin_unlock_irqrestore")
+int BPF_PROG(test_queue_nesting2, raw_spinlock_t *lock, unsigned long flags)
+{
+	__u32 value = 1;
+
+	bpf_map_pop_elem(&map_queue, &value);
+
+	return 0;
+}
+
+SEC("fentry/bpf_fentry_test1")
+int BPF_PROG(test_queue_nesting, int a)
+{
+	__u32 value = 1;
+
+	bpf_map_push_elem(&map_queue, &value, 0);
+
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";