Re: Re: Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
Hi Ingo, (2013/12/13 14:34), Masami Hiramatsu wrote: > And also, even if we can detect the recursion, we can't stop the > kernel, we need to skip the probe. This means that we need to > recover to the main execution path by doing single step. As you > may know, since the single stepping involves the debug exception, > we have to avoid proving on that path too. Or we'll have an > infinite recursion again. I don't see why this is needed: if a "probing is disabled" recursion flag is set the moment the first probe fires, and if it's only cleared once all processing is finished, then any intermediate probes should simply return early from int3 and not fire. >>> >>> No, because the int3 already changes the original instruction. >>> This means that you cannot skip singlestep(or emulate) the >>> instruction which is copied to execution buffer (ainsn->insn), >>> even if you have such the flag. >>> So, kprobe requires the annotations on the singlestep path. >> >> I don't understand this reasoning. >> >> Lets assume we allow a probe to be inserted in the single-step path. >> Such a probe will be an INT3 instruction and if it hits we get a >> recursive INT3 invocation. In that case the INT3 handler should simply >> restore the original instruction and _leave it so_. There's no >> single-stepping needed - the probe is confused and must be discarded. > > But how can we restore the protected kernel text? > If we use text_poke, we also need to prohibit probing on the text_poke > and functions called in the text_poke too. That just shifts the annotated > area to the text_poke. :( OK, I've checked current text_poke() and thought how we can do that. The current text_poke() uses special fixmap to make alias pages for avoiding kernel-text readonly protection. For protecting the fixmap pages, we are currently using text_mutex and this is why we can't use it in exception path. There are other minor issues, but it seems to be fixed easily. :) Thus, for recovering original instruction in the int3 handler, I'd like to propose adding another text_poke like function, which requires another fixmap page and protects it by using raw_spinlock (to avoid tracing), and just support one-byte poke (this means it never across the page boundary). Perhaps, it can be implemented inside kprobes, because it is not useful for other subsystems. Thank you! -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Re: Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
Hi Ingo, (2013/12/13 14:34), Masami Hiramatsu wrote: And also, even if we can detect the recursion, we can't stop the kernel, we need to skip the probe. This means that we need to recover to the main execution path by doing single step. As you may know, since the single stepping involves the debug exception, we have to avoid proving on that path too. Or we'll have an infinite recursion again. I don't see why this is needed: if a probing is disabled recursion flag is set the moment the first probe fires, and if it's only cleared once all processing is finished, then any intermediate probes should simply return early from int3 and not fire. No, because the int3 already changes the original instruction. This means that you cannot skip singlestep(or emulate) the instruction which is copied to execution buffer (ainsn-insn), even if you have such the flag. So, kprobe requires the annotations on the singlestep path. I don't understand this reasoning. Lets assume we allow a probe to be inserted in the single-step path. Such a probe will be an INT3 instruction and if it hits we get a recursive INT3 invocation. In that case the INT3 handler should simply restore the original instruction and _leave it so_. There's no single-stepping needed - the probe is confused and must be discarded. But how can we restore the protected kernel text? If we use text_poke, we also need to prohibit probing on the text_poke and functions called in the text_poke too. That just shifts the annotated area to the text_poke. :( OK, I've checked current text_poke() and thought how we can do that. The current text_poke() uses special fixmap to make alias pages for avoiding kernel-text readonly protection. For protecting the fixmap pages, we are currently using text_mutex and this is why we can't use it in exception path. There are other minor issues, but it seems to be fixed easily. :) Thus, for recovering original instruction in the int3 handler, I'd like to propose adding another text_poke like function, which requires another fixmap page and protects it by using raw_spinlock (to avoid tracing), and just support one-byte poke (this means it never across the page boundary). Perhaps, it can be implemented inside kprobes, because it is not useful for other subsystems. Thank you! -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Re: Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
(2013/12/13 14:34), Masami Hiramatsu wrote: >> Lets assume we allow a probe to be inserted in the single-step path. >> Such a probe will be an INT3 instruction and if it hits we get a >> recursive INT3 invocation. In that case the INT3 handler should simply >> restore the original instruction and _leave it so_. There's no >> single-stepping needed - the probe is confused and must be discarded. > > But how can we restore the protected kernel text? > If we use text_poke, we also need to prohibit probing on the text_poke > and functions called in the text_poke too. That just shifts the annotated > area to the text_poke. :( BTW, currently we mark the text_poke as nokprobe_symbol, but it should be removed. We don't call it from kprobes int3/debug handlers. The patches which removes __kprobes in this series are only for kprobe related files (arch/x86/kernel/kprobes/* or kernel/kprobes.c.) I think we should do it for other parts. Is it better to do that on this series? Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
(2013/12/12 23:03), Ingo Molnar wrote:
>
> * Masami Hiramatsu wrote:
>
>> (2013/12/11 22:34), Ingo Molnar wrote:
>>>
>>> * Masami Hiramatsu wrote:
>>>
> So why are annotations needed at all? What can happen if an
> annotation is missing and a piece of code is probed which is also
> used by the kprobes code internally - do we crash, lock up,
> misbehave or handle it safely?
The kprobe has recursion detector, [...]
>>>
>>> It's the 'current_kprobe' percpu variable, checked via
>>> kprobe_running(), right?
>>
>> Right. :)
>
> So that recursion detection runs a bit late:
>
> /*
> * Interrupts are disabled on entry as trap3 is an interrupt gate and they
> * remain disabled throughout this function.
> */
> static int __kprobes kprobe_handler(struct pt_regs *regs)
> {
> kprobe_opcode_t *addr;
> struct kprobe *p;
> struct kprobe_ctlblk *kcb;
>
> addr = (kprobe_opcode_t *)(regs->ip - sizeof(kprobe_opcode_t));
> /*
> * We don't want to be preempted for the entire
> * duration of kprobe processing. We conditionally
> * re-enable preemption at the end of this function,
> * and also in reenter_kprobe() and setup_singlestep().
> */
> preempt_disable();
>
> kcb = get_kprobe_ctlblk();
> p = get_kprobe(addr);
>
> if (p) {
> if (kprobe_running()) {
>
> this flag should be checked first - the kprobe handler should already
> run in non-preemptible context if it comes from an exception.
No. Even if we check it first, this flag is *only* for recursive
kprobes inside the kprobe handlers, not for the crash case.
If we'd like to check the crash case, we need to do that in
the earliest stage on the int3 interrupt, like in entry_XX.S.
And we need to allow some degree of recursion, since there
are several different int3 users (ftrace, jump label, kgdb),
at least jprobe uses int3 for returning from its handler
(see jprobe_return - kprobe provides break_handler to handle
int3 inside kprobe for this purpose.) Those are orthogonal
features, thus it will not cause infinite recursion.
> For that reason I don't understand the whole
> preempt_disable()/enable() dance - it looks entirely superfluous to
> me. The comment above the preempt_disable() looks mostly bogus.
Actually, this can be removed, because not only what you pointed,
but also the local interrupt is disabled while the int3 is
processing. This means that we don't need to disable preemption.
> ( The flow of logic in the function is rather confusing as well -
> lots of places return from the middle of the function - instead they
> should have the usual 'goto out' kind of code pattern. )
>
[...] but it is detected in the kprobe exception(int3) handler,
this means that if we put a probe before detecting the recursion,
we'll do an infinite recursion.
>>>
>>> So only the (presumably rather narrow) code path leading to the
>>> recursion detection code has to be annotated, correct?
>>
>> Yes, correct.
>
> So, another thing I find confusing is the whole kprobes notifier
> block. Why doesn't it call back specific kprobes handlers, directly
> from do_int3() and do_debug()? That's much more readable and it also
> allows the kprobes code to go earlier in the handler, running its
> recursion code earlier!
Yes, I fixed it on this series! :)
I don't know what had discussed about using notify_die to handle
int3/debug. I guess it looks more generic way at that time.
> Another question I have here is: how does the kprobes code protect
> against interrupts arriving in before the recursion check and running
> a probe recursively?
That does just one recursion, it's no problem. The problem happens
if it causes infinite recursion.
And also, even if we can detect the recursion, we can't stop the
kernel, we need to skip the probe. This means that we need to
recover to the main execution path by doing single step. As you
may know, since the single stepping involves the debug exception,
we have to avoid proving on that path too. Or we'll have an
infinite recursion again.
>>>
>>> I don't see why this is needed: if a "probing is disabled"
>>> recursion flag is set the moment the first probe fires, and if
>>> it's only cleared once all processing is finished, then any
>>> intermediate probes should simply return early from int3 and not
>>> fire.
>>
>> No, because the int3 already changes the original instruction.
>> This means that you cannot skip singlestep(or emulate) the
>> instruction which is copied to execution buffer (ainsn->insn),
>> even if you have such the flag.
>> So, kprobe requires the annotations on the singlestep path.
>
> I don't understand this reasoning.
>
> Lets assume we allow a probe to be inserted in the single-step path.
> Such a probe will be an INT3 instruction and if it hits we get a
> recursive INT3 invocation. In that
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
On 12/12/2013 06:03 AM, Ingo Molnar wrote: >> No, because the int3 already changes the original instruction. >> This means that you cannot skip singlestep(or emulate) the >> instruction which is copied to execution buffer (ainsn->insn), >> even if you have such the flag. >> So, kprobe requires the annotations on the singlestep path. > I don't understand this reasoning. > > Lets assume we allow a probe to be inserted in the single-step path. > Such a probe will be an INT3 instruction and if it hits we get a > recursive INT3 invocation. In that case the INT3 handler should simply > restore the original instruction and _leave it so_. There's no > single-stepping needed - the probe is confused and must be discarded. So if you restore the original instruction, then you're essentially creating a dynamic blacklist for the singlestep path, right? I think that's fine, as long as you still allow recursive probes elsewhere to just singlestep and skip that occurrence. It also helps with the inlining issues, since an inlined function instance in the singlestep path can get dynamically blocked, while still allowing inline instances elsewhere to be probed normally. Then you don't have to force always/never inline decisions - whatever gcc decides to do with inlines and static functions can be dealt with. Josh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
* Masami Hiramatsu wrote:
> (2013/12/11 22:34), Ingo Molnar wrote:
> >
> > * Masami Hiramatsu wrote:
> >
> >>> So why are annotations needed at all? What can happen if an
> >>> annotation is missing and a piece of code is probed which is also
> >>> used by the kprobes code internally - do we crash, lock up,
> >>> misbehave or handle it safely?
> >>
> >> The kprobe has recursion detector, [...]
> >
> > It's the 'current_kprobe' percpu variable, checked via
> > kprobe_running(), right?
>
> Right. :)
So that recursion detection runs a bit late:
/*
* Interrupts are disabled on entry as trap3 is an interrupt gate and they
* remain disabled throughout this function.
*/
static int __kprobes kprobe_handler(struct pt_regs *regs)
{
kprobe_opcode_t *addr;
struct kprobe *p;
struct kprobe_ctlblk *kcb;
addr = (kprobe_opcode_t *)(regs->ip - sizeof(kprobe_opcode_t));
/*
* We don't want to be preempted for the entire
* duration of kprobe processing. We conditionally
* re-enable preemption at the end of this function,
* and also in reenter_kprobe() and setup_singlestep().
*/
preempt_disable();
kcb = get_kprobe_ctlblk();
p = get_kprobe(addr);
if (p) {
if (kprobe_running()) {
this flag should be checked first - the kprobe handler should already
run in non-preemptible context if it comes from an exception.
For that reason I don't understand the whole
preempt_disable()/enable() dance - it looks entirely superfluous to
me. The comment above the preempt_disable() looks mostly bogus.
( The flow of logic in the function is rather confusing as well -
lots of places return from the middle of the function - instead they
should have the usual 'goto out' kind of code pattern. )
> >> [...] but it is detected in the kprobe exception(int3) handler,
> >> this means that if we put a probe before detecting the recursion,
> >> we'll do an infinite recursion.
> >
> > So only the (presumably rather narrow) code path leading to the
> > recursion detection code has to be annotated, correct?
>
> Yes, correct.
So, another thing I find confusing is the whole kprobes notifier
block. Why doesn't it call back specific kprobes handlers, directly
from do_int3() and do_debug()? That's much more readable and it also
allows the kprobes code to go earlier in the handler, running its
recursion code earlier!
Another question I have here is: how does the kprobes code protect
against interrupts arriving in before the recursion check and running
a probe recursively?
> >> And also, even if we can detect the recursion, we can't stop the
> >> kernel, we need to skip the probe. This means that we need to
> >> recover to the main execution path by doing single step. As you
> >> may know, since the single stepping involves the debug exception,
> >> we have to avoid proving on that path too. Or we'll have an
> >> infinite recursion again.
> >
> > I don't see why this is needed: if a "probing is disabled"
> > recursion flag is set the moment the first probe fires, and if
> > it's only cleared once all processing is finished, then any
> > intermediate probes should simply return early from int3 and not
> > fire.
>
> No, because the int3 already changes the original instruction.
> This means that you cannot skip singlestep(or emulate) the
> instruction which is copied to execution buffer (ainsn->insn),
> even if you have such the flag.
> So, kprobe requires the annotations on the singlestep path.
I don't understand this reasoning.
Lets assume we allow a probe to be inserted in the single-step path.
Such a probe will be an INT3 instruction and if it hits we get a
recursive INT3 invocation. In that case the INT3 handler should simply
restore the original instruction and _leave it so_. There's no
single-stepping needed - the probe is confused and must be discarded.
Once the original instruction is restored we simply return from the
int3 exception and the single-step handling execution can continue.
This would be _way_ more robust as we wouldn't have to precisely
annotate anything but the very narrow int3 exception code path and the
'restore original instruction in case of recursion' code path.
Thanks,
Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
* Masami Hiramatsu masami.hiramatsu...@hitachi.com wrote:
(2013/12/11 22:34), Ingo Molnar wrote:
* Masami Hiramatsu masami.hiramatsu...@hitachi.com wrote:
So why are annotations needed at all? What can happen if an
annotation is missing and a piece of code is probed which is also
used by the kprobes code internally - do we crash, lock up,
misbehave or handle it safely?
The kprobe has recursion detector, [...]
It's the 'current_kprobe' percpu variable, checked via
kprobe_running(), right?
Right. :)
So that recursion detection runs a bit late:
/*
* Interrupts are disabled on entry as trap3 is an interrupt gate and they
* remain disabled throughout this function.
*/
static int __kprobes kprobe_handler(struct pt_regs *regs)
{
kprobe_opcode_t *addr;
struct kprobe *p;
struct kprobe_ctlblk *kcb;
addr = (kprobe_opcode_t *)(regs-ip - sizeof(kprobe_opcode_t));
/*
* We don't want to be preempted for the entire
* duration of kprobe processing. We conditionally
* re-enable preemption at the end of this function,
* and also in reenter_kprobe() and setup_singlestep().
*/
preempt_disable();
kcb = get_kprobe_ctlblk();
p = get_kprobe(addr);
if (p) {
if (kprobe_running()) {
this flag should be checked first - the kprobe handler should already
run in non-preemptible context if it comes from an exception.
For that reason I don't understand the whole
preempt_disable()/enable() dance - it looks entirely superfluous to
me. The comment above the preempt_disable() looks mostly bogus.
( The flow of logic in the function is rather confusing as well -
lots of places return from the middle of the function - instead they
should have the usual 'goto out' kind of code pattern. )
[...] but it is detected in the kprobe exception(int3) handler,
this means that if we put a probe before detecting the recursion,
we'll do an infinite recursion.
So only the (presumably rather narrow) code path leading to the
recursion detection code has to be annotated, correct?
Yes, correct.
So, another thing I find confusing is the whole kprobes notifier
block. Why doesn't it call back specific kprobes handlers, directly
from do_int3() and do_debug()? That's much more readable and it also
allows the kprobes code to go earlier in the handler, running its
recursion code earlier!
Another question I have here is: how does the kprobes code protect
against interrupts arriving in before the recursion check and running
a probe recursively?
And also, even if we can detect the recursion, we can't stop the
kernel, we need to skip the probe. This means that we need to
recover to the main execution path by doing single step. As you
may know, since the single stepping involves the debug exception,
we have to avoid proving on that path too. Or we'll have an
infinite recursion again.
I don't see why this is needed: if a probing is disabled
recursion flag is set the moment the first probe fires, and if
it's only cleared once all processing is finished, then any
intermediate probes should simply return early from int3 and not
fire.
No, because the int3 already changes the original instruction.
This means that you cannot skip singlestep(or emulate) the
instruction which is copied to execution buffer (ainsn-insn),
even if you have such the flag.
So, kprobe requires the annotations on the singlestep path.
I don't understand this reasoning.
Lets assume we allow a probe to be inserted in the single-step path.
Such a probe will be an INT3 instruction and if it hits we get a
recursive INT3 invocation. In that case the INT3 handler should simply
restore the original instruction and _leave it so_. There's no
single-stepping needed - the probe is confused and must be discarded.
Once the original instruction is restored we simply return from the
int3 exception and the single-step handling execution can continue.
This would be _way_ more robust as we wouldn't have to precisely
annotate anything but the very narrow int3 exception code path and the
'restore original instruction in case of recursion' code path.
Thanks,
Ingo
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
On 12/12/2013 06:03 AM, Ingo Molnar wrote: No, because the int3 already changes the original instruction. This means that you cannot skip singlestep(or emulate) the instruction which is copied to execution buffer (ainsn-insn), even if you have such the flag. So, kprobe requires the annotations on the singlestep path. I don't understand this reasoning. Lets assume we allow a probe to be inserted in the single-step path. Such a probe will be an INT3 instruction and if it hits we get a recursive INT3 invocation. In that case the INT3 handler should simply restore the original instruction and _leave it so_. There's no single-stepping needed - the probe is confused and must be discarded. So if you restore the original instruction, then you're essentially creating a dynamic blacklist for the singlestep path, right? I think that's fine, as long as you still allow recursive probes elsewhere to just singlestep and skip that occurrence. It also helps with the inlining issues, since an inlined function instance in the singlestep path can get dynamically blocked, while still allowing inline instances elsewhere to be probed normally. Then you don't have to force always/never inline decisions - whatever gcc decides to do with inlines and static functions can be dealt with. Josh -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
(2013/12/12 23:03), Ingo Molnar wrote:
* Masami Hiramatsu masami.hiramatsu...@hitachi.com wrote:
(2013/12/11 22:34), Ingo Molnar wrote:
* Masami Hiramatsu masami.hiramatsu...@hitachi.com wrote:
So why are annotations needed at all? What can happen if an
annotation is missing and a piece of code is probed which is also
used by the kprobes code internally - do we crash, lock up,
misbehave or handle it safely?
The kprobe has recursion detector, [...]
It's the 'current_kprobe' percpu variable, checked via
kprobe_running(), right?
Right. :)
So that recursion detection runs a bit late:
/*
* Interrupts are disabled on entry as trap3 is an interrupt gate and they
* remain disabled throughout this function.
*/
static int __kprobes kprobe_handler(struct pt_regs *regs)
{
kprobe_opcode_t *addr;
struct kprobe *p;
struct kprobe_ctlblk *kcb;
addr = (kprobe_opcode_t *)(regs-ip - sizeof(kprobe_opcode_t));
/*
* We don't want to be preempted for the entire
* duration of kprobe processing. We conditionally
* re-enable preemption at the end of this function,
* and also in reenter_kprobe() and setup_singlestep().
*/
preempt_disable();
kcb = get_kprobe_ctlblk();
p = get_kprobe(addr);
if (p) {
if (kprobe_running()) {
this flag should be checked first - the kprobe handler should already
run in non-preemptible context if it comes from an exception.
No. Even if we check it first, this flag is *only* for recursive
kprobes inside the kprobe handlers, not for the crash case.
If we'd like to check the crash case, we need to do that in
the earliest stage on the int3 interrupt, like in entry_XX.S.
And we need to allow some degree of recursion, since there
are several different int3 users (ftrace, jump label, kgdb),
at least jprobe uses int3 for returning from its handler
(see jprobe_return - kprobe provides break_handler to handle
int3 inside kprobe for this purpose.) Those are orthogonal
features, thus it will not cause infinite recursion.
For that reason I don't understand the whole
preempt_disable()/enable() dance - it looks entirely superfluous to
me. The comment above the preempt_disable() looks mostly bogus.
Actually, this can be removed, because not only what you pointed,
but also the local interrupt is disabled while the int3 is
processing. This means that we don't need to disable preemption.
( The flow of logic in the function is rather confusing as well -
lots of places return from the middle of the function - instead they
should have the usual 'goto out' kind of code pattern. )
[...] but it is detected in the kprobe exception(int3) handler,
this means that if we put a probe before detecting the recursion,
we'll do an infinite recursion.
So only the (presumably rather narrow) code path leading to the
recursion detection code has to be annotated, correct?
Yes, correct.
So, another thing I find confusing is the whole kprobes notifier
block. Why doesn't it call back specific kprobes handlers, directly
from do_int3() and do_debug()? That's much more readable and it also
allows the kprobes code to go earlier in the handler, running its
recursion code earlier!
Yes, I fixed it on this series! :)
I don't know what had discussed about using notify_die to handle
int3/debug. I guess it looks more generic way at that time.
Another question I have here is: how does the kprobes code protect
against interrupts arriving in before the recursion check and running
a probe recursively?
That does just one recursion, it's no problem. The problem happens
if it causes infinite recursion.
And also, even if we can detect the recursion, we can't stop the
kernel, we need to skip the probe. This means that we need to
recover to the main execution path by doing single step. As you
may know, since the single stepping involves the debug exception,
we have to avoid proving on that path too. Or we'll have an
infinite recursion again.
I don't see why this is needed: if a probing is disabled
recursion flag is set the moment the first probe fires, and if
it's only cleared once all processing is finished, then any
intermediate probes should simply return early from int3 and not
fire.
No, because the int3 already changes the original instruction.
This means that you cannot skip singlestep(or emulate) the
instruction which is copied to execution buffer (ainsn-insn),
even if you have such the flag.
So, kprobe requires the annotations on the singlestep path.
I don't understand this reasoning.
Lets assume we allow a probe to be inserted in the single-step path.
Such a probe will be an INT3 instruction and if it hits we get a
recursive INT3 invocation. In that case the INT3 handler should simply
restore the original instruction and _leave it so_. There's no
single-stepping needed - the
Re: Re: Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
(2013/12/13 14:34), Masami Hiramatsu wrote: Lets assume we allow a probe to be inserted in the single-step path. Such a probe will be an INT3 instruction and if it hits we get a recursive INT3 invocation. In that case the INT3 handler should simply restore the original instruction and _leave it so_. There's no single-stepping needed - the probe is confused and must be discarded. But how can we restore the protected kernel text? If we use text_poke, we also need to prohibit probing on the text_poke and functions called in the text_poke too. That just shifts the annotated area to the text_poke. :( BTW, currently we mark the text_poke as nokprobe_symbol, but it should be removed. We don't call it from kprobes int3/debug handlers. The patches which removes __kprobes in this series are only for kprobe related files (arch/x86/kernel/kprobes/* or kernel/kprobes.c.) I think we should do it for other parts. Is it better to do that on this series? Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
(2013/12/11 22:34), Ingo Molnar wrote: > > * Masami Hiramatsu wrote: > >>> So why are annotations needed at all? What can happen if an >>> annotation is missing and a piece of code is probed which is also >>> used by the kprobes code internally - do we crash, lock up, >>> misbehave or handle it safely? >> >> The kprobe has recursion detector, [...] > > It's the 'current_kprobe' percpu variable, checked via > kprobe_running(), right? Right. :) >> [...] but it is detected in the kprobe exception(int3) handler, this >> means that if we put a probe before detecting the recursion, we'll >> do an infinite recursion. > > So only the (presumably rather narrow) code path leading to the > recursion detection code has to be annotated, correct? Yes, correct. >> And also, even if we can detect the recursion, we can't stop the >> kernel, we need to skip the probe. This means that we need to >> recover to the main execution path by doing single step. As you may >> know, since the single stepping involves the debug exception, we >> have to avoid proving on that path too. Or we'll have an infinite >> recursion again. > > I don't see why this is needed: if a "probing is disabled" recursion > flag is set the moment the first probe fires, and if it's only cleared > once all processing is finished, then any intermediate probes should > simply return early from int3 and not fire. No, because the int3 already changes the original instruction. This means that you cannot skip singlestep(or emulate) the instruction which is copied to execution buffer (ainsn->insn), even if you have such the flag. So, kprobe requires the annotations on the singlestep path. Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
* Masami Hiramatsu wrote: > > So why are annotations needed at all? What can happen if an > > annotation is missing and a piece of code is probed which is also > > used by the kprobes code internally - do we crash, lock up, > > misbehave or handle it safely? > > The kprobe has recursion detector, [...] It's the 'current_kprobe' percpu variable, checked via kprobe_running(), right? > [...] but it is detected in the kprobe exception(int3) handler, this > means that if we put a probe before detecting the recursion, we'll > do an infinite recursion. So only the (presumably rather narrow) code path leading to the recursion detection code has to be annotated, correct? > And also, even if we can detect the recursion, we can't stop the > kernel, we need to skip the probe. This means that we need to > recover to the main execution path by doing single step. As you may > know, since the single stepping involves the debug exception, we > have to avoid proving on that path too. Or we'll have an infinite > recursion again. I don't see why this is needed: if a "probing is disabled" recursion flag is set the moment the first probe fires, and if it's only cleared once all processing is finished, then any intermediate probes should simply return early from int3 and not fire. What am I missing? Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
* Masami Hiramatsu masami.hiramatsu...@hitachi.com wrote: So why are annotations needed at all? What can happen if an annotation is missing and a piece of code is probed which is also used by the kprobes code internally - do we crash, lock up, misbehave or handle it safely? The kprobe has recursion detector, [...] It's the 'current_kprobe' percpu variable, checked via kprobe_running(), right? [...] but it is detected in the kprobe exception(int3) handler, this means that if we put a probe before detecting the recursion, we'll do an infinite recursion. So only the (presumably rather narrow) code path leading to the recursion detection code has to be annotated, correct? And also, even if we can detect the recursion, we can't stop the kernel, we need to skip the probe. This means that we need to recover to the main execution path by doing single step. As you may know, since the single stepping involves the debug exception, we have to avoid proving on that path too. Or we'll have an infinite recursion again. I don't see why this is needed: if a probing is disabled recursion flag is set the moment the first probe fires, and if it's only cleared once all processing is finished, then any intermediate probes should simply return early from int3 and not fire. What am I missing? Thanks, Ingo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
(2013/12/11 22:34), Ingo Molnar wrote: * Masami Hiramatsu masami.hiramatsu...@hitachi.com wrote: So why are annotations needed at all? What can happen if an annotation is missing and a piece of code is probed which is also used by the kprobes code internally - do we crash, lock up, misbehave or handle it safely? The kprobe has recursion detector, [...] It's the 'current_kprobe' percpu variable, checked via kprobe_running(), right? Right. :) [...] but it is detected in the kprobe exception(int3) handler, this means that if we put a probe before detecting the recursion, we'll do an infinite recursion. So only the (presumably rather narrow) code path leading to the recursion detection code has to be annotated, correct? Yes, correct. And also, even if we can detect the recursion, we can't stop the kernel, we need to skip the probe. This means that we need to recover to the main execution path by doing single step. As you may know, since the single stepping involves the debug exception, we have to avoid proving on that path too. Or we'll have an infinite recursion again. I don't see why this is needed: if a probing is disabled recursion flag is set the moment the first probe fires, and if it's only cleared once all processing is finished, then any intermediate probes should simply return early from int3 and not fire. No, because the int3 already changes the original instruction. This means that you cannot skip singlestep(or emulate) the instruction which is copied to execution buffer (ainsn-insn), even if you have such the flag. So, kprobe requires the annotations on the singlestep path. Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
(2013/12/11 0:28), Ingo Molnar wrote: > > * Masami Hiramatsu wrote: > >> (2013/12/05 19:21), Ingo Molnar wrote: >>> >>> * Masami Hiramatsu wrote: >>> > So we need both a maintainable and a sane/safe solution, and I'd > like to apply the whole thing at once and be at ease that the > solution is round. We should have done this years ago. For the safeness of kprobes, I have an idea; introduce a whitelist for dynamic events. AFAICS, the biggest unstable issue of kprobes comes from putting *many* probes on the functions called from tracers. >>> >>> If the number of 'noprobe' annotations is expected to explode then >>> maybe another approach should be considered. >> >> No, since this is a "quantitative" issue, the annotation helps us. Sorry for confusion, it should be "the annotation helps us to solves only for qualitative issue". It's my miss... >>> For example in perf we detect recursion. Could kprobes do that and >>> detect hitting a probe while running kprobes code, and ignore it [do >>> an early return]? >> >> Yes, the kprobe itself already has recursion detector and it rejects >> calling handler. > > So why are annotations needed at all? What can happen if an annotation > is missing and a piece of code is probed which is also used by the > kprobes code internally - do we crash, lock up, misbehave or handle it > safely? The kprobe has recursion detector, but it is detected in the kprobe exception(int3) handler, this means that if we put a probe before detecting the recursion, we'll do an infinite recursion. And also, even if we can detect the recursion, we can't stop the kernel, we need to skip the probe. This means that we need to recover to the main execution path by doing single step. As you may know, since the single stepping involves the debug exception, we have to avoid proving on that path too. Or we'll have an infinite recursion again. This is the basic reason why the __kprobes annotation is introduced. Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
* Masami Hiramatsu wrote: > (2013/12/05 19:21), Ingo Molnar wrote: > > > > * Masami Hiramatsu wrote: > > > >>> So we need both a maintainable and a sane/safe solution, and I'd > >>> like to apply the whole thing at once and be at ease that the > >>> solution is round. We should have done this years ago. > >> > >> For the safeness of kprobes, I have an idea; introduce a whitelist > >> for dynamic events. AFAICS, the biggest unstable issue of kprobes > >> comes from putting *many* probes on the functions called from > >> tracers. > > > > If the number of 'noprobe' annotations is expected to explode then > > maybe another approach should be considered. > > No, since this is a "quantitative" issue, the annotation helps us. > > > For example in perf we detect recursion. Could kprobes do that and > > detect hitting a probe while running kprobes code, and ignore it [do > > an early return]? > > Yes, the kprobe itself already has recursion detector and it rejects > calling handler. So why are annotations needed at all? What can happen if an annotation is missing and a piece of code is probed which is also used by the kprobes code internally - do we crash, lock up, misbehave or handle it safely? Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
(2013/12/11 0:28), Ingo Molnar wrote: * Masami Hiramatsu masami.hiramatsu...@hitachi.com wrote: (2013/12/05 19:21), Ingo Molnar wrote: * Masami Hiramatsu masami.hiramatsu...@hitachi.com wrote: So we need both a maintainable and a sane/safe solution, and I'd like to apply the whole thing at once and be at ease that the solution is round. We should have done this years ago. For the safeness of kprobes, I have an idea; introduce a whitelist for dynamic events. AFAICS, the biggest unstable issue of kprobes comes from putting *many* probes on the functions called from tracers. If the number of 'noprobe' annotations is expected to explode then maybe another approach should be considered. No, since this is a quantitative issue, the annotation helps us. Sorry for confusion, it should be the annotation helps us to solves only for qualitative issue. It's my miss... For example in perf we detect recursion. Could kprobes do that and detect hitting a probe while running kprobes code, and ignore it [do an early return]? Yes, the kprobe itself already has recursion detector and it rejects calling handler. So why are annotations needed at all? What can happen if an annotation is missing and a piece of code is probed which is also used by the kprobes code internally - do we crash, lock up, misbehave or handle it safely? The kprobe has recursion detector, but it is detected in the kprobe exception(int3) handler, this means that if we put a probe before detecting the recursion, we'll do an infinite recursion. And also, even if we can detect the recursion, we can't stop the kernel, we need to skip the probe. This means that we need to recover to the main execution path by doing single step. As you may know, since the single stepping involves the debug exception, we have to avoid proving on that path too. Or we'll have an infinite recursion again. This is the basic reason why the __kprobes annotation is introduced. Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
* Masami Hiramatsu masami.hiramatsu...@hitachi.com wrote: (2013/12/05 19:21), Ingo Molnar wrote: * Masami Hiramatsu masami.hiramatsu...@hitachi.com wrote: So we need both a maintainable and a sane/safe solution, and I'd like to apply the whole thing at once and be at ease that the solution is round. We should have done this years ago. For the safeness of kprobes, I have an idea; introduce a whitelist for dynamic events. AFAICS, the biggest unstable issue of kprobes comes from putting *many* probes on the functions called from tracers. If the number of 'noprobe' annotations is expected to explode then maybe another approach should be considered. No, since this is a quantitative issue, the annotation helps us. For example in perf we detect recursion. Could kprobes do that and detect hitting a probe while running kprobes code, and ignore it [do an early return]? Yes, the kprobe itself already has recursion detector and it rejects calling handler. So why are annotations needed at all? What can happen if an annotation is missing and a piece of code is probed which is also used by the kprobes code internally - do we crash, lock up, misbehave or handle it safely? Thanks, Ingo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
(2013/12/07 10:32), Frank Ch. Eigler wrote: > Hi - > > On Sat, Dec 07, 2013 at 08:19:13AM +0900, Masami Hiramatsu wrote: > >> [...] >>> Would you plan to limit kprobes (or just the perf-probe frontend) to >>> only function-entries also? > >> Exactly, yes :). Currently I have a patch for kprobe-tracer >> implementation (not only for perf-probe, but doesn't limit kprobes >> itself). > > Interesting option. It sounds like a restrictive expedient that could > result in kprobes never being made sufficiently robust. the raw-kprobes users like systemtap can also implement its own whitelist. :) ftrace-based whitelist is only useful for ftrace/perf. Anyway, the list is open via debugfs as available_filter_functions. >>> If not, and if intra-function statement-granularity kprobes remain >>> allowed within a function-granularity whitelist, then you might >>> still have those "quantitative" problems. > >> Yes, but as far as I've tested, the performance overhead is not >> high, especially as far as putting kprobes at the entry of those >> functions because of ftrace-based optimization. > > (Would that also make CONFIG_KPROBE_EVENT require KPROBES_ON_FTRACE?) Ah, no but a good point. at least the whitelist requires CONFIG_FUNCTION_TRACER. >>> Even worse, kprobes robustness problems can bite even with a small >>> whitelist, unless you can test the countless subset selections >>> cartesian-product the aggrevating factors (like other tracing >>> facilities being in use at the same time, limited memory, high irq >>> rates, debugging sessions, architectures, whatever). >> >> And also, what script will run on each probe, right? :) > > In the perf-probe world, the closest analogue could be varying the > contextual data that's being extracted (stack traces, parameters, ...). Yes, it should be verified before accessing it (and already done). [...] For the long term solution, I think we can introduce some kind of performance gatekeeper as systemtap does. Counting the miss-hit rate per second and if it go over a threshold, disable next miss-hit (or most miss-hit) probe (as OOM killer does). >>> >>> That would make sense, but again it would not help deal with kprobes >>> robustness (in the kernel-crashing rather than kernel-slowdown sense). >> >> Why would you think so? Is there any hidden path for calling kprobes >> mechanism?? The kernel crash problem just comes from bugs, not the >> quantitative issue. > > I don't think we're disagreeing. A performance-gatekeeper in > perf-probe or nearby would be useful (and manage the kprobe-quantity > problem). It would not be sufficient to prevent the kernel-crashing > bugs. Right. Ah, I just meant that we'd better add those features, not replacing the blacklist. And the blacklist should be maintained anyway. :) Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
Hi - On Sat, Dec 07, 2013 at 08:19:13AM +0900, Masami Hiramatsu wrote: > [...] > > Would you plan to limit kprobes (or just the perf-probe frontend) to > > only function-entries also? > Exactly, yes :). Currently I have a patch for kprobe-tracer > implementation (not only for perf-probe, but doesn't limit kprobes > itself). Interesting option. It sounds like a restrictive expedient that could result in kprobes never being made sufficiently robust. > > If not, and if intra-function statement-granularity kprobes remain > > allowed within a function-granularity whitelist, then you might > > still have those "quantitative" problems. > Yes, but as far as I've tested, the performance overhead is not > high, especially as far as putting kprobes at the entry of those > functions because of ftrace-based optimization. (Would that also make CONFIG_KPROBE_EVENT require KPROBES_ON_FTRACE?) > > Even worse, kprobes robustness problems can bite even with a small > > whitelist, unless you can test the countless subset selections > > cartesian-product the aggrevating factors (like other tracing > > facilities being in use at the same time, limited memory, high irq > > rates, debugging sessions, architectures, whatever). > > And also, what script will run on each probe, right? :) In the perf-probe world, the closest analogue could be varying the contextual data that's being extracted (stack traces, parameters, ...). > >> [...] For the long term solution, I think we can introduce some > >> kind of performance gatekeeper as systemtap does. Counting the > >> miss-hit rate per second and if it go over a threshold, disable next > >> miss-hit (or most miss-hit) probe (as OOM killer does). > > > > That would make sense, but again it would not help deal with kprobes > > robustness (in the kernel-crashing rather than kernel-slowdown sense). > > Why would you think so? Is there any hidden path for calling kprobes > mechanism?? The kernel crash problem just comes from bugs, not the > quantitative issue. I don't think we're disagreeing. A performance-gatekeeper in perf-probe or nearby would be useful (and manage the kprobe-quantity problem). It would not be sufficient to prevent the kernel-crashing bugs. - FChE -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
(2013/12/07 4:07), Frank Ch. Eigler wrote: > Hi, Masami - > > masami.hiramatsu.pt wrote: > >> [...] [...] Then, I'd like to propose this new whitelist feature in kprobe-tracer (not raw kprobe itself). And a sysctl knob for disabling the whitelist. That knob will be /proc/sys/debug/kprobe-event-whitelist and disabling it will mark kernel tainted so that we can check it from bug reports. >>> >>> How would one assemble a reliable whitelist, if we haven't fully >>> characterized the problems that make the blacklist necessary? >> >> As I said, we can use function graph tracer's list as the whitelist, >> since it doesn't include any functions invoked from ftrace's event >> handler. (Note that I don't mention the Systemtap or other user here) >> >> Whitelist is just for keeping the people away from the quantitative >> issue, who just want to trace their subsystems except for ftrace. >> [...] > > Would you plan to limit kprobes (or just the perf-probe frontend) to > only function-entries also? Exactly, yes :). Currently I have a patch for kprobe-tracer implementation (not only for perf-probe, but doesn't limit kprobes itself). > If not, and if intra-function > statement-granularity kprobes remain allowed within a > function-granularity whitelist, then you might still have those > "quantitative" problems. Yes, but as far as I've tested, the performance overhead is not high, especially as far as putting kprobes at the entry of those functions because of ftrace-based optimization. > Even worse, kprobes robustness problems can bite even with a small > whitelist, unless you can test the countless subset selections > cartesian-product the aggrevating factors (like other tracing > facilities being in use at the same time, limited memory, high irq > rates, debugging sessions, architectures, whatever). And also, what script will run on each probe, right? :) >> [...] For the long term solution, I think we can introduce some >> kind of performance gatekeeper as systemtap does. Counting the >> miss-hit rate per second and if it go over a threshold, disable next >> miss-hit (or most miss-hit) probe (as OOM killer does). > > That would make sense, but again it would not help deal with kprobes > robustness (in the kernel-crashing rather than kernel-slowdown sense). Why would you think so? Is there any hidden path for calling kprobes mechanism?? The kernel crash problem just comes from bugs, not the quantitative issue. Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
(2013/12/06 15:54), Sandeepa Prabhu wrote: >>> I am not sure if this question is related, uprobes or ftrace code does >>> not define __kprobes, so is it safe to place kprobe on uprobes or >>> ftrace code? >> >> Yes, it is "safe" in qualitative meaning. But for ftrace code, it could >> give a performance impact by miss-hitting. Since uprobe is independent >> from kprobe, it should work. >> >>> Is it expected from arch code to support such cases? >> >> Yes, the arch dependent implementation is the key. If it shares some >> code which can be called from miss-hit path, it should be blacklisted. > well, isn't the blacklist only for those routines that can not be > handled or may crash kernel, like the code sections called from > exception kprobes exception handlers etc? Yes, that's why the blacklist is needed. > suppose if the probe on routine can miss-hit (probes re-cursing) but > can be handled, it's only a quantitative issue (i.e. performance > impact) so it should be *user's* problem right? I mean, as you said > earlier about having white-list or a performance gatekeeper > (systemtap), one can avoid such cases by white list or removing > miss-hit probes dynamically. But a blacklisting a symbol means > placing a probe on that *can not be handled* and can crash the system, > is it correct? Yes, exactly that is what I meant. :) The blacklist is only for avoiding such fundamental issue, therefore, it strongly depends on the architecture code. Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
Hi, Masami - masami.hiramatsu.pt wrote: > [...] > >> [...] Then, I'd like to propose this new whitelist feature in > >> kprobe-tracer (not raw kprobe itself). And a sysctl knob for > >> disabling the whitelist. That knob will be > >> /proc/sys/debug/kprobe-event-whitelist and disabling it will mark > >> kernel tainted so that we can check it from bug reports. > > > > How would one assemble a reliable whitelist, if we haven't fully > > characterized the problems that make the blacklist necessary? > > As I said, we can use function graph tracer's list as the whitelist, > since it doesn't include any functions invoked from ftrace's event > handler. (Note that I don't mention the Systemtap or other user here) > > Whitelist is just for keeping the people away from the quantitative > issue, who just want to trace their subsystems except for ftrace. > [...] Would you plan to limit kprobes (or just the perf-probe frontend) to only function-entries also? If not, and if intra-function statement-granularity kprobes remain allowed within a function-granularity whitelist, then you might still have those "quantitative" problems. Even worse, kprobes robustness problems can bite even with a small whitelist, unless you can test the countless subset selections cartesian-product the aggrevating factors (like other tracing facilities being in use at the same time, limited memory, high irq rates, debugging sessions, architectures, whatever). > [...] For the long term solution, I think we can introduce some > kind of performance gatekeeper as systemtap does. Counting the > miss-hit rate per second and if it go over a threshold, disable next > miss-hit (or most miss-hit) probe (as OOM killer does). That would make sense, but again it would not help deal with kprobes robustness (in the kernel-crashing rather than kernel-slowdown sense). - FChE -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
Hi, Masami - masami.hiramatsu.pt wrote: [...] [...] Then, I'd like to propose this new whitelist feature in kprobe-tracer (not raw kprobe itself). And a sysctl knob for disabling the whitelist. That knob will be /proc/sys/debug/kprobe-event-whitelist and disabling it will mark kernel tainted so that we can check it from bug reports. How would one assemble a reliable whitelist, if we haven't fully characterized the problems that make the blacklist necessary? As I said, we can use function graph tracer's list as the whitelist, since it doesn't include any functions invoked from ftrace's event handler. (Note that I don't mention the Systemtap or other user here) Whitelist is just for keeping the people away from the quantitative issue, who just want to trace their subsystems except for ftrace. [...] Would you plan to limit kprobes (or just the perf-probe frontend) to only function-entries also? If not, and if intra-function statement-granularity kprobes remain allowed within a function-granularity whitelist, then you might still have those quantitative problems. Even worse, kprobes robustness problems can bite even with a small whitelist, unless you can test the countless subset selections cartesian-product the aggrevating factors (like other tracing facilities being in use at the same time, limited memory, high irq rates, debugging sessions, architectures, whatever). [...] For the long term solution, I think we can introduce some kind of performance gatekeeper as systemtap does. Counting the miss-hit rate per second and if it go over a threshold, disable next miss-hit (or most miss-hit) probe (as OOM killer does). That would make sense, but again it would not help deal with kprobes robustness (in the kernel-crashing rather than kernel-slowdown sense). - FChE -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
(2013/12/06 15:54), Sandeepa Prabhu wrote: I am not sure if this question is related, uprobes or ftrace code does not define __kprobes, so is it safe to place kprobe on uprobes or ftrace code? Yes, it is safe in qualitative meaning. But for ftrace code, it could give a performance impact by miss-hitting. Since uprobe is independent from kprobe, it should work. Is it expected from arch code to support such cases? Yes, the arch dependent implementation is the key. If it shares some code which can be called from miss-hit path, it should be blacklisted. well, isn't the blacklist only for those routines that can not be handled or may crash kernel, like the code sections called from exception kprobes exception handlers etc? Yes, that's why the blacklist is needed. suppose if the probe on routine can miss-hit (probes re-cursing) but can be handled, it's only a quantitative issue (i.e. performance impact) so it should be *user's* problem right? I mean, as you said earlier about having white-list or a performance gatekeeper (systemtap), one can avoid such cases by white list or removing miss-hit probes dynamically. But a blacklisting a symbol means placing a probe on that *can not be handled* and can crash the system, is it correct? Yes, exactly that is what I meant. :) The blacklist is only for avoiding such fundamental issue, therefore, it strongly depends on the architecture code. Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
(2013/12/07 4:07), Frank Ch. Eigler wrote: Hi, Masami - masami.hiramatsu.pt wrote: [...] [...] Then, I'd like to propose this new whitelist feature in kprobe-tracer (not raw kprobe itself). And a sysctl knob for disabling the whitelist. That knob will be /proc/sys/debug/kprobe-event-whitelist and disabling it will mark kernel tainted so that we can check it from bug reports. How would one assemble a reliable whitelist, if we haven't fully characterized the problems that make the blacklist necessary? As I said, we can use function graph tracer's list as the whitelist, since it doesn't include any functions invoked from ftrace's event handler. (Note that I don't mention the Systemtap or other user here) Whitelist is just for keeping the people away from the quantitative issue, who just want to trace their subsystems except for ftrace. [...] Would you plan to limit kprobes (or just the perf-probe frontend) to only function-entries also? Exactly, yes :). Currently I have a patch for kprobe-tracer implementation (not only for perf-probe, but doesn't limit kprobes itself). If not, and if intra-function statement-granularity kprobes remain allowed within a function-granularity whitelist, then you might still have those quantitative problems. Yes, but as far as I've tested, the performance overhead is not high, especially as far as putting kprobes at the entry of those functions because of ftrace-based optimization. Even worse, kprobes robustness problems can bite even with a small whitelist, unless you can test the countless subset selections cartesian-product the aggrevating factors (like other tracing facilities being in use at the same time, limited memory, high irq rates, debugging sessions, architectures, whatever). And also, what script will run on each probe, right? :) [...] For the long term solution, I think we can introduce some kind of performance gatekeeper as systemtap does. Counting the miss-hit rate per second and if it go over a threshold, disable next miss-hit (or most miss-hit) probe (as OOM killer does). That would make sense, but again it would not help deal with kprobes robustness (in the kernel-crashing rather than kernel-slowdown sense). Why would you think so? Is there any hidden path for calling kprobes mechanism?? The kernel crash problem just comes from bugs, not the quantitative issue. Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
Hi - On Sat, Dec 07, 2013 at 08:19:13AM +0900, Masami Hiramatsu wrote: [...] Would you plan to limit kprobes (or just the perf-probe frontend) to only function-entries also? Exactly, yes :). Currently I have a patch for kprobe-tracer implementation (not only for perf-probe, but doesn't limit kprobes itself). Interesting option. It sounds like a restrictive expedient that could result in kprobes never being made sufficiently robust. If not, and if intra-function statement-granularity kprobes remain allowed within a function-granularity whitelist, then you might still have those quantitative problems. Yes, but as far as I've tested, the performance overhead is not high, especially as far as putting kprobes at the entry of those functions because of ftrace-based optimization. (Would that also make CONFIG_KPROBE_EVENT require KPROBES_ON_FTRACE?) Even worse, kprobes robustness problems can bite even with a small whitelist, unless you can test the countless subset selections cartesian-product the aggrevating factors (like other tracing facilities being in use at the same time, limited memory, high irq rates, debugging sessions, architectures, whatever). And also, what script will run on each probe, right? :) In the perf-probe world, the closest analogue could be varying the contextual data that's being extracted (stack traces, parameters, ...). [...] For the long term solution, I think we can introduce some kind of performance gatekeeper as systemtap does. Counting the miss-hit rate per second and if it go over a threshold, disable next miss-hit (or most miss-hit) probe (as OOM killer does). That would make sense, but again it would not help deal with kprobes robustness (in the kernel-crashing rather than kernel-slowdown sense). Why would you think so? Is there any hidden path for calling kprobes mechanism?? The kernel crash problem just comes from bugs, not the quantitative issue. I don't think we're disagreeing. A performance-gatekeeper in perf-probe or nearby would be useful (and manage the kprobe-quantity problem). It would not be sufficient to prevent the kernel-crashing bugs. - FChE -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
(2013/12/07 10:32), Frank Ch. Eigler wrote: Hi - On Sat, Dec 07, 2013 at 08:19:13AM +0900, Masami Hiramatsu wrote: [...] Would you plan to limit kprobes (or just the perf-probe frontend) to only function-entries also? Exactly, yes :). Currently I have a patch for kprobe-tracer implementation (not only for perf-probe, but doesn't limit kprobes itself). Interesting option. It sounds like a restrictive expedient that could result in kprobes never being made sufficiently robust. the raw-kprobes users like systemtap can also implement its own whitelist. :) ftrace-based whitelist is only useful for ftrace/perf. Anyway, the list is open via debugfs as available_filter_functions. If not, and if intra-function statement-granularity kprobes remain allowed within a function-granularity whitelist, then you might still have those quantitative problems. Yes, but as far as I've tested, the performance overhead is not high, especially as far as putting kprobes at the entry of those functions because of ftrace-based optimization. (Would that also make CONFIG_KPROBE_EVENT require KPROBES_ON_FTRACE?) Ah, no but a good point. at least the whitelist requires CONFIG_FUNCTION_TRACER. Even worse, kprobes robustness problems can bite even with a small whitelist, unless you can test the countless subset selections cartesian-product the aggrevating factors (like other tracing facilities being in use at the same time, limited memory, high irq rates, debugging sessions, architectures, whatever). And also, what script will run on each probe, right? :) In the perf-probe world, the closest analogue could be varying the contextual data that's being extracted (stack traces, parameters, ...). Yes, it should be verified before accessing it (and already done). [...] For the long term solution, I think we can introduce some kind of performance gatekeeper as systemtap does. Counting the miss-hit rate per second and if it go over a threshold, disable next miss-hit (or most miss-hit) probe (as OOM killer does). That would make sense, but again it would not help deal with kprobes robustness (in the kernel-crashing rather than kernel-slowdown sense). Why would you think so? Is there any hidden path for calling kprobes mechanism?? The kernel crash problem just comes from bugs, not the quantitative issue. I don't think we're disagreeing. A performance-gatekeeper in perf-probe or nearby would be useful (and manage the kprobe-quantity problem). It would not be sufficient to prevent the kernel-crashing bugs. Right. Ah, I just meant that we'd better add those features, not replacing the blacklist. And the blacklist should be maintained anyway. :) Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
>> I am not sure if this question is related, uprobes or ftrace code does >> not define __kprobes, so is it safe to place kprobe on uprobes or >> ftrace code? > > Yes, it is "safe" in qualitative meaning. But for ftrace code, it could > give a performance impact by miss-hitting. Since uprobe is independent > from kprobe, it should work. > >> Is it expected from arch code to support such cases? > > Yes, the arch dependent implementation is the key. If it shares some > code which can be called from miss-hit path, it should be blacklisted. well, isn't the blacklist only for those routines that can not be handled or may crash kernel, like the code sections called from exception kprobes exception handlers etc? suppose if the probe on routine can miss-hit (probes re-cursing) but can be handled, it's only a quantitative issue (i.e. performance impact) so it should be *user's* problem right? I mean, as you said earlier about having white-list or a performance gatekeeper (systemtap), one can avoid such cases by white list or removing miss-hit probes dynamically. But a blacklisting a symbol means placing a probe on that *can not be handled* and can crash the system, is it correct? Thanks, Sandeepa > > Thank you, > > -- > Masami HIRAMATSU > IT Management Research Dept. Linux Technology Center > Hitachi, Ltd., Yokohama Research Laboratory > E-mail: masami.hiramatsu...@hitachi.com > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
(2013/12/05 22:08), Sandeepa Prabhu wrote: >> OK, I think the kprobe is like a strong medicine, not a toy, >> since it can intercept most of the kernel functions which >> may process a sensitive user private data. Thus even if we >> fix all bugs and make it safe, I don't think we can open >> it for all users (of course, there should be a knob to open >> for any or restricted users.) >> >>> So we need both a maintainable and a sane/safe solution, and I'd like >>> to apply the whole thing at once and be at ease that the solution is >>> round. We should have done this years ago. >> >> For the safeness of kprobes, I have an idea; introduce a whitelist >> for dynamic events. AFAICS, the biggest unstable issue of kprobes >> comes from putting *many* probes on the functions called from tracers. >> >> It doesn't crash the kernel but slows down so much, because every >> probes hit many other nested miss-hit probes. This gives us a big >> performance impact. However, on the other side, this kind of feature >> can be used *for debugging* static trace events by dynamic one if we >> carefully use a small number of probes on such functions. :) >> >> Thus, I think we can restrict users from probing such functions by >> using a whitelist which ftrace does already have; >> available_filter_functions :) > I am not sure if this question is related, uprobes or ftrace code does > not define __kprobes, so is it safe to place kprobe on uprobes or > ftrace code? Yes, it is "safe" in qualitative meaning. But for ftrace code, it could give a performance impact by miss-hitting. Since uprobe is independent from kprobe, it should work. > Is it expected from arch code to support such cases? Yes, the arch dependent implementation is the key. If it shares some code which can be called from miss-hit path, it should be blacklisted. Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
(2013/12/05 23:49), Frank Ch. Eigler wrote: > > Hi, Masami - > > masami.hiramatsu.pt wrote: > >> [...] >> For the safeness of kprobes, I have an idea; introduce a whitelist >> for dynamic events. AFAICS, the biggest unstable issue of kprobes >> comes from putting *many* probes on the functions called from tracers. > > Why do you think so? Oh, because I actually hit this problem when enabling kprobe-events on every *ftrace-related* functions(ring buffer, trace filter etc.) It doesn't crash the kernel but it slows down the machine very much. And finally I have to reboot it forcibly. But when I just enables a few probes on those functions, the system has no problem. In this case, almost probes are miss-hit because of recursion, but anyway each miss-hit involves int3/debug interrupts and it increases the processing time of one event handling by ftrace as below. 1. hit a kprobe outside of ftrace 2. kprobe calls event handler 3. the event handler calls ftrace-related functions to reserve buffer, check filter, commit buffer etc. 3-1. each ftrace/ringbuffer function hits a kprobe 3-2. the kprobe detect recursion and just do single-step and return 4. do single stepping 5. return from kprobe Note that all the problem happens inside the event handler. > We have had problems with single kprobes in the > "wrong" spot. The main reason I showed spraying them widely is to get > wide coverage with minimal information/effort, not to suggest that the > number of concurrent probes per se is a problem. (We have had > systemtap scripts probing some areas of the kernel with thousands of > active kprobes, e.g. for statement-by-statement variable-watching > jobs, and these have worked fine.) Ah, sorry for confusion. Agreed. I just tried to explain that kprobes can cause a performance problem under *very specific* operation. So the whitelist is just for keeping people away from it. >> It doesn't crash the kernel but slows down so much, because every >> probes hit many other nested miss-hit probes. > > (kprobes does have code to detect & handle reentrancy.) Right. :) >> This gives us a big performance impact. [...] > > Sure, but I'd expect to see pure slowdowns show their impact with > time-related problems like watchdogs firing or timeouts. I doubt it can cause, because each probe processing time is still small enough to slip through the watchdog. >> [...] Then, I'd like to propose this new whitelist feature in >> kprobe-tracer (not raw kprobe itself). And a sysctl knob for >> disabling the whitelist. That knob will be >> /proc/sys/debug/kprobe-event-whitelist and disabling it will mark >> kernel tainted so that we can check it from bug reports. > > How would one assemble a reliable whitelist, if we haven't fully > characterized the problems that make the blacklist necessary? As I said, we can use function graph tracer's list as the whitelist, since it doesn't include any functions invoked from ftrace's event handler. (Note that I don't mention the Systemtap or other user here) Whitelist is just for keeping the people away from the quantitative issue, who just want to trace their subsystems except for ftrace. For example, such people may try to probe every functions (e.g. perf probe --add '* $vars' : actually this is why I don't release wildcard support on perf probe yet). Of course I can implement the whitelist feature in perf probe only, that will allow me to support wildcard on perf probe. :) For the long term solution, I think we can introduce some kind of performance gatekeeper as systemtap does. Counting the miss-hit rate per second and if it go over a threshold, disable next miss-hit (or most miss-hit) probe (as OOM killer does). Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
(2013/12/05 19:21), Ingo Molnar wrote: > > * Masami Hiramatsu wrote: > >>> So we need both a maintainable and a sane/safe solution, and I'd >>> like to apply the whole thing at once and be at ease that the >>> solution is round. We should have done this years ago. >> >> For the safeness of kprobes, I have an idea; introduce a whitelist >> for dynamic events. AFAICS, the biggest unstable issue of kprobes >> comes from putting *many* probes on the functions called from >> tracers. > > If the number of 'noprobe' annotations is expected to explode then > maybe another approach should be considered. No, since this is a "quantitative" issue, the annotation helps us. > For example in perf we detect recursion. Could kprobes do that and > detect hitting a probe while running kprobes code, and ignore it [do > an early return]? Yes, the kprobe itself already has recursion detector and it rejects calling handler. > > That way most of the annotations could be removed and kprobes would > become inherently safe. Is there any complication I'm missing? That is actually what I'm doing with cleanup patches. :) Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
Hi, Masami - masami.hiramatsu.pt wrote: > [...] > For the safeness of kprobes, I have an idea; introduce a whitelist > for dynamic events. AFAICS, the biggest unstable issue of kprobes > comes from putting *many* probes on the functions called from tracers. Why do you think so? We have had problems with single kprobes in the "wrong" spot. The main reason I showed spraying them widely is to get wide coverage with minimal information/effort, not to suggest that the number of concurrent probes per se is a problem. (We have had systemtap scripts probing some areas of the kernel with thousands of active kprobes, e.g. for statement-by-statement variable-watching jobs, and these have worked fine.) > It doesn't crash the kernel but slows down so much, because every > probes hit many other nested miss-hit probes. (kprobes does have code to detect & handle reentrancy.) > This gives us a big performance impact. [...] Sure, but I'd expect to see pure slowdowns show their impact with time-related problems like watchdogs firing or timeouts. > [...] Then, I'd like to propose this new whitelist feature in > kprobe-tracer (not raw kprobe itself). And a sysctl knob for > disabling the whitelist. That knob will be > /proc/sys/debug/kprobe-event-whitelist and disabling it will mark > kernel tainted so that we can check it from bug reports. How would one assemble a reliable whitelist, if we haven't fully characterized the problems that make the blacklist necessary? - FChE -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
> OK, I think the kprobe is like a strong medicine, not a toy, > since it can intercept most of the kernel functions which > may process a sensitive user private data. Thus even if we > fix all bugs and make it safe, I don't think we can open > it for all users (of course, there should be a knob to open > for any or restricted users.) > >> So we need both a maintainable and a sane/safe solution, and I'd like >> to apply the whole thing at once and be at ease that the solution is >> round. We should have done this years ago. > > For the safeness of kprobes, I have an idea; introduce a whitelist > for dynamic events. AFAICS, the biggest unstable issue of kprobes > comes from putting *many* probes on the functions called from tracers. > > It doesn't crash the kernel but slows down so much, because every > probes hit many other nested miss-hit probes. This gives us a big > performance impact. However, on the other side, this kind of feature > can be used *for debugging* static trace events by dynamic one if we > carefully use a small number of probes on such functions. :) > > Thus, I think we can restrict users from probing such functions by > using a whitelist which ftrace does already have; > available_filter_functions :) I am not sure if this question is related, uprobes or ftrace code does not define __kprobes, so is it safe to place kprobe on uprobes or ftrace code? Is it expected from arch code to support such cases? Thanks, Sandeepa -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
* Masami Hiramatsu wrote: > > So we need both a maintainable and a sane/safe solution, and I'd > > like to apply the whole thing at once and be at ease that the > > solution is round. We should have done this years ago. > > For the safeness of kprobes, I have an idea; introduce a whitelist > for dynamic events. AFAICS, the biggest unstable issue of kprobes > comes from putting *many* probes on the functions called from > tracers. If the number of 'noprobe' annotations is expected to explode then maybe another approach should be considered. For example in perf we detect recursion. Could kprobes do that and detect hitting a probe while running kprobes code, and ignore it [do an early return]? That way most of the annotations could be removed and kprobes would become inherently safe. Is there any complication I'm missing? Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
* Masami Hiramatsu masami.hiramatsu...@hitachi.com wrote: So we need both a maintainable and a sane/safe solution, and I'd like to apply the whole thing at once and be at ease that the solution is round. We should have done this years ago. For the safeness of kprobes, I have an idea; introduce a whitelist for dynamic events. AFAICS, the biggest unstable issue of kprobes comes from putting *many* probes on the functions called from tracers. If the number of 'noprobe' annotations is expected to explode then maybe another approach should be considered. For example in perf we detect recursion. Could kprobes do that and detect hitting a probe while running kprobes code, and ignore it [do an early return]? That way most of the annotations could be removed and kprobes would become inherently safe. Is there any complication I'm missing? Thanks, Ingo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
OK, I think the kprobe is like a strong medicine, not a toy, since it can intercept most of the kernel functions which may process a sensitive user private data. Thus even if we fix all bugs and make it safe, I don't think we can open it for all users (of course, there should be a knob to open for any or restricted users.) So we need both a maintainable and a sane/safe solution, and I'd like to apply the whole thing at once and be at ease that the solution is round. We should have done this years ago. For the safeness of kprobes, I have an idea; introduce a whitelist for dynamic events. AFAICS, the biggest unstable issue of kprobes comes from putting *many* probes on the functions called from tracers. It doesn't crash the kernel but slows down so much, because every probes hit many other nested miss-hit probes. This gives us a big performance impact. However, on the other side, this kind of feature can be used *for debugging* static trace events by dynamic one if we carefully use a small number of probes on such functions. :) Thus, I think we can restrict users from probing such functions by using a whitelist which ftrace does already have; available_filter_functions :) I am not sure if this question is related, uprobes or ftrace code does not define __kprobes, so is it safe to place kprobe on uprobes or ftrace code? Is it expected from arch code to support such cases? Thanks, Sandeepa -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
Hi, Masami - masami.hiramatsu.pt wrote: [...] For the safeness of kprobes, I have an idea; introduce a whitelist for dynamic events. AFAICS, the biggest unstable issue of kprobes comes from putting *many* probes on the functions called from tracers. Why do you think so? We have had problems with single kprobes in the wrong spot. The main reason I showed spraying them widely is to get wide coverage with minimal information/effort, not to suggest that the number of concurrent probes per se is a problem. (We have had systemtap scripts probing some areas of the kernel with thousands of active kprobes, e.g. for statement-by-statement variable-watching jobs, and these have worked fine.) It doesn't crash the kernel but slows down so much, because every probes hit many other nested miss-hit probes. (kprobes does have code to detect handle reentrancy.) This gives us a big performance impact. [...] Sure, but I'd expect to see pure slowdowns show their impact with time-related problems like watchdogs firing or timeouts. [...] Then, I'd like to propose this new whitelist feature in kprobe-tracer (not raw kprobe itself). And a sysctl knob for disabling the whitelist. That knob will be /proc/sys/debug/kprobe-event-whitelist and disabling it will mark kernel tainted so that we can check it from bug reports. How would one assemble a reliable whitelist, if we haven't fully characterized the problems that make the blacklist necessary? - FChE -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
(2013/12/05 19:21), Ingo Molnar wrote: * Masami Hiramatsu masami.hiramatsu...@hitachi.com wrote: So we need both a maintainable and a sane/safe solution, and I'd like to apply the whole thing at once and be at ease that the solution is round. We should have done this years ago. For the safeness of kprobes, I have an idea; introduce a whitelist for dynamic events. AFAICS, the biggest unstable issue of kprobes comes from putting *many* probes on the functions called from tracers. If the number of 'noprobe' annotations is expected to explode then maybe another approach should be considered. No, since this is a quantitative issue, the annotation helps us. For example in perf we detect recursion. Could kprobes do that and detect hitting a probe while running kprobes code, and ignore it [do an early return]? Yes, the kprobe itself already has recursion detector and it rejects calling handler. That way most of the annotations could be removed and kprobes would become inherently safe. Is there any complication I'm missing? That is actually what I'm doing with cleanup patches. :) Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
(2013/12/05 23:49), Frank Ch. Eigler wrote: Hi, Masami - masami.hiramatsu.pt wrote: [...] For the safeness of kprobes, I have an idea; introduce a whitelist for dynamic events. AFAICS, the biggest unstable issue of kprobes comes from putting *many* probes on the functions called from tracers. Why do you think so? Oh, because I actually hit this problem when enabling kprobe-events on every *ftrace-related* functions(ring buffer, trace filter etc.) It doesn't crash the kernel but it slows down the machine very much. And finally I have to reboot it forcibly. But when I just enables a few probes on those functions, the system has no problem. In this case, almost probes are miss-hit because of recursion, but anyway each miss-hit involves int3/debug interrupts and it increases the processing time of one event handling by ftrace as below. 1. hit a kprobe outside of ftrace 2. kprobe calls event handler 3. the event handler calls ftrace-related functions to reserve buffer, check filter, commit buffer etc. 3-1. each ftrace/ringbuffer function hits a kprobe 3-2. the kprobe detect recursion and just do single-step and return 4. do single stepping 5. return from kprobe Note that all the problem happens inside the event handler. We have had problems with single kprobes in the wrong spot. The main reason I showed spraying them widely is to get wide coverage with minimal information/effort, not to suggest that the number of concurrent probes per se is a problem. (We have had systemtap scripts probing some areas of the kernel with thousands of active kprobes, e.g. for statement-by-statement variable-watching jobs, and these have worked fine.) Ah, sorry for confusion. Agreed. I just tried to explain that kprobes can cause a performance problem under *very specific* operation. So the whitelist is just for keeping people away from it. It doesn't crash the kernel but slows down so much, because every probes hit many other nested miss-hit probes. (kprobes does have code to detect handle reentrancy.) Right. :) This gives us a big performance impact. [...] Sure, but I'd expect to see pure slowdowns show their impact with time-related problems like watchdogs firing or timeouts. I doubt it can cause, because each probe processing time is still small enough to slip through the watchdog. [...] Then, I'd like to propose this new whitelist feature in kprobe-tracer (not raw kprobe itself). And a sysctl knob for disabling the whitelist. That knob will be /proc/sys/debug/kprobe-event-whitelist and disabling it will mark kernel tainted so that we can check it from bug reports. How would one assemble a reliable whitelist, if we haven't fully characterized the problems that make the blacklist necessary? As I said, we can use function graph tracer's list as the whitelist, since it doesn't include any functions invoked from ftrace's event handler. (Note that I don't mention the Systemtap or other user here) Whitelist is just for keeping the people away from the quantitative issue, who just want to trace their subsystems except for ftrace. For example, such people may try to probe every functions (e.g. perf probe --add '* $vars' : actually this is why I don't release wildcard support on perf probe yet). Of course I can implement the whitelist feature in perf probe only, that will allow me to support wildcard on perf probe. :) For the long term solution, I think we can introduce some kind of performance gatekeeper as systemtap does. Counting the miss-hit rate per second and if it go over a threshold, disable next miss-hit (or most miss-hit) probe (as OOM killer does). Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
(2013/12/05 22:08), Sandeepa Prabhu wrote: OK, I think the kprobe is like a strong medicine, not a toy, since it can intercept most of the kernel functions which may process a sensitive user private data. Thus even if we fix all bugs and make it safe, I don't think we can open it for all users (of course, there should be a knob to open for any or restricted users.) So we need both a maintainable and a sane/safe solution, and I'd like to apply the whole thing at once and be at ease that the solution is round. We should have done this years ago. For the safeness of kprobes, I have an idea; introduce a whitelist for dynamic events. AFAICS, the biggest unstable issue of kprobes comes from putting *many* probes on the functions called from tracers. It doesn't crash the kernel but slows down so much, because every probes hit many other nested miss-hit probes. This gives us a big performance impact. However, on the other side, this kind of feature can be used *for debugging* static trace events by dynamic one if we carefully use a small number of probes on such functions. :) Thus, I think we can restrict users from probing such functions by using a whitelist which ftrace does already have; available_filter_functions :) I am not sure if this question is related, uprobes or ftrace code does not define __kprobes, so is it safe to place kprobe on uprobes or ftrace code? Yes, it is safe in qualitative meaning. But for ftrace code, it could give a performance impact by miss-hitting. Since uprobe is independent from kprobe, it should work. Is it expected from arch code to support such cases? Yes, the arch dependent implementation is the key. If it shares some code which can be called from miss-hit path, it should be blacklisted. Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
I am not sure if this question is related, uprobes or ftrace code does not define __kprobes, so is it safe to place kprobe on uprobes or ftrace code? Yes, it is safe in qualitative meaning. But for ftrace code, it could give a performance impact by miss-hitting. Since uprobe is independent from kprobe, it should work. Is it expected from arch code to support such cases? Yes, the arch dependent implementation is the key. If it shares some code which can be called from miss-hit path, it should be blacklisted. well, isn't the blacklist only for those routines that can not be handled or may crash kernel, like the code sections called from exception kprobes exception handlers etc? suppose if the probe on routine can miss-hit (probes re-cursing) but can be handled, it's only a quantitative issue (i.e. performance impact) so it should be *user's* problem right? I mean, as you said earlier about having white-list or a performance gatekeeper (systemtap), one can avoid such cases by white list or removing miss-hit probes dynamically. But a blacklisting a symbol means placing a probe on that *can not be handled* and can crash the system, is it correct? Thanks, Sandeepa Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
(2013/12/04 17:46), Sandeepa Prabhu wrote: > On 4 December 2013 13:09, Masami Hiramatsu > wrote: >> (2013/12/04 11:54), Sandeepa Prabhu wrote: >>> On 4 December 2013 06:58, Masami Hiramatsu >>> wrote: Hi, Here is the version 4 of NOKPORBE_SYMBOL series. In this version, I removed the cleanup patches and add bugfixes I've found, since those bugs will be critical. Rest of the cleanup and visible blacklists will be proposed later in another series. Oh, just one new thing, I added a new RFC patch which removes the dependency of notify_die() from kprobes miss-hit/recovery path. Since the notify_die() involves locking and lockdep code which invokes a lot of heavy printk functions etc. This helped me to minimize the blacklist and provides more stability for kprobes. Actually, most of int3 handlers are already called from do_int3 directly, I think this change is acceptable too. Here is the updates about NOKPROBE_SYMBOL(). - Now _ASM_NOKPROBE() macro is introduced for assembly symbols on x86. - Rename kprobe_blackpoint to kprobe_blacklist_entry and simplify it. Also NOKPROBE_SYMBOL() macro just saves the address of non-probe-able symbols. --- Masami Hiramatsu (6): >>> kprobes: Prohibit probing on .entry.text code kprobes: Introduce NOKPROBE_SYMBOL() macro for blacklist >>> Hi Masami, >>> Is it good idea to split "arch/x86" code from generic kernel changes? >>> Then we just need to take above two patches for verifying it on arm64 >>> or other platforms. >> >> Yeah, it can be. >> However I think you can apply it without any problem on arm64 tree too, >> since it "just adds" an asm macro in arch/x86/include/asm/asm.h. >> It should not have any effect for other arch. Could you try it? :) > Hmm, for the second patch, git am failed with: "error: patch failed: > kernel/sched/core.c:2662", > manually patched to resolve it. aarch64 tree is right now at Linux 3.13-rc2. Ah I see, that must be changed because it is the change related to introducing new blacklist itself. It is not solved by splitting arch/x86 change. Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
(2013/12/04 17:45), Ingo Molnar wrote: > > * Masami Hiramatsu wrote: > >> Hi, >> Here is the version 4 of NOKPORBE_SYMBOL series. >> >> In this version, I removed the cleanup patches and >> add bugfixes I've found, since those bugs will be >> critical. >> >> Rest of the cleanup and visible blacklists will be proposed later in >> another series. > > Ok, let me make it clear: we need _both_ the conceptual cleanups and > the bug fixes. I see. Why I split this out is because it includes an RFC patch, and for easier review. I still have a series of cleanups :) > Right now kprobes are restricted to root, and they are unsafe and > buggy, and rather fundamentally so, because probing cannot be done > safely without potentially crashing the kernel. So there's no > 'regression' to be fixed, it's mostly about pre-existing bugs - so > there's no requirement for them to come before maintainability > cleanups. OK, I think the kprobe is like a strong medicine, not a toy, since it can intercept most of the kernel functions which may process a sensitive user private data. Thus even if we fix all bugs and make it safe, I don't think we can open it for all users (of course, there should be a knob to open for any or restricted users.) > So we need both a maintainable and a sane/safe solution, and I'd like > to apply the whole thing at once and be at ease that the solution is > round. We should have done this years ago. For the safeness of kprobes, I have an idea; introduce a whitelist for dynamic events. AFAICS, the biggest unstable issue of kprobes comes from putting *many* probes on the functions called from tracers. It doesn't crash the kernel but slows down so much, because every probes hit many other nested miss-hit probes. This gives us a big performance impact. However, on the other side, this kind of feature can be used *for debugging* static trace events by dynamic one if we carefully use a small number of probes on such functions. :) Thus, I think we can restrict users from probing such functions by using a whitelist which ftrace does already have; available_filter_functions :) Then, I'd like to propose this new whitelist feature in kprobe-tracer (not raw kprobe itself). And a sysctl knob for disabling the whitelist. That knob will be /proc/sys/debug/kprobe-event-whitelist and disabling it will mark kernel tainted so that we can check it from bug reports. > So could you please send a whole series that I can apply to -tip as a > work in progress tree, and then we can see what is left to be solved? Sure. :) BTW, would I better fold the cleanups for reducing the number of patches? Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
On 4 December 2013 13:09, Masami Hiramatsu wrote: > (2013/12/04 11:54), Sandeepa Prabhu wrote: >> On 4 December 2013 06:58, Masami Hiramatsu >> wrote: >>> Hi, >>> Here is the version 4 of NOKPORBE_SYMBOL series. >>> >>> In this version, I removed the cleanup patches and >>> add bugfixes I've found, since those bugs will be >>> critical. >>> Rest of the cleanup and visible blacklists will be >>> proposed later in another series. >>> >>> Oh, just one new thing, I added a new RFC patch which >>> removes the dependency of notify_die() from kprobes >>> miss-hit/recovery path. Since the notify_die() involves >>> locking and lockdep code which invokes a lot of heavy >>> printk functions etc. This helped me to minimize the >>> blacklist and provides more stability for kprobes. >>> Actually, most of int3 handlers are already called >>> from do_int3 directly, I think this change is acceptable >>> too. >>> >>> Here is the updates about NOKPROBE_SYMBOL(). >>> - Now _ASM_NOKPROBE() macro is introduced for assembly >>>symbols on x86. >>> - Rename kprobe_blackpoint to kprobe_blacklist_entry >>>and simplify it. Also NOKPROBE_SYMBOL() macro just >>>saves the address of non-probe-able symbols. >>> >>> --- >>> >>> Masami Hiramatsu (6): >> >>> kprobes: Prohibit probing on .entry.text code >>> kprobes: Introduce NOKPROBE_SYMBOL() macro for blacklist >> Hi Masami, >> Is it good idea to split "arch/x86" code from generic kernel changes? >> Then we just need to take above two patches for verifying it on arm64 >> or other platforms. > > Yeah, it can be. > However I think you can apply it without any problem on arm64 tree too, > since it "just adds" an asm macro in arch/x86/include/asm/asm.h. > It should not have any effect for other arch. Could you try it? :) Hmm, for the second patch, git am failed with: "error: patch failed: kernel/sched/core.c:2662", manually patched to resolve it. aarch64 tree is right now at Linux 3.13-rc2. Anyways, no conflicts for x86 arch files. Thanks, Sandeepa > Thank you, > > > -- > Masami HIRAMATSU > IT Management Research Dept. Linux Technology Center > Hitachi, Ltd., Yokohama Research Laboratory > E-mail: masami.hiramatsu...@hitachi.com > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
* Masami Hiramatsu wrote: > Hi, > Here is the version 4 of NOKPORBE_SYMBOL series. > > In this version, I removed the cleanup patches and > add bugfixes I've found, since those bugs will be > critical. > > Rest of the cleanup and visible blacklists will be proposed later in > another series. Ok, let me make it clear: we need _both_ the conceptual cleanups and the bug fixes. Right now kprobes are restricted to root, and they are unsafe and buggy, and rather fundamentally so, because probing cannot be done safely without potentially crashing the kernel. So there's no 'regression' to be fixed, it's mostly about pre-existing bugs - so there's no requirement for them to come before maintainability cleanups. So we need both a maintainable and a sane/safe solution, and I'd like to apply the whole thing at once and be at ease that the solution is round. We should have done this years ago. So could you please send a whole series that I can apply to -tip as a work in progress tree, and then we can see what is left to be solved? Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
On 4 December 2013 13:09, Masami Hiramatsu masami.hiramatsu...@hitachi.com wrote: (2013/12/04 11:54), Sandeepa Prabhu wrote: On 4 December 2013 06:58, Masami Hiramatsu masami.hiramatsu...@hitachi.com wrote: Hi, Here is the version 4 of NOKPORBE_SYMBOL series. In this version, I removed the cleanup patches and add bugfixes I've found, since those bugs will be critical. Rest of the cleanup and visible blacklists will be proposed later in another series. Oh, just one new thing, I added a new RFC patch which removes the dependency of notify_die() from kprobes miss-hit/recovery path. Since the notify_die() involves locking and lockdep code which invokes a lot of heavy printk functions etc. This helped me to minimize the blacklist and provides more stability for kprobes. Actually, most of int3 handlers are already called from do_int3 directly, I think this change is acceptable too. Here is the updates about NOKPROBE_SYMBOL(). - Now _ASM_NOKPROBE() macro is introduced for assembly symbols on x86. - Rename kprobe_blackpoint to kprobe_blacklist_entry and simplify it. Also NOKPROBE_SYMBOL() macro just saves the address of non-probe-able symbols. --- Masami Hiramatsu (6): kprobes: Prohibit probing on .entry.text code kprobes: Introduce NOKPROBE_SYMBOL() macro for blacklist Hi Masami, Is it good idea to split arch/x86 code from generic kernel changes? Then we just need to take above two patches for verifying it on arm64 or other platforms. Yeah, it can be. However I think you can apply it without any problem on arm64 tree too, since it just adds an asm macro in arch/x86/include/asm/asm.h. It should not have any effect for other arch. Could you try it? :) Hmm, for the second patch, git am failed with: error: patch failed: kernel/sched/core.c:2662, manually patched to resolve it. aarch64 tree is right now at Linux 3.13-rc2. Anyways, no conflicts for x86 arch files. Thanks, Sandeepa Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
* Masami Hiramatsu masami.hiramatsu...@hitachi.com wrote: Hi, Here is the version 4 of NOKPORBE_SYMBOL series. In this version, I removed the cleanup patches and add bugfixes I've found, since those bugs will be critical. Rest of the cleanup and visible blacklists will be proposed later in another series. Ok, let me make it clear: we need _both_ the conceptual cleanups and the bug fixes. Right now kprobes are restricted to root, and they are unsafe and buggy, and rather fundamentally so, because probing cannot be done safely without potentially crashing the kernel. So there's no 'regression' to be fixed, it's mostly about pre-existing bugs - so there's no requirement for them to come before maintainability cleanups. So we need both a maintainable and a sane/safe solution, and I'd like to apply the whole thing at once and be at ease that the solution is round. We should have done this years ago. So could you please send a whole series that I can apply to -tip as a work in progress tree, and then we can see what is left to be solved? Thanks, Ingo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
(2013/12/04 17:45), Ingo Molnar wrote: * Masami Hiramatsu masami.hiramatsu...@hitachi.com wrote: Hi, Here is the version 4 of NOKPORBE_SYMBOL series. In this version, I removed the cleanup patches and add bugfixes I've found, since those bugs will be critical. Rest of the cleanup and visible blacklists will be proposed later in another series. Ok, let me make it clear: we need _both_ the conceptual cleanups and the bug fixes. I see. Why I split this out is because it includes an RFC patch, and for easier review. I still have a series of cleanups :) Right now kprobes are restricted to root, and they are unsafe and buggy, and rather fundamentally so, because probing cannot be done safely without potentially crashing the kernel. So there's no 'regression' to be fixed, it's mostly about pre-existing bugs - so there's no requirement for them to come before maintainability cleanups. OK, I think the kprobe is like a strong medicine, not a toy, since it can intercept most of the kernel functions which may process a sensitive user private data. Thus even if we fix all bugs and make it safe, I don't think we can open it for all users (of course, there should be a knob to open for any or restricted users.) So we need both a maintainable and a sane/safe solution, and I'd like to apply the whole thing at once and be at ease that the solution is round. We should have done this years ago. For the safeness of kprobes, I have an idea; introduce a whitelist for dynamic events. AFAICS, the biggest unstable issue of kprobes comes from putting *many* probes on the functions called from tracers. It doesn't crash the kernel but slows down so much, because every probes hit many other nested miss-hit probes. This gives us a big performance impact. However, on the other side, this kind of feature can be used *for debugging* static trace events by dynamic one if we carefully use a small number of probes on such functions. :) Thus, I think we can restrict users from probing such functions by using a whitelist which ftrace does already have; available_filter_functions :) Then, I'd like to propose this new whitelist feature in kprobe-tracer (not raw kprobe itself). And a sysctl knob for disabling the whitelist. That knob will be /proc/sys/debug/kprobe-event-whitelist and disabling it will mark kernel tainted so that we can check it from bug reports. So could you please send a whole series that I can apply to -tip as a work in progress tree, and then we can see what is left to be solved? Sure. :) BTW, would I better fold the cleanups for reducing the number of patches? Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
(2013/12/04 17:46), Sandeepa Prabhu wrote: On 4 December 2013 13:09, Masami Hiramatsu masami.hiramatsu...@hitachi.com wrote: (2013/12/04 11:54), Sandeepa Prabhu wrote: On 4 December 2013 06:58, Masami Hiramatsu masami.hiramatsu...@hitachi.com wrote: Hi, Here is the version 4 of NOKPORBE_SYMBOL series. In this version, I removed the cleanup patches and add bugfixes I've found, since those bugs will be critical. Rest of the cleanup and visible blacklists will be proposed later in another series. Oh, just one new thing, I added a new RFC patch which removes the dependency of notify_die() from kprobes miss-hit/recovery path. Since the notify_die() involves locking and lockdep code which invokes a lot of heavy printk functions etc. This helped me to minimize the blacklist and provides more stability for kprobes. Actually, most of int3 handlers are already called from do_int3 directly, I think this change is acceptable too. Here is the updates about NOKPROBE_SYMBOL(). - Now _ASM_NOKPROBE() macro is introduced for assembly symbols on x86. - Rename kprobe_blackpoint to kprobe_blacklist_entry and simplify it. Also NOKPROBE_SYMBOL() macro just saves the address of non-probe-able symbols. --- Masami Hiramatsu (6): kprobes: Prohibit probing on .entry.text code kprobes: Introduce NOKPROBE_SYMBOL() macro for blacklist Hi Masami, Is it good idea to split arch/x86 code from generic kernel changes? Then we just need to take above two patches for verifying it on arm64 or other platforms. Yeah, it can be. However I think you can apply it without any problem on arm64 tree too, since it just adds an asm macro in arch/x86/include/asm/asm.h. It should not have any effect for other arch. Could you try it? :) Hmm, for the second patch, git am failed with: error: patch failed: kernel/sched/core.c:2662, manually patched to resolve it. aarch64 tree is right now at Linux 3.13-rc2. Ah I see, that must be changed because it is the change related to introducing new blacklist itself. It is not solved by splitting arch/x86 change. Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
(2013/12/04 11:54), Sandeepa Prabhu wrote: > On 4 December 2013 06:58, Masami Hiramatsu > wrote: >> Hi, >> Here is the version 4 of NOKPORBE_SYMBOL series. >> >> In this version, I removed the cleanup patches and >> add bugfixes I've found, since those bugs will be >> critical. >> Rest of the cleanup and visible blacklists will be >> proposed later in another series. >> >> Oh, just one new thing, I added a new RFC patch which >> removes the dependency of notify_die() from kprobes >> miss-hit/recovery path. Since the notify_die() involves >> locking and lockdep code which invokes a lot of heavy >> printk functions etc. This helped me to minimize the >> blacklist and provides more stability for kprobes. >> Actually, most of int3 handlers are already called >> from do_int3 directly, I think this change is acceptable >> too. >> >> Here is the updates about NOKPROBE_SYMBOL(). >> - Now _ASM_NOKPROBE() macro is introduced for assembly >>symbols on x86. >> - Rename kprobe_blackpoint to kprobe_blacklist_entry >>and simplify it. Also NOKPROBE_SYMBOL() macro just >>saves the address of non-probe-able symbols. >> >> --- >> >> Masami Hiramatsu (6): > >> kprobes: Prohibit probing on .entry.text code >> kprobes: Introduce NOKPROBE_SYMBOL() macro for blacklist > Hi Masami, > Is it good idea to split "arch/x86" code from generic kernel changes? > Then we just need to take above two patches for verifying it on arm64 > or other platforms. Yeah, it can be. However I think you can apply it without any problem on arm64 tree too, since it "just adds" an asm macro in arch/x86/include/asm/asm.h. It should not have any effect for other arch. Could you try it? :) Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
On 4 December 2013 06:58, Masami Hiramatsu wrote: > Hi, > Here is the version 4 of NOKPORBE_SYMBOL series. > > In this version, I removed the cleanup patches and > add bugfixes I've found, since those bugs will be > critical. > Rest of the cleanup and visible blacklists will be > proposed later in another series. > > Oh, just one new thing, I added a new RFC patch which > removes the dependency of notify_die() from kprobes > miss-hit/recovery path. Since the notify_die() involves > locking and lockdep code which invokes a lot of heavy > printk functions etc. This helped me to minimize the > blacklist and provides more stability for kprobes. > Actually, most of int3 handlers are already called > from do_int3 directly, I think this change is acceptable > too. > > Here is the updates about NOKPROBE_SYMBOL(). > - Now _ASM_NOKPROBE() macro is introduced for assembly >symbols on x86. > - Rename kprobe_blackpoint to kprobe_blacklist_entry >and simplify it. Also NOKPROBE_SYMBOL() macro just >saves the address of non-probe-able symbols. > > --- > > Masami Hiramatsu (6): > kprobes: Prohibit probing on .entry.text code > kprobes: Introduce NOKPROBE_SYMBOL() macro for blacklist Hi Masami, Is it good idea to split "arch/x86" code from generic kernel changes? Then we just need to take above two patches for verifying it on arm64 or other platforms. Thanks, Sandeepa > [BUGFIX] kprobes/x86: Prohibit probing on debug_stack_* > [BUGFIX] x86: Prohibit probing on native_set_debugreg > [BUGFIX] x86: Prohibit probing on thunk functions and restore > [RFC] kprobes/x86: Call exception handlers directly from > do_int3/do_debug > > > Documentation/kprobes.txt | 16 + > arch/x86/include/asm/asm.h|7 ++ > arch/x86/include/asm/kprobes.h|2 + > arch/x86/kernel/cpu/common.c |4 + > arch/x86/kernel/entry_32.S| 33 --- > arch/x86/kernel/entry_64.S| 20 --- > arch/x86/kernel/kprobes/core.c| 32 -- > arch/x86/kernel/paravirt.c|5 ++ > arch/x86/kernel/traps.c | 10 +++ > arch/x86/lib/thunk_32.S |3 + > arch/x86/lib/thunk_64.S |3 + > include/asm-generic/vmlinux.lds.h |9 +++ > include/linux/kprobes.h | 21 ++- > kernel/kprobes.c | 113 > - > kernel/sched/core.c |1 > 15 files changed, 147 insertions(+), 132 deletions(-) > > -- > Masami HIRAMATSU > IT Management Research Dept. Linux Technology Center > Hitachi, Ltd., Yokohama Research Laboratory > E-mail: masami.hiramatsu...@hitachi.com > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
Hi, Here is the version 4 of NOKPORBE_SYMBOL series. In this version, I removed the cleanup patches and add bugfixes I've found, since those bugs will be critical. Rest of the cleanup and visible blacklists will be proposed later in another series. Oh, just one new thing, I added a new RFC patch which removes the dependency of notify_die() from kprobes miss-hit/recovery path. Since the notify_die() involves locking and lockdep code which invokes a lot of heavy printk functions etc. This helped me to minimize the blacklist and provides more stability for kprobes. Actually, most of int3 handlers are already called from do_int3 directly, I think this change is acceptable too. Here is the updates about NOKPROBE_SYMBOL(). - Now _ASM_NOKPROBE() macro is introduced for assembly symbols on x86. - Rename kprobe_blackpoint to kprobe_blacklist_entry and simplify it. Also NOKPROBE_SYMBOL() macro just saves the address of non-probe-able symbols. --- Masami Hiramatsu (6): kprobes: Prohibit probing on .entry.text code kprobes: Introduce NOKPROBE_SYMBOL() macro for blacklist [BUGFIX] kprobes/x86: Prohibit probing on debug_stack_* [BUGFIX] x86: Prohibit probing on native_set_debugreg [BUGFIX] x86: Prohibit probing on thunk functions and restore [RFC] kprobes/x86: Call exception handlers directly from do_int3/do_debug Documentation/kprobes.txt | 16 + arch/x86/include/asm/asm.h|7 ++ arch/x86/include/asm/kprobes.h|2 + arch/x86/kernel/cpu/common.c |4 + arch/x86/kernel/entry_32.S| 33 --- arch/x86/kernel/entry_64.S| 20 --- arch/x86/kernel/kprobes/core.c| 32 -- arch/x86/kernel/paravirt.c|5 ++ arch/x86/kernel/traps.c | 10 +++ arch/x86/lib/thunk_32.S |3 + arch/x86/lib/thunk_64.S |3 + include/asm-generic/vmlinux.lds.h |9 +++ include/linux/kprobes.h | 21 ++- kernel/kprobes.c | 113 - kernel/sched/core.c |1 15 files changed, 147 insertions(+), 132 deletions(-) -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
Hi, Here is the version 4 of NOKPORBE_SYMBOL series. In this version, I removed the cleanup patches and add bugfixes I've found, since those bugs will be critical. Rest of the cleanup and visible blacklists will be proposed later in another series. Oh, just one new thing, I added a new RFC patch which removes the dependency of notify_die() from kprobes miss-hit/recovery path. Since the notify_die() involves locking and lockdep code which invokes a lot of heavy printk functions etc. This helped me to minimize the blacklist and provides more stability for kprobes. Actually, most of int3 handlers are already called from do_int3 directly, I think this change is acceptable too. Here is the updates about NOKPROBE_SYMBOL(). - Now _ASM_NOKPROBE() macro is introduced for assembly symbols on x86. - Rename kprobe_blackpoint to kprobe_blacklist_entry and simplify it. Also NOKPROBE_SYMBOL() macro just saves the address of non-probe-able symbols. --- Masami Hiramatsu (6): kprobes: Prohibit probing on .entry.text code kprobes: Introduce NOKPROBE_SYMBOL() macro for blacklist [BUGFIX] kprobes/x86: Prohibit probing on debug_stack_* [BUGFIX] x86: Prohibit probing on native_set_debugreg [BUGFIX] x86: Prohibit probing on thunk functions and restore [RFC] kprobes/x86: Call exception handlers directly from do_int3/do_debug Documentation/kprobes.txt | 16 + arch/x86/include/asm/asm.h|7 ++ arch/x86/include/asm/kprobes.h|2 + arch/x86/kernel/cpu/common.c |4 + arch/x86/kernel/entry_32.S| 33 --- arch/x86/kernel/entry_64.S| 20 --- arch/x86/kernel/kprobes/core.c| 32 -- arch/x86/kernel/paravirt.c|5 ++ arch/x86/kernel/traps.c | 10 +++ arch/x86/lib/thunk_32.S |3 + arch/x86/lib/thunk_64.S |3 + include/asm-generic/vmlinux.lds.h |9 +++ include/linux/kprobes.h | 21 ++- kernel/kprobes.c | 113 - kernel/sched/core.c |1 15 files changed, 147 insertions(+), 132 deletions(-) -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
On 4 December 2013 06:58, Masami Hiramatsu masami.hiramatsu...@hitachi.com wrote: Hi, Here is the version 4 of NOKPORBE_SYMBOL series. In this version, I removed the cleanup patches and add bugfixes I've found, since those bugs will be critical. Rest of the cleanup and visible blacklists will be proposed later in another series. Oh, just one new thing, I added a new RFC patch which removes the dependency of notify_die() from kprobes miss-hit/recovery path. Since the notify_die() involves locking and lockdep code which invokes a lot of heavy printk functions etc. This helped me to minimize the blacklist and provides more stability for kprobes. Actually, most of int3 handlers are already called from do_int3 directly, I think this change is acceptable too. Here is the updates about NOKPROBE_SYMBOL(). - Now _ASM_NOKPROBE() macro is introduced for assembly symbols on x86. - Rename kprobe_blackpoint to kprobe_blacklist_entry and simplify it. Also NOKPROBE_SYMBOL() macro just saves the address of non-probe-able symbols. --- Masami Hiramatsu (6): kprobes: Prohibit probing on .entry.text code kprobes: Introduce NOKPROBE_SYMBOL() macro for blacklist Hi Masami, Is it good idea to split arch/x86 code from generic kernel changes? Then we just need to take above two patches for verifying it on arm64 or other platforms. Thanks, Sandeepa [BUGFIX] kprobes/x86: Prohibit probing on debug_stack_* [BUGFIX] x86: Prohibit probing on native_set_debugreg [BUGFIX] x86: Prohibit probing on thunk functions and restore [RFC] kprobes/x86: Call exception handlers directly from do_int3/do_debug Documentation/kprobes.txt | 16 + arch/x86/include/asm/asm.h|7 ++ arch/x86/include/asm/kprobes.h|2 + arch/x86/kernel/cpu/common.c |4 + arch/x86/kernel/entry_32.S| 33 --- arch/x86/kernel/entry_64.S| 20 --- arch/x86/kernel/kprobes/core.c| 32 -- arch/x86/kernel/paravirt.c|5 ++ arch/x86/kernel/traps.c | 10 +++ arch/x86/lib/thunk_32.S |3 + arch/x86/lib/thunk_64.S |3 + include/asm-generic/vmlinux.lds.h |9 +++ include/linux/kprobes.h | 21 ++- kernel/kprobes.c | 113 - kernel/sched/core.c |1 15 files changed, 147 insertions(+), 132 deletions(-) -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip v4 0/6] kprobes: introduce NOKPROBE_SYMBOL() and fixes crash bugs
(2013/12/04 11:54), Sandeepa Prabhu wrote: On 4 December 2013 06:58, Masami Hiramatsu masami.hiramatsu...@hitachi.com wrote: Hi, Here is the version 4 of NOKPORBE_SYMBOL series. In this version, I removed the cleanup patches and add bugfixes I've found, since those bugs will be critical. Rest of the cleanup and visible blacklists will be proposed later in another series. Oh, just one new thing, I added a new RFC patch which removes the dependency of notify_die() from kprobes miss-hit/recovery path. Since the notify_die() involves locking and lockdep code which invokes a lot of heavy printk functions etc. This helped me to minimize the blacklist and provides more stability for kprobes. Actually, most of int3 handlers are already called from do_int3 directly, I think this change is acceptable too. Here is the updates about NOKPROBE_SYMBOL(). - Now _ASM_NOKPROBE() macro is introduced for assembly symbols on x86. - Rename kprobe_blackpoint to kprobe_blacklist_entry and simplify it. Also NOKPROBE_SYMBOL() macro just saves the address of non-probe-able symbols. --- Masami Hiramatsu (6): kprobes: Prohibit probing on .entry.text code kprobes: Introduce NOKPROBE_SYMBOL() macro for blacklist Hi Masami, Is it good idea to split arch/x86 code from generic kernel changes? Then we just need to take above two patches for verifying it on arm64 or other platforms. Yeah, it can be. However I think you can apply it without any problem on arm64 tree too, since it just adds an asm macro in arch/x86/include/asm/asm.h. It should not have any effect for other arch. Could you try it? :) Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
