[PATCH bpf-next v1 0/2] Transfer RCU lock state across subprog calls

[PATCH bpf-next v1 0/2] Transfer RCU lock state across subprog calls

[Kumar Kartikeya Dwivedi]

David suggested during the discussion in [0] that we should handle RCU
locks in a similar fashion to spin locks where the verifier understands
when a lock held in a caller is released in callee, or lock taken in
callee is released in a caller, or the callee is called within a lock
critical section. This set extends the same semantics to RCU read locks
and adds a few selftests to verify correct behavior. This issue has also
come up for sched-ext programs.

This would now allow static subprog calls to be made without errors
within RCU read sections, for subprogs to release RCU locks of callers
and return to them, or for subprogs to take RCU lock which is later
released in the caller.

  [0]: https://lore.kernel.org/bpf/20240204120206.796412-1-memxor@gmail.com

Kumar Kartikeya Dwivedi (2):
  bpf: Transfer RCU lock state between subprog calls
  selftests/bpf: Add tests for RCU lock transfer between subprogs

 kernel/bpf/verifier.c                         |  3 +-
 .../selftests/bpf/prog_tests/rcu_read_lock.c  |  3 +
 .../selftests/bpf/progs/rcu_read_lock.c       | 64 +++++++++++++++++++
 3 files changed, 68 insertions(+), 2 deletions(-)


base-commit: 2a79690eae953daaac232f93e6c5ac47ac539f2d
-- 
2.40.1

[PATCH bpf-next v1 1/2] bpf: Transfer RCU lock state between subprog calls

[Kumar Kartikeya Dwivedi]

Allow transferring an imbalanced RCU lock state between subprog calls
during verification. This allows patterns where a subprog call returns
with an RCU lock held, or a subprog call releases an RCU lock held by
the caller. Currently, the verifier would end up complaining if the RCU
lock is not released when processing an exit from a subprog, which is
non-ideal if its execution is supposed to be enclosed in an RCU read
section of the caller.

Instead, simply only check whether we are processing exit for frame#0
and do not complain on an active RCU lock otherwise. We only need to
update the check when processing BPF_EXIT insn, as copy_verifier_state
is already set up to do the right thing.

Suggested-by: David Vernet <void@manifault.com>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 kernel/bpf/verifier.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 64fa188d00ad..993712b9996b 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -17698,8 +17698,7 @@ static int do_check(struct bpf_verifier_env *env)
 					return -EINVAL;
 				}
 
-				if (env->cur_state->active_rcu_lock &&
-				    !in_rbtree_lock_required_cb(env)) {
+				if (env->cur_state->active_rcu_lock && !env->cur_state->curframe) {
 					verbose(env, "bpf_rcu_read_unlock is missing\n");
 					return -EINVAL;
 				}
-- 
2.40.1

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

[Kumar Kartikeya Dwivedi]

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 <memxor@gmail.com>
---
 .../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

Re: [PATCH bpf-next v1 1/2] bpf: Transfer RCU lock state between subprog calls

[Yafang Shao]

On Mon, Feb 5, 2024 at 7:02 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
>
> Allow transferring an imbalanced RCU lock state between subprog calls
> during verification. This allows patterns where a subprog call returns
> with an RCU lock held, or a subprog call releases an RCU lock held by
> the caller. Currently, the verifier would end up complaining if the RCU
> lock is not released when processing an exit from a subprog, which is
> non-ideal if its execution is supposed to be enclosed in an RCU read
> section of the caller.
>
> Instead, simply only check whether we are processing exit for frame#0
> and do not complain on an active RCU lock otherwise. We only need to
> update the check when processing BPF_EXIT insn, as copy_verifier_state
> is already set up to do the right thing.
>
> Suggested-by: David Vernet <void@manifault.com>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>

Tested-by: Yafang Shao <laoar.shao@gmail.com>

> ---
>  kernel/bpf/verifier.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 64fa188d00ad..993712b9996b 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -17698,8 +17698,7 @@ static int do_check(struct bpf_verifier_env *env)
>                                         return -EINVAL;
>                                 }
>
> -                               if (env->cur_state->active_rcu_lock &&
> -                                   !in_rbtree_lock_required_cb(env)) {
> +                               if (env->cur_state->active_rcu_lock && !env->cur_state->curframe) {
>                                         verbose(env, "bpf_rcu_read_unlock is missing\n");
>                                         return -EINVAL;
>                                 }
> --
> 2.40.1
>
>


-- 
Regards
Yafang

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

[Yafang Shao]

On Mon, Feb 5, 2024 at 7:02 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> 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 <memxor@gmail.com>
> ---
>  .../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
>
>


-- 
Regards
Yafang

Re: [PATCH bpf-next v1 1/2] bpf: Transfer RCU lock state between subprog calls

[Yonghong Song]

On 2/4/24 3:02 PM, Kumar Kartikeya Dwivedi wrote:
> Allow transferring an imbalanced RCU lock state between subprog calls
> during verification. This allows patterns where a subprog call returns
> with an RCU lock held, or a subprog call releases an RCU lock held by
> the caller. Currently, the verifier would end up complaining if the RCU
> lock is not released when processing an exit from a subprog, which is
> non-ideal if its execution is supposed to be enclosed in an RCU read
> section of the caller.
>
> Instead, simply only check whether we are processing exit for frame#0
> and do not complain on an active RCU lock otherwise. We only need to
> update the check when processing BPF_EXIT insn, as copy_verifier_state
> is already set up to do the right thing.
>
> Suggested-by: David Vernet <void@manifault.com>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>

Acked-by: Yonghong Song <yonghong.song@linux.dev>

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

[Yonghong Song]

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 <memxor@gmail.com>

Acked-by: Yonghong Song <yonghong.song@linux.dev>

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

[Kumar Kartikeya Dwivedi]

On Mon, 5 Feb 2024 at 03:54, Yafang Shao <laoar.shao@gmail.com> wrote:
>
> On Mon, Feb 5, 2024 at 7:02 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> 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.

> [...]