Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755539AbbFBFsJ (ORCPT ); Tue, 2 Jun 2015 01:48:09 -0400 Received: from mail-wg0-f43.google.com ([74.125.82.43]:33638 "EHLO mail-wg0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752585AbbFBFr7 (ORCPT ); Tue, 2 Jun 2015 01:47:59 -0400 Date: Tue, 2 Jun 2015 07:47:50 +0200 From: Ingo Molnar To: Eugene Shatokhin Cc: Masami Hiramatsu , Andy Lutomirski , Ingo Molnar , LKML Subject: Re: [PATCH 2/2] kprobes/x86: Use 16 bytes for each instruction slot again Message-ID: <20150602054750.GB21382@gmail.com> References: <1433176331-479-1-git-send-email-eugene.shatokhin@rosalab.ru> <1433176331-479-3-git-send-email-eugene.shatokhin@rosalab.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1433176331-479-3-git-send-email-eugene.shatokhin@rosalab.ru> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2382 Lines: 66 * Eugene Shatokhin wrote: > Commit 91e5ed49fca0 ("x86/asm/decoder: Fix and enforce max instruction > size in the insn decoder") has changed MAX_INSN_SIZE from 16 to 15 bytes > on x86. > > As a side effect, the slots Kprobes use to store the instructions became > 1 byte shorter. This is unfortunate because, for example, the Kprobes' > "boost" feature can not be used now for the instructions of length 11, > like a quite common kind of MOV: > * movq $0xffffffffffffffff,-0x3fe8(%rax) (48 c7 80 18 c0 ff ff ff ff ff ff) > * movq $0x0,0x88(%rdi) (48 c7 87 88 00 00 00 00 00 00 00) > and so on. > > This patch makes the insn slots 16 bytes long, like they were before while > keeping MAX_INSN_SIZE intact. > > Other tools may benefit from this change as well. > > Signed-off-by: Eugene Shatokhin > --- > arch/x86/include/asm/kprobes.h | 1 + > arch/x86/kernel/kprobes/core.c | 2 +- > kernel/kprobes.c | 8 ++++++-- > 3 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/include/asm/kprobes.h b/arch/x86/include/asm/kprobes.h > index 4421b5d..f3f0b4e 100644 > --- a/arch/x86/include/asm/kprobes.h > +++ b/arch/x86/include/asm/kprobes.h > @@ -27,6 +27,7 @@ > #include > > #define __ARCH_WANT_KPROBES_INSN_SLOT > +#define KPROBE_INSN_SLOT_SIZE 16 Naming aside, it's totally unacceptable to add a define like this with no explanation in the code at all. > --- a/kernel/kprobes.c > +++ b/kernel/kprobes.c > @@ -90,6 +89,11 @@ static raw_spinlock_t *kretprobe_table_lock_ptr(unsigned long hash) > static LIST_HEAD(kprobe_blacklist); > > #ifdef __ARCH_WANT_KPROBES_INSN_SLOT > + > +#ifndef KPROBE_INSN_SLOT_SIZE > +#define KPROBE_INSN_SLOT_SIZE MAX_INSN_SIZE > +#endif Ditto. There's no explanation, no way for an arch maintainer to find out whether to redefine this or not. This adds some magic logic that is just as likely to break in the future as the old code did. This series needs to fix the primary cause (==poorly documented, illogical code), not just the symptom/bug. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/