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
jirka
>
> >
> > >
> > > > Note, the ftrace_test_recursion_*() code needs to be updated because it
> > > > currently does disable preemption, which it doesn't have to. And that
> > > > can cause migrate_disable() to do something different. It only disabled
> > > > preemption, as there was a time that it needed to, but now it doesn't.
> > > > But the users of it will need to be audited to make sure that they
> > > > don't need the side effect of it disabling preemption.
> > > >
> > >
> > > disabling preemption is not expected by bpf prog, so I think we should
> > > change it.
> >
> > The disabling of preemption was just done because every place that used it
> > happened to also disable preemption. So it was just a clean up, not a
> > requirement. Although the documentation said it did disable preemption :-/
> >
> > See ce5e48036c9e7 ("ftrace: disable preemption when recursion locked")
> >
> > I think I can add a ftrace_test_recursion_try_aquire() and release() that
> > is does the same thing without preemption. That way, we don't need to
> > revert that patch, and use that instead.
> >
> > -- Steve
>
>
>
> --
> Regards
> Yafang
