On Apr 24 2024, Alexei Starovoitov wrote:
On Wed, Apr 24, 2024 at 8:06 AM Alexei Starovoitov alexei.starovoitov@gmail.com wrote:
On Tue, Apr 23, 2024 at 7:55 PM Alexei Starovoitov alexei.starovoitov@gmail.com wrote:
On Sat, Apr 20, 2024 at 2:10 AM Benjamin Tissoires bentiss@kernel.org wrote:
We need to teach the verifier about the second argument which is declared as void * but which is of type KF_ARG_PTR_TO_MAP. We could have dropped this extra case if we declared the second argument as struct bpf_map *, but that means users will have to do extra casting to have their program compile.
We also need to duplicate the timer code for the checking if the map argument is matching the provided workqueue.
Signed-off-by: Benjamin Tissoires bentiss@kernel.org
FWIW, I still have one concern with this implementation:
bpf_wq_work() access ->prog without protection, but I think this might be racing with bpf_wq_set_callback(): if we have the following:
CPU 0 CPU 1 bpf_wq_set_callback() bpf_start() bpf_wq_work(): prog = cb->prog;
bpf_wq_set_callback() cb->prog = prog; bpf_prog_put(prev) rcu_assign_ptr(cb->callback_fn, callback_fn); callback = READ_ONCE(w->cb.callback_fn);
As I understand callback_fn is fine, prog might be, but we clearly have an inconstency between "prog" and "callback_fn" as they can come from 2 different bpf_wq_set_callback() calls.
IMO we should protect this by the async->lock, but I'm not sure if it's OK or not.
I see the concern, but I think it's overkill. Here 'prog' is used to pass it into __bpf_prog_enter_sleepable_recur() to keep the standard pattern of calling into sleepable prog. But it won't recurse. We can open code migrate_disable,etc from there except this_cpu_inc_return, but it's an overkill. The passed 'prog' is irrelevant. If somebody tries really hard by having two progs sharing the same map with bpf_wq and racing to set_callback... I can see how prog won't match callback, but it won't make a difference. prog is not going trigger recursion check (unless somebody tries is obsessed) and not going to UAF. I imagine it's possible to attach somewhere in core wq callback invocation path with fentry, set_callback to the same prog, and technically it's kinda sorta recursion, but different subprogs, so not a safety issue. The code as-is is fine. imo.
After sleeping on it, I realized that the use of __bpf_prog_enter_sleepable_recur() here is very much incorrect :( The tests are passing only because we don't inc prog->active when we run the prog via prog_run cmd. Adding the following: diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c index f6aad4ed2ab2..0732dfe22204 100644 --- a/net/bpf/test_run.c +++ b/net/bpf/test_run.c @@ -1514,7 +1514,9 @@ int bpf_prog_test_run_syscall(struct bpf_prog *prog, }
rcu_read_lock_trace();
this_cpu_inc_return(*(prog->active)); retval = bpf_prog_run_pin_on_cpu(prog, ctx);this_cpu_dec(*(prog->active)); rcu_read_unlock_trace();makes the test fail sporadically. Or 100% fail when the kernel is booted with 1 cpu.
Could you send a quick follow up to replace __bpf_prog_enter_sleepable_recur() with rcu_read_lock_trace(); migrate_disable(); ?
Or I'll do it in an hour or so.
Considering two broken-build reports already I've applied the following fix: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=...
that addresses the build issue on !JIT and fixes this recursion problem.
Thanks a lot for fixing this on my behalf. I was slightly puzzled by the broken-build report but really I wasn't in position to think straight yesterday (got a pretty big muscular pain in the back and had to go to the doctor to get painkillers)
I haven't forgoten about the double list walk in 9/16, I'll hopefully send the fix today.
Cheers, Benjamin