Re: [PATCH bpf-next v1 1/2] bpf: Fix sleepable context checks and remove in_sleepable helper
"Kumar Kartikeya Dwivedi" <[email protected]>
| Newsgroups | org.kernel.vger.bpf |
|---|---|
| Message-ID | <[email protected]> |
On Thu Aug 6, 2026 at 11:10 PM CEST, Eduard Zingerman wrote: > On Thu, 2026-08-06 at 18:40 +0200, Kumar Kartikeya Dwivedi wrote: > > ... > >> Several call sites incorrectly used in_sleepable() where >> in_sleepable_context() is required. This allowed sleepable programs >> in non-sleepable contexts (e.g., inside bpf_rcu_read_lock()) to >> incorrectly use sleepable iterators and kfuncs: >> >> - check_css_task_iter_allowlist() returned in_sleepable() and >> therefore allowed css_task iterator in any sleepable program, >> even inside RCU/preempt/lock/IRQ-disabled regions. Fix it to >> use in_sleepable_context(). > > ... > >> @@ -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); >> } >> } > > I don't understand this change. The comment on top of the > check_css_task_iter_allowlist() says that it is about > cgroup.c:css_set_lock, not the current context per se. I haven't looked deeply into this on whether this should change, that said the usage of in_sleepable() is obviously wrong here, in that it is not enough. > > The flags on bpf_iter_css_task_new() and bpf_iter_css_task_next() > is what should govern the decision regarding whether the function is > allowed within some kind of a critical section. > > BTF_ID_FLAGS(func, bpf_iter_css_task_new, KF_ITER_NEW) > BTF_ID_FLAGS(func, bpf_iter_css_task_next, KF_ITER_NEXT | KF_RET_NULL) > > Should these wield a KF_SLEEPABLE? > > Looking at the bpf_iter_css_task_new() body, it uses bpf_mem_alloc() > which does not sleep and css_task_iter_start() which takes and > releases a spin lock. > > Please elaborate. > Going back to https://lore.kernel.org/all/[email protected], the reasoning certainly seems a bit dubious, esp. with changes since. In any case I will take a look at it tomorrow, but I think the conversion is ok. We might also want to use a filter instead of hardcoding this into the verifier. > ...