Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965711AbdGTPvq (ORCPT ); Thu, 20 Jul 2017 11:51:46 -0400 Received: from mail.kernel.org ([198.145.29.99]:59004 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934635AbdGTPvp (ORCPT ); Thu, 20 Jul 2017 11:51:45 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 7C1A022B67 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=mhiramat@kernel.org Date: Fri, 21 Jul 2017 00:51:40 +0900 From: Masami Hiramatsu To: Linus Torvalds Cc: kernel test robot , Ananth N Mavinakayanahalli , Anil S Keshavamurthy , 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 Subject: Re: [lkp-robot] [include/linux/string.h] 6974f0c455: kernel_BUG_at_lib/string.c Message-Id: <20170721005140.ebf6d119bb30cc6cfe12ae61@kernel.org> In-Reply-To: References: <20170720014238.GH27396@yexl-desktop> X-Mailer: Sylpheed 3.5.0 (GTK+ 2.24.30; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3835 Lines: 106 On Wed, 19 Jul 2017 21:04:25 -0700 Linus Torvalds wrote: > 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? > BTW, I've confirmed this works. Acked-by: Masami Hiramatsu Tested-by: Masami Hiramatsu Thank you! > 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 -- Masami Hiramatsu