[bpf-next,v1,2/2] selftests/bpf: Add tests for RCU lock transfer between subprogs

Commit Message

Kumar Kartikeya Dwivedi Feb. 4, 2024, 11:02 p.m. UTC

Add selftests covering the following cases:
- A static subprog called from within a RCU read section works
- A static subprog taking an RCU read lock which is released in caller works
- A static subprog releasing the caller's RCU read lock works

Signed-off-by: Kumar Kartikeya Dwivedi <[e-mail]>
---
 .../selftests/bpf/prog_tests/rcu_read_lock.c  |  3 +
 .../selftests/bpf/progs/rcu_read_lock.c       | 64 +++++++++++++++++++
 2 files changed, 67 insertions(+)

Comments

Yafang Shao Feb. 5, 2024, 2:54 a.m. UTC | #1

On Mon, Feb 5, 2024 at 7:02 AM Kumar Kartikeya Dwivedi <[e-mail]> wrote:
>
> Add selftests covering the following cases:
> - A static subprog called from within a RCU read section works
> - A static subprog taking an RCU read lock which is released in caller works
> - A static subprog releasing the caller's RCU read lock works

Given the global subprog is not allowed,  we'd better add failure
cases for a global subprog.

>
> Signed-off-by: Kumar Kartikeya Dwivedi <[e-mail]>
> ---
>  .../selftests/bpf/prog_tests/rcu_read_lock.c  |  3 +
>  .../selftests/bpf/progs/rcu_read_lock.c       | 64 +++++++++++++++++++
>  2 files changed, 67 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/rcu_read_lock.c b/tools/testing/selftests/bpf/prog_tests/rcu_read_lock.c
> index 3f1f58d3a729..328a25e031d8 100644
> --- a/tools/testing/selftests/bpf/prog_tests/rcu_read_lock.c
> +++ b/tools/testing/selftests/bpf/prog_tests/rcu_read_lock.c
> @@ -29,6 +29,9 @@ static void test_success(void)
>         bpf_program__set_autoload(skel->progs.non_sleepable_1, true);
>         bpf_program__set_autoload(skel->progs.non_sleepable_2, true);
>         bpf_program__set_autoload(skel->progs.task_trusted_non_rcuptr, true);
> +       bpf_program__set_autoload(skel->progs.rcu_read_lock_subprog, true);
> +       bpf_program__set_autoload(skel->progs.rcu_read_lock_subprog_lock, true);
> +       bpf_program__set_autoload(skel->progs.rcu_read_lock_subprog_unlock, true);
>         err = rcu_read_lock__load(skel);
>         if (!ASSERT_OK(err, "skel_load"))
>                 goto out;
> diff --git a/tools/testing/selftests/bpf/progs/rcu_read_lock.c b/tools/testing/selftests/bpf/progs/rcu_read_lock.c
> index 14fb01437fb8..687df026feb0 100644
> --- a/tools/testing/selftests/bpf/progs/rcu_read_lock.c
> +++ b/tools/testing/selftests/bpf/progs/rcu_read_lock.c
> @@ -319,3 +319,67 @@ int cross_rcu_region(void *ctx)
>         bpf_rcu_read_unlock();
>         return 0;
>  }
> +
> +__noinline
> +static int static_subprog(void *ctx)
> +{
> +       volatile int ret = 0;
> +
> +       if (bpf_get_prandom_u32())
> +               return ret + 42;
> +       return ret + bpf_get_prandom_u32();
> +}
> +
> +__noinline
> +static int static_subprog_lock(void *ctx)
> +{
> +       volatile int ret = 0;
> +
> +       bpf_rcu_read_lock();
> +       if (bpf_get_prandom_u32())
> +               return ret + 42;
> +       return ret + bpf_get_prandom_u32();
> +}
> +
> +__noinline
> +static int static_subprog_unlock(void *ctx)
> +{
> +       volatile int ret = 0;
> +
> +       bpf_rcu_read_unlock();
> +       if (bpf_get_prandom_u32())
> +               return ret + 42;
> +       return ret + bpf_get_prandom_u32();
> +}
> +
> +SEC("?fentry.s/" SYS_PREFIX "sys_getpgid")
> +int rcu_read_lock_subprog(void *ctx)
> +{
> +       volatile int ret = 0;
> +
> +       bpf_rcu_read_lock();
> +       if (bpf_get_prandom_u32())
> +               ret += static_subprog(ctx);
> +       bpf_rcu_read_unlock();
> +       return 0;
> +}
> +
> +SEC("?fentry.s/" SYS_PREFIX "sys_getpgid")
> +int rcu_read_lock_subprog_lock(void *ctx)
> +{
> +       volatile int ret = 0;
> +
> +       ret += static_subprog_lock(ctx);
> +       bpf_rcu_read_unlock();
> +       return 0;
> +}
> +
> +SEC("?fentry.s/" SYS_PREFIX "sys_getpgid")
> +int rcu_read_lock_subprog_unlock(void *ctx)
> +{
> +       volatile int ret = 0;
> +
> +       bpf_rcu_read_lock();
> +       ret += static_subprog_unlock(ctx);
> +       return 0;
> +}
> --
> 2.40.1
>
>

Yonghong Song Feb. 5, 2024, 3:13 a.m. UTC | #2

On 2/4/24 3:02 PM, Kumar Kartikeya Dwivedi wrote:
> Add selftests covering the following cases:
> - A static subprog called from within a RCU read section works
> - A static subprog taking an RCU read lock which is released in caller works
> - A static subprog releasing the caller's RCU read lock works
>
> Signed-off-by: Kumar Kartikeya Dwivedi <[e-mail]>

Acked-by: Yonghong Song <[e-mail]>

Kumar Kartikeya Dwivedi Feb. 5, 2024, 5:34 a.m. UTC | #3

On Mon, 5 Feb 2024 at 03:54, Yafang Shao <[e-mail]> wrote:
>
> On Mon, Feb 5, 2024 at 7:02 AM Kumar Kartikeya Dwivedi <[e-mail]> wrote:
> >
> > Add selftests covering the following cases:
> > - A static subprog called from within a RCU read section works
> > - A static subprog taking an RCU read lock which is released in caller works
> > - A static subprog releasing the caller's RCU read lock works
>
> Given the global subprog is not allowed,  we'd better add failure
> cases for a global subprog.
>

I will add tests for global subprogs, but just to be clear, it's not
that they are disallowed, but that the verifier won't be able to see
whether their caller holds an RCU read lock or not. Therefore, it
would be similar to how the main subprog is verified, in the verifier
state for them active_rcu_lock will be false, and whatever that
entails will follow.

They can be called within an RCU read section, but they can't leave
the lock in an imbalanced state on exit.

> [...]

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/rcu_read_lock.c b/tools/testing/selftests/bpf/prog_tests/rcu_read_lock.c
index 3f1f58d3a729..328a25e031d8 100644
--- a/tools/testing/selftests/bpf/prog_tests/rcu_read_lock.c
+++ b/tools/testing/selftests/bpf/prog_tests/rcu_read_lock.c
@@ -29,6 +29,9 @@  static void test_success(void)
 	bpf_program__set_autoload(skel->progs.non_sleepable_1, true);
 	bpf_program__set_autoload(skel->progs.non_sleepable_2, true);
 	bpf_program__set_autoload(skel->progs.task_trusted_non_rcuptr, true);
+	bpf_program__set_autoload(skel->progs.rcu_read_lock_subprog, true);
+	bpf_program__set_autoload(skel->progs.rcu_read_lock_subprog_lock, true);
+	bpf_program__set_autoload(skel->progs.rcu_read_lock_subprog_unlock, true);
 	err = rcu_read_lock__load(skel);
 	if (!ASSERT_OK(err, "skel_load"))
 		goto out;
diff --git a/tools/testing/selftests/bpf/progs/rcu_read_lock.c b/tools/testing/selftests/bpf/progs/rcu_read_lock.c
index 14fb01437fb8..687df026feb0 100644
--- a/tools/testing/selftests/bpf/progs/rcu_read_lock.c
+++ b/tools/testing/selftests/bpf/progs/rcu_read_lock.c
@@ -319,3 +319,67 @@  int cross_rcu_region(void *ctx)
 	bpf_rcu_read_unlock();
 	return 0;
 }
+
+__noinline
+static int static_subprog(void *ctx)
+{
+	volatile int ret = 0;
+
+	if (bpf_get_prandom_u32())
+		return ret + 42;
+	return ret + bpf_get_prandom_u32();
+}
+
+__noinline
+static int static_subprog_lock(void *ctx)
+{
+	volatile int ret = 0;
+
+	bpf_rcu_read_lock();
+	if (bpf_get_prandom_u32())
+		return ret + 42;
+	return ret + bpf_get_prandom_u32();
+}
+
+__noinline
+static int static_subprog_unlock(void *ctx)
+{
+	volatile int ret = 0;
+
+	bpf_rcu_read_unlock();
+	if (bpf_get_prandom_u32())
+		return ret + 42;
+	return ret + bpf_get_prandom_u32();
+}
+
+SEC("?fentry.s/" SYS_PREFIX "sys_getpgid")
+int rcu_read_lock_subprog(void *ctx)
+{
+	volatile int ret = 0;
+
+	bpf_rcu_read_lock();
+	if (bpf_get_prandom_u32())
+		ret += static_subprog(ctx);
+	bpf_rcu_read_unlock();
+	return 0;
+}
+
+SEC("?fentry.s/" SYS_PREFIX "sys_getpgid")
+int rcu_read_lock_subprog_lock(void *ctx)
+{
+	volatile int ret = 0;
+
+	ret += static_subprog_lock(ctx);
+	bpf_rcu_read_unlock();
+	return 0;
+}
+
+SEC("?fentry.s/" SYS_PREFIX "sys_getpgid")
+int rcu_read_lock_subprog_unlock(void *ctx)
+{
+	volatile int ret = 0;
+
+	bpf_rcu_read_lock();
+	ret += static_subprog_unlock(ctx);
+	return 0;
+}