[RFC,bpf-next,v3,02/16] bpf/verifier: introduce in_sleepable() helper

Commit Message

Benjamin Tissoires Feb. 21, 2024, 4:25 p.m. UTC

No code change, but it'll allow to have only one place to change
everything when we add in_sleepable in cur_state.

Signed-off-by: Benjamin Tissoires <[e-mail]>

---

no changes in v3

changes in v2 (compared to the one attaches to v1 0/9):
- dropped the cur_state flag, so it can be put first
---
 kernel/bpf/verifier.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

Comments

Alexei Starovoitov Feb. 23, 2024, 1:56 a.m. UTC | #1

On Wed, Feb 21, 2024 at 8:25 AM Benjamin Tissoires <[e-mail]> wrote:
> @@ -18193,7 +18198,7 @@ static int resolve_pseudo_ldimm64(struct bpf_verifier_env *env)
>                                 return -E2BIG;
>                         }
>
> -                       if (env->prog->aux->sleepable)
> +                       if (in_sleepable(env))
>                                 atomic64_inc(&map->sleepable_refcnt);

this one doesn't look correct.
The verifier didn't start its main loop when resolve_pseudo_ldimm64()
is called.
It also loses symmetry with other sleepable_refcnt operations
in syscall.c and core.c

I reverted this hunk and applied patches 1,2,3
with minor edits, like removing unnecessary parens in patch 3,
and patch subject rewords.

Benjamin Tissoires Feb. 23, 2024, 7:46 p.m. UTC | #2

On Feb 22 2024, Alexei Starovoitov wrote:
> On Wed, Feb 21, 2024 at 8:25 AM Benjamin Tissoires <[e-mail]> wrote:
> > @@ -18193,7 +18198,7 @@ static int resolve_pseudo_ldimm64(struct bpf_verifier_env *env)
> >                                 return -E2BIG;
> >                         }
> >
> > -                       if (env->prog->aux->sleepable)
> > +                       if (in_sleepable(env))
> >                                 atomic64_inc(&map->sleepable_refcnt);
> 
> this one doesn't look correct.
> The verifier didn't start its main loop when resolve_pseudo_ldimm64()
> is called.
> It also loses symmetry with other sleepable_refcnt operations
> in syscall.c and core.c
> 
> I reverted this hunk and applied patches 1,2,3
> with minor edits, like removing unnecessary parens in patch 3,
> and patch subject rewords.

Thanks a lot. I'll work on the rest of the series next week.

Cheers,
Benjamin

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 88e9d2e4c29f..7a4b19bea2ac 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5255,6 +5255,11 @@  static int map_kptr_match_type(struct bpf_verifier_env *env,
 	return -EINVAL;
 }
 
+static bool in_sleepable(struct bpf_verifier_env *env)
+{
+	return env->prog->aux->sleepable;
+}
+
 /* The non-sleepable programs and sleepable programs with explicit bpf_rcu_read_lock()
  * can dereference RCU protected pointers and result is PTR_TRUSTED.
  */
@@ -5262,7 +5267,7 @@  static bool in_rcu_cs(struct bpf_verifier_env *env)
 {
 	return env->cur_state->active_rcu_lock ||
 	       env->cur_state->active_lock.ptr ||
-	       !env->prog->aux->sleepable;
+	       !in_sleepable(env);
 }
 
 /* Once GCC supports btf_type_tag the following mechanism will be replaced with tag check */
@@ -10164,7 +10169,7 @@  static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 		return -EINVAL;
 	}
 
-	if (!env->prog->aux->sleepable && fn->might_sleep) {
+	if (!in_sleepable(env) && fn->might_sleep) {
 		verbose(env, "helper call might sleep in a non-sleepable prog\n");
 		return -EINVAL;
 	}
@@ -10194,7 +10199,7 @@  static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 			return -EINVAL;
 		}
 
-		if (env->prog->aux->sleepable && is_storage_get_function(func_id))
+		if (in_sleepable(env) && is_storage_get_function(func_id))
 			env->insn_aux_data[insn_idx].storage_get_func_atomic = true;
 	}
 
@@ -11535,7 +11540,7 @@  static bool check_css_task_iter_allowlist(struct bpf_verifier_env *env)
 			return true;
 		fallthrough;
 	default:
-		return env->prog->aux->sleepable;
+		return in_sleepable(env);
 	}
 }
 
@@ -12056,7 +12061,7 @@  static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 	}
 
 	sleepable = is_kfunc_sleepable(&meta);
-	if (sleepable && !env->prog->aux->sleepable) {
+	if (sleepable && !in_sleepable(env)) {
 		verbose(env, "program must be sleepable to call sleepable kfunc %s\n", func_name);
 		return -EACCES;
 	}
@@ -18193,7 +18198,7 @@  static int resolve_pseudo_ldimm64(struct bpf_verifier_env *env)
 				return -E2BIG;
 			}
 
-			if (env->prog->aux->sleepable)
+			if (in_sleepable(env))
 				atomic64_inc(&map->sleepable_refcnt);
 			/* hold the map. If the program is rejected by verifier,
 			 * the map will be released by release_maps() or it
@@ -19669,7 +19674,7 @@  static int do_misc_fixups(struct bpf_verifier_env *env)
 		}
 
 		if (is_storage_get_function(insn->imm)) {
-			if (!env->prog->aux->sleepable ||
+			if (!in_sleepable(env) ||
 			    env->insn_aux_data[i + delta].storage_get_func_atomic)
 				insn_buf[0] = BPF_MOV64_IMM(BPF_REG_5, (__force __s32)GFP_ATOMIC);
 			else