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

[Steven Rostedt <rostedt@goodmis.org>]

On Mon, 10 Apr 2023 13:36:32 +0800
Yafang Shao <laoar.shao@gmail.com> 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.

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.

-- Steve