Re: [PATCH] tracing: Refuse fprobe if RCU is not watching

[Jiri Olsa <olsajiri@gmail.com>]

On Wed, Apr 12, 2023 at 10:37:07PM +0800, Yafang Shao wrote:
> On Tue, Apr 11, 2023 at 5:35 AM Jiri Olsa <olsajiri@gmail.com> 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@goodmis.org> wrote:
> > > >
> > > > On Mon, 10 Apr 2023 21:56:16 +0800
> > > > Yafang Shao <laoar.shao@gmail.com> 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?

sounds good

> 
> BTW, maybe we need to add a new fentry test case to attach all
> available FUNCs parsed from /sys/kernel/btf/vmlinux.

that might be tricky because we don't have multi trampoline attach
support at the moment, so it will take forever

jirka