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

Commit Message

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

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 <[e-mail]>
Signed-off-by: Kumar Kartikeya Dwivedi <[e-mail]>
---
 kernel/bpf/verifier.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

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

On Mon, Feb 5, 2024 at 7:02 AM Kumar Kartikeya Dwivedi <[e-mail]> 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 <[e-mail]>
> Signed-off-by: Kumar Kartikeya Dwivedi <[e-mail]>

Tested-by: Yafang Shao <[e-mail]>

> ---
>  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
>
>

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

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 <[e-mail]>
> Signed-off-by: Kumar Kartikeya Dwivedi <[e-mail]>

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

Patch

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;
 				}