On Mon, Apr 10, 2023 at 10:02 AM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> On Sun, 9 Apr 2023 22:44:37 +0800
> Yafang Shao <laoar.shao@xxxxxxxxx> wrote:
>
> > > You can use ftrace_test_recursion_trylock() before using migrate_disable()
> > > if you ensure the callback is only for fentry. Can you prepare a fentry
> > > specific callback?
> > >
> >
> > fentry uses both ftrace-based probe and text-poke-based probe.
> > ftrace_test_recursion_trylock() can avoid the recursion in the
> > ftrace-based probe. Not sure if we can split bpf_trampoline_enter into
> > two callbacks, one for ftrace and another for text poke. I will
> > analyze it. Thanks for your suggestion.
>
> ftrace_test_recusion_trylock() works with anything. What it basically
> is doing is to make sure that you do not pass that same location in the
> same context.
>
> That is, it sets a specific bit to which context it is in (normal,
> softirq, irq or NMI). If you call it again in the same context, it will
> return false (recursion detected).
>
> That is,
>
> normal:
> ftrace_test_recursion_trylock(); <- OK
>
> softirq:
> ftrace_test_recursion_trylock() <- OK
>
> hard-irq:
> ftrace_test_recusion_trylock() <- OK
> [ recusion happens ]
> ftrace_test_recursion_trylock() <- FAIL!
>
> That's because it is perfectly fine for the same code path to enter
> ftrace code in different contexts, but if it happens in the same
> context, that has to be due something calling back into itself.
>
> Thus if you had:
>
> bit = ftrace_test_recursion_trylock();
> if (bit < 0)
> return;
> migrate_disable();
> ftrace_test_test_recursion_unlock(bit);
>
> It would prevent migrate_disable from recursing.
>
> bpf_func()
> bit = ftrace_test_recursion_trylock() <<- returns 1
> migrate_disable()
> preempt_count_add()
> bpf_func()
> bit = ftrace_test_recusion_trylock() <<- returns -1
> if (bit < 0)
> return;
>
> It would prevent the crash.
>
Many thanks for the detailed explanation.
I think ftrace_test_recursion_trylock() can't apply to migreate_enable().
If we change as follows to prevent migrate_enable() from recursing,
bit = ftrace_test_recursion_trylock();
if (bit < 0)
return;
migrate_enable();
ftrace_test_recursion_unlock(bit);
We have called migrate_disable() before, so if we don't call
migrate_enable() it will cause other issues.
--
Regards
Yafang
