[PATCH -tip ] [BUGFIX] kprobes: Move hash_64() into .text.kprobe section

[PATCH -tip ] [BUGFIX] kprobes: Move hash_64() into .text.kprobe section

[Masami Hiramatsu]

Beacuse hash_64() is called from the get_kprobe() inside
int3 handler, kernel causes int3 recursion and crashes if
kprobes user puts a probe on it.

Usually hash_64() is inlined into caller function, but in
some cases, it has instances by gcc's interprocedural
constant propagation.

This patch adds __kprobes tag on the hash_64() and moves
all those instances into .text.kprobe section so that
kprobes can refuse probing on the instances.

I've ensured that all hash_64 instances moves to the
address between __kprobes_text_start and __kprobes_text_end
with this patch as below.

ffffffff8138bea0 T __kprobes_text_start
ffffffff8138bec0 t hash_64.constprop.8
ffffffff8138ef98 t hash_64.constprop.26
ffffffff8138efae t hash_64
ffffffff8138f066 t hash_64.constprop.43
ffffffff8138f649 t hash_64.constprop.25
ffffffff8139103a t hash_64.constprop.77
ffffffff81391050 t hash_64.constprop.24
ffffffff81391066 t hash_64.constprop.40
ffffffff8139107c t hash_64.constprop.15
ffffffff81391092 T __kprobes_text_end

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Reported-by: Timo Juhani Lindfors <timo.lindfors@iki.fi>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Nadia Yvette Chambers <nyc@holomorphy.com>
Cc: Pavel Emelyanov <xemul@parallels.com>
Cc: Jiri Kosina <jkosina@suse.cz>
Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
---
 include/linux/hash.h |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/hash.h b/include/linux/hash.h
index 61c97ae..d83f62f 100644
--- a/include/linux/hash.h
+++ b/include/linux/hash.h
@@ -15,6 +15,7 @@
  */
 
 #include <asm/types.h>
+#include <linux/kprobes.h>
 
 /* 2^31 + 2^29 - 2^25 + 2^22 - 2^19 - 2^16 + 1 */
 #define GOLDEN_RATIO_PRIME_32 0x9e370001UL
@@ -31,7 +32,7 @@
 #error Wordsize not 32 or 64
 #endif
 
-static inline u64 hash_64(u64 val, unsigned int bits)
+static __kprobes inline u64 hash_64(u64 val, unsigned int bits)
 {
 	u64 hash = val;
 

Re: [PATCH -tip ] [BUGFIX] kprobes: Move hash_64() into .text.kprobe section

[Ananth N Mavinakayanahalli]

On Mon, Mar 11, 2013 at 11:22:33PM +0900, Masami Hiramatsu wrote:
> Beacuse hash_64() is called from the get_kprobe() inside
> int3 handler, kernel causes int3 recursion and crashes if
> kprobes user puts a probe on it.
> 
> Usually hash_64() is inlined into caller function, but in
> some cases, it has instances by gcc's interprocedural
> constant propagation.
> 
> This patch adds __kprobes tag on the hash_64() and moves
> all those instances into .text.kprobe section so that
> kprobes can refuse probing on the instances.
> 
> I've ensured that all hash_64 instances moves to the
> address between __kprobes_text_start and __kprobes_text_end
> with this patch as below.
> 
> ffffffff8138bea0 T __kprobes_text_start
> ffffffff8138bec0 t hash_64.constprop.8
> ffffffff8138ef98 t hash_64.constprop.26
> ffffffff8138efae t hash_64
> ffffffff8138f066 t hash_64.constprop.43
> ffffffff8138f649 t hash_64.constprop.25
> ffffffff8139103a t hash_64.constprop.77
> ffffffff81391050 t hash_64.constprop.24
> ffffffff81391066 t hash_64.constprop.40
> ffffffff8139107c t hash_64.constprop.15
> ffffffff81391092 T __kprobes_text_end
> 
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Reported-by: Timo Juhani Lindfors <timo.lindfors@iki.fi>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Nadia Yvette Chambers <nyc@holomorphy.com>
> Cc: Pavel Emelyanov <xemul@parallels.com>
> Cc: Jiri Kosina <jkosina@suse.cz>
> Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>

Acked-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>

Re: [PATCH -tip ] [BUGFIX] kprobes: Move hash_64() into .text.kprobe section

[Ingo Molnar]

* Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:

> Beacuse hash_64() is called from the get_kprobe() inside
> int3 handler, kernel causes int3 recursion and crashes if
> kprobes user puts a probe on it.
> 
> Usually hash_64() is inlined into caller function, but in
> some cases, it has instances by gcc's interprocedural
> constant propagation.
> 
> This patch adds __kprobes tag on the hash_64() and moves
> all those instances into .text.kprobe section so that
> kprobes can refuse probing on the instances.
> 
> I've ensured that all hash_64 instances moves to the
> address between __kprobes_text_start and __kprobes_text_end
> with this patch as below.
> 
> ffffffff8138bea0 T __kprobes_text_start
> ffffffff8138bec0 t hash_64.constprop.8
> ffffffff8138ef98 t hash_64.constprop.26
> ffffffff8138efae t hash_64
> ffffffff8138f066 t hash_64.constprop.43
> ffffffff8138f649 t hash_64.constprop.25
> ffffffff8139103a t hash_64.constprop.77
> ffffffff81391050 t hash_64.constprop.24
> ffffffff81391066 t hash_64.constprop.40
> ffffffff8139107c t hash_64.constprop.15
> ffffffff81391092 T __kprobes_text_end
> 
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Reported-by: Timo Juhani Lindfors <timo.lindfors@iki.fi>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Nadia Yvette Chambers <nyc@holomorphy.com>
> Cc: Pavel Emelyanov <xemul@parallels.com>
> Cc: Jiri Kosina <jkosina@suse.cz>
> Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
> ---
>  include/linux/hash.h |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/hash.h b/include/linux/hash.h
> index 61c97ae..d83f62f 100644
> --- a/include/linux/hash.h
> +++ b/include/linux/hash.h
> @@ -15,6 +15,7 @@
>   */
>  
>  #include <asm/types.h>
> +#include <linux/kprobes.h>

I have no objections to the fix itself, but this inclusion of kprobes.h in 
hash.h is somewhat sad: kprobes.h is a 'fat' header that includes a lot of 
header files - while hash.h is a really basic type header included in 
close to a hundred .c files.

I think one solution would be for the __kprobes definition to move to a 
more basic header file - such as types.h or compiler.h (where the 
'notrace' attribute is placed too), to stop this header dependency creep.

Thanks,

	Ingo

Re: Re: [PATCH -tip ] [BUGFIX] kprobes: Move hash_64() into .text.kprobe section

[Masami Hiramatsu]

(2013/03/12 17:16), Ingo Molnar wrote:
> 
> * Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> 
>> Beacuse hash_64() is called from the get_kprobe() inside
>> int3 handler, kernel causes int3 recursion and crashes if
>> kprobes user puts a probe on it.
>>
>> Usually hash_64() is inlined into caller function, but in
>> some cases, it has instances by gcc's interprocedural
>> constant propagation.
>>
>> This patch adds __kprobes tag on the hash_64() and moves
>> all those instances into .text.kprobe section so that
>> kprobes can refuse probing on the instances.
>>
>> I've ensured that all hash_64 instances moves to the
>> address between __kprobes_text_start and __kprobes_text_end
>> with this patch as below.
>>
>> ffffffff8138bea0 T __kprobes_text_start
>> ffffffff8138bec0 t hash_64.constprop.8
>> ffffffff8138ef98 t hash_64.constprop.26
>> ffffffff8138efae t hash_64
>> ffffffff8138f066 t hash_64.constprop.43
>> ffffffff8138f649 t hash_64.constprop.25
>> ffffffff8139103a t hash_64.constprop.77
>> ffffffff81391050 t hash_64.constprop.24
>> ffffffff81391066 t hash_64.constprop.40
>> ffffffff8139107c t hash_64.constprop.15
>> ffffffff81391092 T __kprobes_text_end
>>
>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>> Reported-by: Timo Juhani Lindfors <timo.lindfors@iki.fi>
>> Cc: "David S. Miller" <davem@davemloft.net>
>> Cc: Nadia Yvette Chambers <nyc@holomorphy.com>
>> Cc: Pavel Emelyanov <xemul@parallels.com>
>> Cc: Jiri Kosina <jkosina@suse.cz>
>> Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
>> ---
>>  include/linux/hash.h |    3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/hash.h b/include/linux/hash.h
>> index 61c97ae..d83f62f 100644
>> --- a/include/linux/hash.h
>> +++ b/include/linux/hash.h
>> @@ -15,6 +15,7 @@
>>   */
>>  
>>  #include <asm/types.h>
>> +#include <linux/kprobes.h>
> 
> I have no objections to the fix itself, but this inclusion of kprobes.h in 
> hash.h is somewhat sad: kprobes.h is a 'fat' header that includes a lot of 
> header files - while hash.h is a really basic type header included in 
> close to a hundred .c files.
> 
> I think one solution would be for the __kprobes definition to move to a 
> more basic header file - such as types.h or compiler.h (where the 
> 'notrace' attribute is placed too), to stop this header dependency creep.

Agreed, that sounds nice to me too!

Thank you,

-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH -tip ] [BUGFIX] kprobes: Move hash_64() into .text.kprobe section

[Ingo Molnar]

* Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:

> @@ -31,7 +32,7 @@
>  #error Wordsize not 32 or 64
>  #endif
>  
> -static inline u64 hash_64(u64 val, unsigned int bits)
> +static __kprobes inline u64 hash_64(u64 val, unsigned int bits)
>  {
>  	u64 hash = val;

We should also, really, really fix the '__kprobes' misnomer and switch to 
the '__noprobe' pattern or so. The naming does not make it obvious at all 
that what we do here is to turn _off_ kprobing of select functions...

The only complication is that __kprobes is now present in 600+ places, 
which will create merge conflicts. If you remind me during the next merge 
window I can generate the rename on the spot and send it to Linus without 
anyone having to carry the patch for too long.

Thanks,

	Ingo

Re: [PATCH -tip ] [BUGFIX] kprobes: Move hash_64() into .text.kprobe section

[Masami Hiramatsu]

(2013/03/12 17:21), Ingo Molnar wrote:
> 
> * Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> 
>> @@ -31,7 +32,7 @@
>>  #error Wordsize not 32 or 64
>>  #endif
>>  
>> -static inline u64 hash_64(u64 val, unsigned int bits)
>> +static __kprobes inline u64 hash_64(u64 val, unsigned int bits)
>>  {
>>  	u64 hash = val;
> 
> We should also, really, really fix the '__kprobes' misnomer and switch to 
> the '__noprobe' pattern or so. The naming does not make it obvious at all 
> that what we do here is to turn _off_ kprobing of select functions...

Agreed.

> The only complication is that __kprobes is now present in 600+ places, 
> which will create merge conflicts. If you remind me during the next merge 
> window I can generate the rename on the spot and send it to Linus without 
> anyone having to carry the patch for too long.

Let me confirm that I just move __kprobes definition into
compiler.h this time, and ping you when the next merge
window opens, is that correct?

Thank you,

-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH -tip ] [BUGFIX] kprobes: Move hash_64() into .text.kprobe section

[Ingo Molnar]

* Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:

> > The only complication is that __kprobes is now present in 600+ places, 
> > which will create merge conflicts. If you remind me during the next 
> > merge window I can generate the rename on the spot and send it to 
> > Linus without anyone having to carry the patch for too long.
> 
> Let me confirm that I just move __kprobes definition into compiler.h 
> this time, and ping you when the next merge window opens, is that 
> correct?

Yeah, two stages - that would be cool.

Thanks,

	Ingo

Re: [PATCH -tip ] [BUGFIX] kprobes: Move hash_64() into .text.kprobe section

[Linus Torvalds]

On Mon, Mar 11, 2013 at 7:22 AM, Masami Hiramatsu
<masami.hiramatsu.pt@hitachi.com> wrote:
> Beacuse hash_64() is called from the get_kprobe() inside
> int3 handler, kernel causes int3 recursion and crashes if
> kprobes user puts a probe on it.
>
> Usually hash_64() is inlined into caller function, but in
> some cases, it has instances by gcc's interprocedural
> constant propagation.
>
> This patch adds __kprobes tag on the hash_64()

NAK. Don't do this. Just force inlining. There's absolutely no way we
want to start adding __kprobe to random helper functions like this.

This isn't even about where "__kprobes" exists and whether we want to
include the header file. This is about the fact that hash64 has
absolutely *nothing* to do with kprobes, and we simply shouldn't do
crap like this regardless of whether we need a new #include or not.

                  Linus

Re: [PATCH -tip ] [BUGFIX] kprobes: Move hash_64() into .text.kprobe section

[Masami Hiramatsu]

(2013/03/13 1:04), Linus Torvalds wrote:
> On Mon, Mar 11, 2013 at 7:22 AM, Masami Hiramatsu
> <masami.hiramatsu.pt@hitachi.com> wrote:
>> Beacuse hash_64() is called from the get_kprobe() inside
>> int3 handler, kernel causes int3 recursion and crashes if
>> kprobes user puts a probe on it.
>>
>> Usually hash_64() is inlined into caller function, but in
>> some cases, it has instances by gcc's interprocedural
>> constant propagation.
>>
>> This patch adds __kprobes tag on the hash_64()
> 
> NAK. Don't do this. Just force inlining. There's absolutely no way we
> want to start adding __kprobe to random helper functions like this.

I see.

> This isn't even about where "__kprobes" exists and whether we want to
> include the header file. This is about the fact that hash64 has
> absolutely *nothing* to do with kprobes, and we simply shouldn't do
> crap like this regardless of whether we need a new #include or not.

OK, then I'll update it to just use __always_inline.

Thank you!

-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH -tip ] [BUGFIX] kprobes: Move hash_64() into .text.kprobe section

[Timo Juhani Lindfors]

Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> writes:
> OK, then I'll update it to just use __always_inline.

I get a similar case of infinite recursion if I try to kprobe
"inat_get_opcode_attribute":

PID: 3028   TASK: ffff88003c67e8c0  CPU: 1   COMMAND: "insmod"
 #0 [ffff88003d60b9b8] __schedule at ffffffff813777f8
 #1 [ffff88003d60b9d0] inat_get_opcode_attribute at ffffffff811c95a9
 #2 [ffff88003d60b9e0] notifier_call_chain at ffffffff8137b5a3
 #3 [ffff88003d60ba20] notify_die at ffffffff8137b60c
 #4 [ffff88003d60ba50] do_int3 at ffffffff81378fa0
 #5 [ffff88003d60ba70] xen_int3 at ffffffff8137887e
    [exception RIP: inat_get_opcode_attribute+1]
    RIP: ffffffff811c95a9  RSP: ffff88003d60bb20  RFLAGS: 00000006
    RAX: 0000000000000200  RBX: ffffffffa00070f0  RCX: 00000000ffffffff
    RDX: ffff88003f80dd90  RSI: ffff88003d60bcc8  RDI: 0000000000000040
    RBP: ffffffffa019b000   R8: 0000000000000000   R9: ffffffff81629b10
    R10: 00000000000066a8  R11: ffffffffa019b000  R12: ffff88003f80dd90
    R13: ffffffff811c95a8  R14: ffffffff811c95a9  R15: ffffffffa019b010
    ORIG_RAX: ffffffffffffffff  CS: 10000e030  SS: e02b
 #6 [ffff88003d60bb20] skip_prefixes at ffffffff81379b6e
 #7 [ffff88003d60bb30] set_current_kprobe.isra.4 at ffffffff81379bb0
 #8 [ffff88003d60bb40] kprobe_exceptions_notify at ffffffff8137a446
 #9 [ffff88003d60bba0] notifier_call_chain at ffffffff8137b5a3
#10 [ffff88003d60bbe0] notify_die at ffffffff8137b60c
#11 [ffff88003d60bc10] do_int3 at ffffffff81378fa0
#12 [ffff88003d60bc30] xen_int3 at ffffffff8137887e
    [exception RIP: inat_get_opcode_attribute+1]
    RIP: ffffffff811c95a9  RSP: ffff88003d60bce0  RFLAGS: 00000246
    RAX: 0000000000000001  RBX: ffff88003d60bdb0  RCX: 0000000000000000
    RDX: ffff88003d60be10  RSI: ffff88003d60be10  RDI: 0000000000000040
    RBP: 0000000000000000   R8: ffff88003d60bdb0   R9: ffffffff811c95a8
    R10: 00000000000066a8  R11: ffffffffa019b000  R12: ffffffff811c9540
    R13: ffffffff811c95ad  R14: 0000000000000000  R15: ffffffffa019b010
    ORIG_RAX: ffffffffffffffff  CS: e030  SS: e02b
#13 [ffff88003d60bce0] insn_get_prefixes at ffffffff811c9721
#14 [ffff88003d60bd10] insn_get_opcode at ffffffff811c9923
#15 [ffff88003d60bd30] insn_get_modrm at ffffffff811c9a2e
#16 [ffff88003d60bd50] insn_get_sib at ffffffff811c9af8
#17 [ffff88003d60bd60] insn_get_displacement at ffffffff811c9b5d
#18 [ffff88003d60bd70] insn_get_immediate at ffffffff811c9c48
#19 [ffff88003d60bd80] insn_get_length at ffffffff811c9f97
#20 [ffff88003d60bd90] can_optimize at ffffffff8137a96e
#21 [ffff88003d60be50] arch_prepare_optimized_kprobe at ffffffff8137ab2c
#22 [ffff88003d60bea0] alloc_aggr_kprobe.isra.17 at ffffffff8137bb9b
#23 [ffff88003d60bec0] register_kprobe at ffffffff8137cf16
#24 [ffff88003d60bf00] init_module at ffffffffa001101b [testcase1]
#25 [ffff88003d60bf10] do_one_initcall at ffffffff810020b6
#26 [ffff88003d60bf40] sys_init_module at ffffffff81083c4f
#27 [ffff88003d60bf80] system_call_fastpath at ffffffff8137d6e9
    RIP: 00007f0fec23814a  RSP: 00007fff29328218  RFLAGS: 00000206
    RAX: 00000000000000af  RBX: ffffffff8137d6e9  RCX: 00007f0fec23448a
    RDX: 00007f0fed0b0010  RSI: 000000000002be0b  RDI: 00007f0fec8df000
    RBP: 00007f0fed0b11d0   R8: 0000000000000003   R9: 0000000000000000
    R10: 00007f0fec23448a  R11: 0000000000000206  R12: 00007f0fed0b0010
    R13: 00007f0fed0b12a0  R14: 00007f0fed0b00c0  R15: 0000000000000000
    ORIG_RAX: 00000000000000af  CS: e033  SS: e02b

Re: [PATCH -tip ] [BUGFIX] kprobes: Move hash_64() into .text.kprobe section

[Masami Hiramatsu]

(2013/03/13 22:28), Timo Juhani Lindfors wrote:
> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> writes:
>> OK, then I'll update it to just use __always_inline.
> 
> I get a similar case of infinite recursion if I try to kprobe
> "inat_get_opcode_attribute":

Oops, right! And this is caused by below callchain
set_current_kprobes->is_IF_modifier->skip_prefixes
->inat_get_opcode_attribute
However, this looks very wired that the copied instruction
(ainsn->insn) is analyzed at every probe hit.
I think we should add a bit flag indicating IF modifier
to the ainsn.

Thank you for reporting!!

-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH -tip ] [BUGFIX] kprobes: Move hash_64() into .text.kprobe section

[Timo Juhani Lindfors]

Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> writes:
> Thank you for reporting!!

Thanks for fixing these! I spent some time trying to automate the
process of finding sensitive functions and eventually resorted into
booting a kvm instance with a minimal initrd to test every single
function in a clean and reproducible environment.

I found 7 more cases where calling register_kprobe() leads to an instant
kernel panic:

__flush_tlb_single
native_flush_tlb
native_safe_halt
native_set_pgd
native_set_pmd
native_set_pud
native_write_cr0

You can see full kernel console output for each function at
http://lindi.iki.fi/lindi/linux/kprobes/panics_2013-03-18/

Re: Re: [PATCH -tip ] [BUGFIX] kprobes: Move hash_64() into .text.kprobe section

[Masami Hiramatsu]

(2013/03/19 5:57), Timo Juhani Lindfors wrote:
> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> writes:
>> Thank you for reporting!!
> 
> Thanks for fixing these! I spent some time trying to automate the
> process of finding sensitive functions and eventually resorted into
> booting a kvm instance with a minimal initrd to test every single
> function in a clean and reproducible environment.
> 
> I found 7 more cases where calling register_kprobe() leads to an instant
> kernel panic:
> 
> __flush_tlb_single
> native_flush_tlb
> native_safe_halt
> native_set_pgd
> native_set_pmd
> native_set_pud
> native_write_cr0

Ah, right and Great! these native_* things are too fundamental one.
Hmm, curiously, those are defined as inline functions, and
I also couldn't find some of those symbols even with your previous
kconfig.

> You can see full kernel console output for each function at
> http://lindi.iki.fi/lindi/linux/kprobes/panics_2013-03-18/

As you can see, your panic messages, most of them caused GFP.
This may mean that int3 software exception must not happened
on those sites. Not the recursive call.

Perhaps, I'd better add those native_* things into symbol-name
based blacklist, instead of adding __kprobes, because those
are not related to kprobes recursion.

Thank you,

-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: Re: [PATCH -tip ] [BUGFIX] kprobes: Move hash_64() into .text.kprobe section

[Ingo Molnar]

* Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:

> (2013/03/19 5:57), Timo Juhani Lindfors wrote:
> > Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> writes:
> >> Thank you for reporting!!
> > 
> > Thanks for fixing these! I spent some time trying to automate the
> > process of finding sensitive functions and eventually resorted into
> > booting a kvm instance with a minimal initrd to test every single
> > function in a clean and reproducible environment.
> > 
> > I found 7 more cases where calling register_kprobe() leads to an instant
> > kernel panic:
> > 
> > __flush_tlb_single
> > native_flush_tlb
> > native_safe_halt
> > native_set_pgd
> > native_set_pmd
> > native_set_pud
> > native_write_cr0
> 
> Ah, right and Great! these native_* things are too fundamental one.
> Hmm, curiously, those are defined as inline functions, and
> I also couldn't find some of those symbols even with your previous
> kconfig.
> 
> > You can see full kernel console output for each function at
> > http://lindi.iki.fi/lindi/linux/kprobes/panics_2013-03-18/
> 
> As you can see, your panic messages, most of them caused GFP.
> This may mean that int3 software exception must not happened
> on those sites. Not the recursive call.
> 
> Perhaps, I'd better add those native_* things into symbol-name
> based blacklist, instead of adding __kprobes, because those
> are not related to kprobes recursion.

Blacklists are not really good in general - it's easy for a symbol to be 
renamed and the blacklist misses them silently ...

symbol name and annotation should go hand in hand.

Thanks,

	Ingo

Re: Re: Re: [PATCH -tip ] [BUGFIX] kprobes: Move hash_64() into .text.kprobe section

[Masami Hiramatsu]

(2013/03/21 20:39), Ingo Molnar wrote:
> 
> * Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> 
>> (2013/03/19 5:57), Timo Juhani Lindfors wrote:
>>> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> writes:
>>>> Thank you for reporting!!
>>>
>>> Thanks for fixing these! I spent some time trying to automate the
>>> process of finding sensitive functions and eventually resorted into
>>> booting a kvm instance with a minimal initrd to test every single
>>> function in a clean and reproducible environment.
>>>
>>> I found 7 more cases where calling register_kprobe() leads to an instant
>>> kernel panic:
>>>
>>> __flush_tlb_single
>>> native_flush_tlb
>>> native_safe_halt
>>> native_set_pgd
>>> native_set_pmd
>>> native_set_pud
>>> native_write_cr0
>>
>> Ah, right and Great! these native_* things are too fundamental one.
>> Hmm, curiously, those are defined as inline functions, and
>> I also couldn't find some of those symbols even with your previous
>> kconfig.
>>
>>> You can see full kernel console output for each function at
>>> http://lindi.iki.fi/lindi/linux/kprobes/panics_2013-03-18/
>>
>> As you can see, your panic messages, most of them caused GFP.
>> This may mean that int3 software exception must not happened
>> on those sites. Not the recursive call.
>>
>> Perhaps, I'd better add those native_* things into symbol-name
>> based blacklist, instead of adding __kprobes, because those
>> are not related to kprobes recursion.
> 
> Blacklists are not really good in general - it's easy for a symbol to be 
> renamed and the blacklist misses them silently ...

Ah, right.

> 
> symbol name and annotation should go hand in hand.

Thus, I think we'd better moving __kprobes into compiler.h first.

Anyway, I'm still waiting for the actual kconfig from Timo,
because I couldn't reproduce the problem yet (no such symbols).

Thank you,

-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com