On Mon, Apr 10, 2023 at 6:30 PM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> On Mon, 10 Apr 2023 13:36:32 +0800
> Yafang Shao <laoar.shao@xxxxxxxxx> wrote:
>
> > 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.
>
> Right. Because you called migrate_disable() before (and protected it
> with the ftrace_test_recursion_trylock(), the second call is guaranteed
> to succeed!
>
> [1] bit = ftrace_test_recursion_trylock();
> if (bit < 0)
> return;
> migrate_disable();
> ftrace_test_recursion_trylock(bit);
>
> [..]
>
> [2] ftrace_test_recursion_trylock();
> migrate_enable();
> ftrace_test_recursion_trylock(bit);
>
> You don't even need to read the bit again, because it will be the same
> as the first call [1]. That's because it returns the recursion level
> you are in. A function will have the same recursion level through out
> the function (as long as it always calls ftrace_test_recursion_unlock()
> between them).
>
> bpf_func()
> bit = ftrace_test_recursion_trylock(); <-- OK
> migrate_disable();
> ftrace_test_recursion_unlock(bit);
>
> [..]
>
> ftrace_test_recursion_trylock(); <<-- guaranteed to be OK
> migrate_enable() <<<-- gets traced
>
> bpf_func()
> bit = ftrace_test_recursion_trylock() <-- FAILED
> if (bit < 0)
> return;
>
> ftrace_test_recursion_unlock(bit);
>
>
> See, still works!
>
> The migrate_enable() will never be called without the migrate_disable()
> as the migrate_disable() only gets called when not being recursed.
>
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 ?
> 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.
--
Regards
Yafang
