Re: [PATCH bpf-next v1 1/2] bpf: Fix sleepable context checks and remove in_sleepable helper

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Replacing `in_sleepable()` with `in_sleepable_context()` in the first sleepable check renders the second check dead code and degrades the verifier error message.

Pre-existing issues:
- [High] Incomplete fix: Bypassed sleepable context restrictions on the `css_task` iterator for LSM and TRACE_ITER BPF programs allow 'sleeping in atomic context' bugs on PREEMPT_RT.
--

--- Patch [1]: [PATCH bpf-next v1 1/2] bpf: Fix sleepable context checks and remove in_sleepable helper ---
commit fe09e0b3c58d1fc2d9c5ba46b9ee32ba789423ae
Author: Kumar Kartikeya Dwivedi <[email protected]>

bpf: Fix sleepable context checks and remove in_sleepable helper

This patch updates several call sites to use in_sleepable_context() instead
of in_sleepable() to ensure that sleepable kfuncs and iterators are rejected
when called from non-sleepable contexts, such as inside RCU read-side
critical sections. It also removes the in_sleepable helper entirely to
prevent future misuse.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index e6233c0081d10..3dadf2b33a367 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -12083,7 +12078,7 @@ static bool check_css_task_iter_allowlist(struct bpf_verifier_env *env)
>  			return true;
>  		fallthrough;
>  	default:
> -		return in_sleepable(env);
> +		return in_sleepable_context(env);
>  	}
>  }

[Severity: High]
This is a pre-existing issue, but does this bypass the newly applied sleepable
context check for LSM and TRACE_ITER programs?

Looking at the full function:

static bool check_css_task_iter_allowlist(struct bpf_verifier_env *env)
{
	enum bpf_prog_type prog_type = resolve_prog_type(env->prog);

	switch (prog_type) {
	case BPF_PROG_TYPE_LSM:
		return true;
	case BPF_PROG_TYPE_TRACING:
		if (env->prog->expected_attach_type == BPF_TRACE_ITER)
			return true;
...

If an LSM or BPF_TRACE_ITER program explicitly enters a non-sleepable
context (e.g., via bpf_preempt_disable()) and then initializes a css_task
iterator via bpf_iter_css_task_new(), they return true unconditionally.

Because the css_task iterator acquires css_set_lock (which maps to a sleepable
rt_mutex on PREEMPT_RT), could this cause a fatal lockdep splat or "sleeping
in atomic context" kernel panic on PREEMPT_RT kernels?

[ ... ]

> @@ -13090,7 +13085,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  	}
>  
>  	sleepable = bpf_is_kfunc_sleepable(&meta);
> -	if (sleepable && !in_sleepable(env)) {
> +	if (sleepable && !in_sleepable_context(env)) {
>  		verbose(env, "program must be sleepable to call sleepable kfunc %s\n", func_name);
>  		return -EACCES;
>  	}

[Severity: Medium]
Does replacing this check render the second sleepable check in this function
unreachable?

Because sleepable kfuncs are not lock-manipulation kfuncs themselves, the
context state cannot change between this first check and the second check
further down in check_kfunc_call():

	if (sleepable && !in_sleepable_context(env)) {
		verbose(env, "kernel func %s is sleepable within %s\n",
			func_name, non_sleepable_context_description(env));
		return -EACCES;
	}

If a user writes a sleepable BPF program, enters a non-sleepable context,
and then calls a sleepable kfunc, the verifier will exit at the first check
with the generic "program must be sleepable..." error.

This is factually misleading since the program is sleepable but just inside
a non-sleepable context.

Is it possible to remove the first check entirely or consolidate them so we
keep the more accurate context-aware error message?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1