On Tue, Apr 11, 2023 at 5:35 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
>
> On Mon, Apr 10, 2023 at 10:20:31PM +0800, Yafang Shao wrote:
> > On Mon, Apr 10, 2023 at 10:12 PM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> > >
> > > On Mon, 10 Apr 2023 21:56:16 +0800
> > > Yafang Shao <laoar.shao@xxxxxxxxx> wrote:
> > >
> > > > Thanks for your explanation again.
> > > > BPF trampoline is a little special. It includes three parts, as follows,
> > > >
> > > > ret = __bpf_prog_enter();
> > > > if (ret)
> > > > prog->bpf_func();
> > > > __bpf_prog_exit();
> > > >
> > > > migrate_disable() is called in __bpf_prog_enter() and migrate_enable()
> > > > in __bpf_prog_exit():
> > > >
> > > > ret = __bpf_prog_enter();
> > > > migrate_disable();
> > > > if (ret)
> > > > prog->bpf_func();
> > > > __bpf_prog_exit();
> > > > migrate_enable();
> > > >
> > > > That said, if we haven't executed migrate_disable() in
> > > > __bpf_prog_enter(), we shouldn't execute migrate_enable() in
> > > > __bpf_prog_exit().
> > > > Can ftrace_test_recursion_trylock() be applied to this pattern ?
> > >
> > > Yes, it can! And in this you would need to not call migrate_enable()
> > > because if the trace_recursion_trylock() failed, it would prevent
> > > migrate_disable() from being called (and should not let the bpf_func() from
> > > being called either. And then the migrate_enable in __bpf_prog_exit() would
> > > need to know not to call migrate_enable() which checking the return value
> > > of ftrace_test_recursion_trylock() would give the same value as what the
> > > one before migrate_disable() had.
> > >
> >
> > That needs some changes in invoke_bpf_prog() in files
> > arch/${ARCH}/net/bpf_jit_comp.c.
> > But I will have a try. We can then remove the bpf_prog->active, that
> > will be a good cleanup as well.
>
> I was wondering if it's worth the effort to do that just to be able to attach
> bpf prog to preempt_count_add/sub and was going to suggest to add them to
> btf_id_deny as Steven pointed out earlier as possible solution
>
> but if that might turn out as alternative to prog->active, that'd be great
>
I think we can do it in two steps,
1. Fix this crash by adding preempt_count_{sub,add} into btf_id deny list.
The stable kernel may need this fix, so we'd better make it
simpler, then it can be backported easily.
2. Replace prog->active with the new
test_recursion_try_{acquire,release} introduced by Steven
That's an improvement. We can do it in a separate patchset.
WDYT?
BTW, maybe we need to add a new fentry test case to attach all
available FUNCs parsed from /sys/kernel/btf/vmlinux.
--
Regards
Yafang
