[v4,0/2] fix & prevent the missing preemption disabling

Message

王贇 Oct. 18, 2021, 3:38 a.m. UTC

The testing show that perf_ftrace_function_call() are using smp_processor_id()
with preemption enabled, all the checking on CPU could be wrong after preemption.

As Peter point out, the section between ftrace_test_recursion_trylock/unlock()
pair require the preemption to be disabled as 'Documentation/trace/ftrace-uses.rst'
explained, but currently the work is done outside of the helpers.

And since the internal using of trace_test_and_set_recursion()
and trace_clear_recursion() also require preemption to be disabled, we
can just merge the logical together.

Patch 1/2 will make sure preemption disabled when recursion lock succeed,
patch 2/2 will do smp_processor_id() checking after trylock() to address the
issue.

v1: https://lore.kernel.org/all/[e-mail]/
v2: https://lore.kernel.org/all/[e-mail]/
v3: https://lore.kernel.org/all/[e-mail]/

Michael Wang (2):
  ftrace: disable preemption when recursion locked
  ftrace: do CPU checking after preemption disabled

 arch/csky/kernel/probes/ftrace.c     |  2 --
 arch/parisc/kernel/ftrace.c          |  2 --
 arch/powerpc/kernel/kprobes-ftrace.c |  2 --
 arch/riscv/kernel/probes/ftrace.c    |  2 --
 arch/x86/kernel/kprobes/ftrace.c     |  2 --
 include/linux/trace_recursion.h      | 20 +++++++++++++++++++-
 kernel/livepatch/patch.c             | 13 +++++++------
 kernel/trace/ftrace.c                | 15 +++++----------
 kernel/trace/trace_event_perf.c      |  6 +++---
 kernel/trace/trace_functions.c       |  5 -----
 10 files changed, 34 insertions(+), 35 deletions(-)

Comments

王贇 Oct. 26, 2021, 2:09 a.m. UTC | #1

Just a ping, to see if there are any more comments :-P

Regards,
Michael Wang

On 2021/10/18 上午11:38, 王贇 wrote:
> The testing show that perf_ftrace_function_call() are using smp_processor_id()
> with preemption enabled, all the checking on CPU could be wrong after preemption.
> 
> As Peter point out, the section between ftrace_test_recursion_trylock/unlock()
> pair require the preemption to be disabled as 'Documentation/trace/ftrace-uses.rst'
> explained, but currently the work is done outside of the helpers.
> 
> And since the internal using of trace_test_and_set_recursion()
> and trace_clear_recursion() also require preemption to be disabled, we
> can just merge the logical together.
> 
> Patch 1/2 will make sure preemption disabled when recursion lock succeed,
> patch 2/2 will do smp_processor_id() checking after trylock() to address the
> issue.
> 
> v1: https://lore.kernel.org/all/[e-mail]/
> v2: https://lore.kernel.org/all/[e-mail]/
> v3: https://lore.kernel.org/all/[e-mail]/
> 
> Michael Wang (2):
>   ftrace: disable preemption when recursion locked
>   ftrace: do CPU checking after preemption disabled
> 
>  arch/csky/kernel/probes/ftrace.c     |  2 --
>  arch/parisc/kernel/ftrace.c          |  2 --
>  arch/powerpc/kernel/kprobes-ftrace.c |  2 --
>  arch/riscv/kernel/probes/ftrace.c    |  2 --
>  arch/x86/kernel/kprobes/ftrace.c     |  2 --
>  include/linux/trace_recursion.h      | 20 +++++++++++++++++++-
>  kernel/livepatch/patch.c             | 13 +++++++------
>  kernel/trace/ftrace.c                | 15 +++++----------
>  kernel/trace/trace_event_perf.c      |  6 +++---
>  kernel/trace/trace_functions.c       |  5 -----
>  10 files changed, 34 insertions(+), 35 deletions(-)
>

Steven Rostedt Oct. 26, 2021, 2:42 a.m. UTC | #2

On Tue, 26 Oct 2021 10:09:12 +0800
王贇 <[e-mail]> wrote:

> Just a ping, to see if there are any more comments :-P

I guess you missed what went into mainline (and your name found a bug
in my perl script for importing patches ;-)

  https://lore.kernel.org/all/[e-mail]/

Which means patch 1 needs to change:

> +	/*
> +	 * Disable preemption to fulfill the promise.
> +	 *
> +	 * Don't worry about the bit 0 cases, they indicate
> +	 * the disabling behaviour has already been done by
> +	 * internal call previously.
> +	 */
> +	preempt_disable_notrace();
> +
>  	return bit + 1;
>  }
> 
> +/*
> + * Preemption will be enabled (if it was previously enabled).
> + */
>  static __always_inline void trace_clear_recursion(int bit)
>  {
>  	if (!bit)
>  		return;
> 
> +	if (bit > 0)
> +		preempt_enable_notrace();
> +

Where this wont work anymore.

Need to preempt disable and enable always.

-- Steve


>  	barrier();
>  	bit--;
>  	trace_recursion_clear(bit);
> @@ -209,7 +227,7 @@ static __always_inline void trace_clear_recursion(int bit)
>   * tracing recursed in the same context (normal vs interrupt),
>   *

王贇 Oct. 26, 2021, 2:52 a.m. UTC | #3

On 2021/10/26 上午10:42, Steven Rostedt wrote:
> On Tue, 26 Oct 2021 10:09:12 +0800
> 王贇 <[e-mail]> wrote:
> 
>> Just a ping, to see if there are any more comments :-P
> 
> I guess you missed what went into mainline (and your name found a bug
> in my perl script for importing patches ;-)
> 
>   https://lore.kernel.org/all/[e-mail]/

Cool~ Missing some chinese font maybe, that's fine :-)

> 
> Which means patch 1 needs to change:
>> +	/*
>> +	 * Disable preemption to fulfill the promise.
>> +	 *
>> +	 * Don't worry about the bit 0 cases, they indicate
>> +	 * the disabling behaviour has already been done by
>> +	 * internal call previously.
>> +	 */
>> +	preempt_disable_notrace();
>> +
>>  	return bit + 1;
>>  }
>>
>> +/*
>> + * Preemption will be enabled (if it was previously enabled).
>> + */
>>  static __always_inline void trace_clear_recursion(int bit)
>>  {
>>  	if (!bit)
>>  		return;
>>
>> +	if (bit > 0)
>> +		preempt_enable_notrace();
>> +
> 
> Where this wont work anymore.
> 
> Need to preempt disable and enable always.

Yup, will rebase on the latest changes~

Regards,
Michael Wang

> 
> -- Steve
> 
> 
>>  	barrier();
>>  	bit--;
>>  	trace_recursion_clear(bit);
>> @@ -209,7 +227,7 @@ static __always_inline void trace_clear_recursion(int bit)
>>   * tracing recursed in the same context (normal vs interrupt),
>>   *