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 <[email protected]>
Reported-by: Timo Juhani Lindfors <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Nadia Yvette Chambers <[email protected]>
Cc: Pavel Emelyanov <[email protected]>
Cc: Jiri Kosina <[email protected]>
Cc: Ananth N Mavinakayanahalli <[email protected]>
---
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;
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 <[email protected]>
> Reported-by: Timo Juhani Lindfors <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: Nadia Yvette Chambers <[email protected]>
> Cc: Pavel Emelyanov <[email protected]>
> Cc: Jiri Kosina <[email protected]>
> Cc: Ananth N Mavinakayanahalli <[email protected]>
Acked-by: Ananth N Mavinakayanahalli <[email protected]>
* Masami Hiramatsu <[email protected]> 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 <[email protected]>
> Reported-by: Timo Juhani Lindfors <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: Nadia Yvette Chambers <[email protected]>
> Cc: Pavel Emelyanov <[email protected]>
> Cc: Jiri Kosina <[email protected]>
> Cc: Ananth N Mavinakayanahalli <[email protected]>
> ---
> 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
* Masami Hiramatsu <[email protected]> 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
(2013/03/12 17:16), Ingo Molnar wrote:
>
> * Masami Hiramatsu <[email protected]> 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 <[email protected]>
>> Reported-by: Timo Juhani Lindfors <[email protected]>
>> Cc: "David S. Miller" <[email protected]>
>> Cc: Nadia Yvette Chambers <[email protected]>
>> Cc: Pavel Emelyanov <[email protected]>
>> Cc: Jiri Kosina <[email protected]>
>> Cc: Ananth N Mavinakayanahalli <[email protected]>
>> ---
>> 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: [email protected]
(2013/03/12 17:21), Ingo Molnar wrote:
>
> * Masami Hiramatsu <[email protected]> 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: [email protected]
* Masami Hiramatsu <[email protected]> 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
On Mon, Mar 11, 2013 at 7:22 AM, Masami Hiramatsu
<[email protected]> 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
(2013/03/13 1:04), Linus Torvalds wrote:
> On Mon, Mar 11, 2013 at 7:22 AM, Masami Hiramatsu
> <[email protected]> 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: [email protected]
Masami Hiramatsu <[email protected]> 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
(2013/03/13 22:28), Timo Juhani Lindfors wrote:
> Masami Hiramatsu <[email protected]> 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: [email protected]
Masami Hiramatsu <[email protected]> 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/
(2013/03/19 5:57), Timo Juhani Lindfors wrote:
> Masami Hiramatsu <[email protected]> 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: [email protected]
* Masami Hiramatsu <[email protected]> wrote:
> (2013/03/19 5:57), Timo Juhani Lindfors wrote:
> > Masami Hiramatsu <[email protected]> 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
(2013/03/21 20:39), Ingo Molnar wrote:
>
> * Masami Hiramatsu <[email protected]> wrote:
>
>> (2013/03/19 5:57), Timo Juhani Lindfors wrote:
>>> Masami Hiramatsu <[email protected]> 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: [email protected]