trace: trace_kprobe.c always shows interrupts off

trace: trace_kprobe.c always shows interrupts off

[Mathieu Desnoyers]

Hi,

I notice that trace_kprobe.c does local_save_flags() within
__kprobe_trace_func(), which is called (at least on x86) with
interrupts always disabled. This is then used as interrupt on/off
state within the recorded event, which is misleading.

I also don't understand why arch/x86/kernel/kprobes/ftrace.c
pre handler disables interrupts, considering the following
comment above arch/x86/kernel/kprobes/core.c: kprobe_int3_handler()

 * Interrupts are disabled on entry as trap3 is an interrupt gate and they
 * remain disabled throughout this function.

A struct pt_regs is received by this function, but I don't see
any way to get the state of irq enable/disable from struct pt_regs
across architectures.

Any thoughts on how to fix this ?

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: trace: trace_kprobe.c always shows interrupts off

[Steven Rostedt]

On Tue, 17 Nov 2015 16:35:35 +0000 (UTC)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:

> Hi,
> 
> I notice that trace_kprobe.c does local_save_flags() within
> __kprobe_trace_func(), which is called (at least on x86) with
> interrupts always disabled. This is then used as interrupt on/off
> state within the recorded event, which is misleading.

Kinda. The kprobe itself has interrupts disabled, so it's only a white
lie.

> 
> I also don't understand why arch/x86/kernel/kprobes/ftrace.c
> pre handler disables interrupts, considering the following
> comment above arch/x86/kernel/kprobes/core.c: kprobe_int3_handler()
> 
>  * Interrupts are disabled on entry as trap3 is an interrupt gate and they
>  * remain disabled throughout this function.

I think you answered your own question. The key piece you may be
missing is that kprobe_ftrace_handler() is not called from a trap, but
from a function traced callback, which does not disable interrupts.

> 
> A struct pt_regs is received by this function, but I don't see
> any way to get the state of irq enable/disable from struct pt_regs
> across architectures.
> 
> Any thoughts on how to fix this ?

Create a cross arch: flags = regs_irq_save(regs) function.

-- Steve

RE: trace: trace_kprobe.c always shows interrupts off

[平松雅巳 / HIRAMATU，MASAMI]

Hi Mathieu, Steven,

From: Steven Rostedt [mailto:rostedt@goodmis.org]
>
>On Tue, 17 Nov 2015 16:35:35 +0000 (UTC)
>Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
>
>> Hi,
>>
>> I notice that trace_kprobe.c does local_save_flags() within
>> __kprobe_trace_func(), which is called (at least on x86) with
>> interrupts always disabled. This is then used as interrupt on/off
>> state within the recorded event, which is misleading.
>
>Kinda. The kprobe itself has interrupts disabled, so it's only a white
>lie.
>
>>
>> I also don't understand why arch/x86/kernel/kprobes/ftrace.c
>> pre handler disables interrupts, considering the following
>> comment above arch/x86/kernel/kprobes/core.c: kprobe_int3_handler()
>>
>>  * Interrupts are disabled on entry as trap3 is an interrupt gate and they
>>  * remain disabled throughout this function.
>
>I think you answered your own question. The key piece you may be
>missing is that kprobe_ftrace_handler() is not called from a trap, but
>from a function traced callback, which does not disable interrupts.

right, since the user can not forcibly change any kprobes to
jump, those handlers should be ran under the same environment.

>> A struct pt_regs is received by this function, but I don't see
>> any way to get the state of irq enable/disable from struct pt_regs
>> across architectures.
>>
>> Any thoughts on how to fix this ?
>
>Create a cross arch: flags = regs_irq_save(regs) function.
>

Good idea! Anyway, we can start on x86, in other arch we can
prepare a dummy function to return current interrupt state(as
we are doing now).

Thank you,

----
Masami Hiramatsu 

Re: trace: trace_kprobe.c always shows interrupts off

[Mathieu Desnoyers]

----- On Nov 18, 2015, at 1:51 AM, masami hiramatsu pt masami.hiramatsu.pt@hitachi.com wrote:

> Hi Mathieu, Steven,
> 
> From: Steven Rostedt [mailto:rostedt@goodmis.org]
>>
>>On Tue, 17 Nov 2015 16:35:35 +0000 (UTC)
>>Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
>>
>>> Hi,
>>>
>>> I notice that trace_kprobe.c does local_save_flags() within
>>> __kprobe_trace_func(), which is called (at least on x86) with
>>> interrupts always disabled. This is then used as interrupt on/off
>>> state within the recorded event, which is misleading.
>>
>>Kinda. The kprobe itself has interrupts disabled, so it's only a white
>>lie.
>>
>>>
>>> I also don't understand why arch/x86/kernel/kprobes/ftrace.c
>>> pre handler disables interrupts, considering the following
>>> comment above arch/x86/kernel/kprobes/core.c: kprobe_int3_handler()
>>>
>>>  * Interrupts are disabled on entry as trap3 is an interrupt gate and they
>>>  * remain disabled throughout this function.
>>
>>I think you answered your own question. The key piece you may be
>>missing is that kprobe_ftrace_handler() is not called from a trap, but
>>from a function traced callback, which does not disable interrupts.
> 
> right, since the user can not forcibly change any kprobes to
> jump, those handlers should be ran under the same environment.
> 
>>> A struct pt_regs is received by this function, but I don't see
>>> any way to get the state of irq enable/disable from struct pt_regs
>>> across architectures.
>>>
>>> Any thoughts on how to fix this ?
>>
>>Create a cross arch: flags = regs_irq_save(regs) function.
>>
> 
> Good idea! Anyway, we can start on x86, in other arch we can
> prepare a dummy function to return current interrupt state(as
> we are doing now).

Derived from Steven's idea, I added a lttng_regs_irqs_disabled(struct pt_regs *regs)
to lttng-modules. The generic version returns "-1", which means that the architecture
don't support it yet. It returns 1 or 0 when implemented.

I prefer this approach compared to fall-back to irqs_disabled(), because
users then know that they can trust the information when it is
available.

Returning an unknown state with a regs_irq_save(regs) did not
seem obvious, because there is no guarantee that a value like
"-1" does not mean something specific on some architectures.
One possibility would be to make it return success/failure,
and populate the flags in an output parameter, but that seems
rather cumbersome.

When regs_irq_save() becomes widely available across architectures
in newer kernels, I'll use it of course.

Thanks,

Mathieu


> 
> Thank you,
> 
> ----
> Masami Hiramatsu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com