Mathieu, can you look at this?
[ more below ] On Mon, 21 Oct 2024 18:23:47 +0000 Jordan Rife <[email protected]> wrote: > I performed a bisection and this issue starts with commit a363d27cdbc2 > ("tracing: Allow system call tracepoints to handle page faults") which > introduces this change. > > > + * > > + * With @syscall=0, the tracepoint callback array dereference is > > + * protected by disabling preemption. > > + * With @syscall=1, the tracepoint callback array dereference is > > + * protected by Tasks Trace RCU, which allows probes to handle page > > + * faults. > > */ > > #define __DO_TRACE(name, args, cond, syscall) > > \ > > do { \ > > @@ -204,11 +212,17 @@ static inline struct tracepoint > > *tracepoint_ptr_deref(tracepoint_ptr_t *p) > > if (!(cond)) \ > > return; \ > > \ > > - preempt_disable_notrace(); \ > > + if (syscall) \ > > + rcu_read_lock_trace(); \ > > + else \ > > + preempt_disable_notrace(); \ > > \ > > __DO_TRACE_CALL(name, TP_ARGS(args)); \ > > \ > > - preempt_enable_notrace(); \ > > + if (syscall) \ > > + rcu_read_unlock_trace(); \ > > + else \ > > + preempt_enable_notrace(); \ > > } while (0) > > Link: > https://lore.kernel.org/bpf/[email protected]/ > > I reproduced the bug locally by running syz-execprog inside a QEMU VM. > > > ./syz-execprog -repeat=0 -procs=5 ./repro.syz.txt > > I /think/ what is happening is that with this change preemption may now > occur leading to a scenario where the RCU grace period is insufficient > in a few places where call_rcu() is used. In other words, there are a > few scenarios where call_rcu_tasks_trace() should be used instead to > prevent a use-after-free bug when a preempted tracepoint call tries to > access a program, link, etc. that was freed. It seems the syzkaller > program induces page faults while attaching raw tracepoints to > sys_enter making preemption more likely to occur. > > kernel/tracepoint.c > =================== > > ... > > static inline void release_probes(struct tracepoint_func *old) > > { > > ... > > call_rcu(&tp_probes->rcu, rcu_free_old_probes); <-- Here Have you tried just changing this one to call_rcu_tasks_trace()? -- Steve > > ... > > } > > ... > > kernel/bpf/syscall.c > ==================== > > static void __bpf_prog_put_noref(struct bpf_prog *prog, bool deferred) > > { > > bpf_prog_kallsyms_del_all(prog); > > btf_put(prog->aux->btf); > > module_put(prog->aux->mod); > > kvfree(prog->aux->jited_linfo); > > kvfree(prog->aux->linfo); > > kfree(prog->aux->kfunc_tab); > > if (prog->aux->attach_btf) > > btf_put(prog->aux->attach_btf); > > > > if (deferred) { > > if (prog->sleepable) <------ HERE: New condition needed? > > call_rcu_tasks_trace(&prog->aux->rcu, > > __bpf_prog_put_rcu); > > else > > call_rcu(&prog->aux->rcu, __bpf_prog_put_rcu); > > } else { > > __bpf_prog_put_rcu(&prog->aux->rcu); > > } > > } > > > > static void bpf_link_free(struct bpf_link *link) > > { > > const struct bpf_link_ops *ops = link->ops; > > bool sleepable = false; > > > > bpf_link_free_id(link->id); > > if (link->prog) { > > sleepable = link->prog->sleepable; > > /* detach BPF program, clean up used resources */ > > ops->release(link); > > bpf_prog_put(link->prog); > > } > > if (ops->dealloc_deferred) { > > /* schedule BPF link deallocation; if underlying BPF program > > * is sleepable, we need to first wait for RCU tasks trace > > * sync, then go through "classic" RCU grace period > > */ > > if (prog->sleepable) <------ HERE: New condition needed? > > call_rcu_tasks_trace(&link->rcu, > > bpf_link_defer_dealloc_mult_rcu_gp); > > else > > call_rcu(&link->rcu, bpf_link_defer_dealloc_rcu_gp); > > } else if (ops->dealloc) > > ops->dealloc(link); > > } > > After patching things locally to ensure that call_rcu_tasks_trace() is > always used in these three places I was unable to induce a KASAN bug > to occur whereas before it happened pretty much every time I ran > ./sys-execprog within a minute or so. > > I'm a bit unsure about the actual conditions under which > call_rcu_tasks_trace() should be used here though. Should there perhaps > be another condition such as `preemptable` which is used to determine > if call_rcu_tasks_trace() or call_rcu() should be used to free > links/programs? Is there any harm in just using call_rcu_tasks_trace() > every time in combination with rcu_trace_implies_rcu_gp() like it is > in bpf_link_defer_dealloc_mult_rcu_gp()? > > > static void bpf_link_defer_dealloc_mult_rcu_gp(struct rcu_head *rcu)? > > { > > if (rcu_trace_implies_rcu_gp()) > > bpf_link_defer_dealloc_rcu_gp(rcu); > > else > > call_rcu(rcu, bpf_link_defer_dealloc_rcu_gp); > > } > > - Jordan
