Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751984AbdGTEE1 (ORCPT ); Thu, 20 Jul 2017 00:04:27 -0400 Received: from mail-oi0-f67.google.com ([209.85.218.67]:38299 "EHLO mail-oi0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751876AbdGTEE0 (ORCPT ); Thu, 20 Jul 2017 00:04:26 -0400 MIME-Version: 1.0 In-Reply-To: <20170720014238.GH27396@yexl-desktop> References: <20170720014238.GH27396@yexl-desktop> From: Linus Torvalds Date: Wed, 19 Jul 2017 21:04:25 -0700 X-Google-Sender-Auth: lMnfnsxEivSnUmBkKGOiEcGK-eY Message-ID: Subject: Re: [lkp-robot] [include/linux/string.h] 6974f0c455: kernel_BUG_at_lib/string.c To: kernel test robot , Ananth N Mavinakayanahalli , Anil S Keshavamurthy , Masami Hiramatsu Cc: Daniel Micay , Kees Cook , Arnd Bergmann , Mark Rutland , Daniel Axtens , Rasmus Villemoes , Andy Shevchenko , Chris Metcalf , Thomas Gleixner , "H. Peter Anvin" , Ingo Molnar , Andrew Morton , LKML , LKP Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3372 Lines: 91 Hmm. I wonder why the kernel test robot ends up having that annoying line doubling for the dmesg. On Wed, Jul 19, 2017 at 6:42 PM, kernel test robot wrote: > > FYI, we noticed the following commit: > > commit: 6974f0c4555e285ab217cee58b6e874f776ff409 ("include/linux/string.h: add the option of fortified string.h functions") > > caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace): It does strike me that the fortify_panic() code once again makes life unnecessarily hard for everybody by using "BUG()" What the hell is wrong with people? I feel; like I have to say this multiple times for every single merge window, and sometimes in between. BUG() and BUG_ON() are not acceptable debugging things. They kill the machine. They make for bad debugging. > [ 8.134860] kernel BUG at lib/string.c:985! This is basically an entirely useless piece of completely information-less garbage. It would have been much nicer if all the fortify_panic() calls had instead used WARN_ONCE() with helpful pointers to what is going on. As it is, the full dmesg does show that detected buffer overflow in memcpy but since it was printed out separately, if comes before the "-- cut here --" part and didn't get reported in the summary. > [ 8.134886] arch_prepare_optimized_kprobe+0xd5/0x171 It's apparently this: /* Copy arch-dep-instance from template */ memcpy(buf, &optprobe_template_entry, TMPL_END_IDX); and looking at the code generation, the conditional in the fortify case is # ./include/linux/string.h:307: if (p_size < size || q_size < size) cmpq $1, %r13 #, _14 jbe .L109 #, but it's hard to tell whether that is p_size or q_size. One of them must be ~0ul (or maybe both are 1) for it to have simplified to that single conditional. So the fortify_string code has decided that only a single-byte (or empty) memcpy is ok. And that, in turn, seems to be because we're copying from optprobe_template_entry, which is declared as extern __visible kprobe_opcode_t optprobe_template_entry; so the fortify code decides it's a single character. Does just changing all those things to be declared as arrays fix things? IOW, a patch something like this white-space damaged mess: diff --git a/arch/x86/include/asm/kprobes.h b/arch/x86/include/asm/kprobes.h index 34b984c60790..6cf65437b5e5 100644 --- a/arch/x86/include/asm/kprobes.h +++ b/arch/x86/include/asm/kprobes.h @@ -52,10 +52,10 @@ typedef u8 kprobe_opcode_t; #define flush_insn_slot(p) do { } while (0) /* optinsn template addresses */ -extern __visible kprobe_opcode_t optprobe_template_entry; -extern __visible kprobe_opcode_t optprobe_template_val; -extern __visible kprobe_opcode_t optprobe_template_call; -extern __visible kprobe_opcode_t optprobe_template_end; +extern __visible kprobe_opcode_t optprobe_template_entry[]; +extern __visible kprobe_opcode_t optprobe_template_val[]; +extern __visible kprobe_opcode_t optprobe_template_call[]; +extern __visible kprobe_opcode_t optprobe_template_end[]; #define MAX_OPTIMIZED_LENGTH (MAX_INSN_SIZE + RELATIVE_ADDR_SIZE) #define MAX_OPTINSN_SIZE \ (((unsigned long)&optprobe_template_end - \ Hmm? Linus