Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757966Ab3HHQZ4 (ORCPT ); Thu, 8 Aug 2013 12:25:56 -0400 Received: from mail-ve0-f179.google.com ([209.85.128.179]:61239 "EHLO mail-ve0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752234Ab3HHQZz (ORCPT ); Thu, 8 Aug 2013 12:25:55 -0400 MIME-Version: 1.0 In-Reply-To: <20130808154107.GA28971@redhat.com> References: <20130806154314.GA398@redhat.com> <20130807192734.GA8395@redhat.com> <20130808154107.GA28971@redhat.com> Date: Thu, 8 Aug 2013 09:25:54 -0700 X-Google-Sender-Auth: V6wqyeQFvgqkB5WMrdCKuUms35g Message-ID: Subject: Re: [PATCH 0/1] (Was: Linux 3.11-rc4) From: Linus Torvalds To: Oleg Nesterov Cc: Grazvydas Ignotas , Felipe Contreras , Linux Kernel Mailing List , Frederic Weisbecker , Ingo Molnar , Denys Vlasenko 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: 2267 Lines: 58 On Thu, Aug 8, 2013 at 8:41 AM, Oleg Nesterov wrote: > > On x86 execute breakpoints are only a single byte, which has to be > the first byte of the instruction. IOW the hardware requires len = 1 > in dr7 or it doesn't work (iirc). > > But for some reason perf requires bp_len = sizeof(long), not 1. And > note that it sets info->len = X86_BREAKPOINT_LEN_X. The comment says: > > x86 inst breakpoints need to have a specific undefined len > > but despite its "special" name LEN_X is simply LEN_1, and other code > relies on this fact. > > Now, ptrace correctly requires DR_LEN_1. So arch_bp_generic_fields() > translates this into "gen_len = sizeof(long)" for validation. Yeah, that just sounds insane. I suspect it's some misguided attempt to be compatible either with some broken old version of perf. But if so, I agree that the compatibility code should be elsewhere, and not in "let's turn the _correct_ length of 1 into some random crap because we screwed up elsewhere". >> But the kernel address checking definitely needs to stay around for >> security reasons. > > Sure. And btw it doesn't look right. I sent the patch below twice (iirc), > perhaps I should resend it again. Your patch looks correct. That said, > - return (va >= TASK_SIZE) && ((va + len - 1) >= TASK_SIZE); > + return (va >= TASK_SIZE) || ((va + len - 1) >= TASK_SIZE); I'd much rather make this be more clearly about overflow, and write this as something like last = va + len - 1; /* Check for overflow too */ if (last < va || end >= TASK_SIZE) because quite frankly, the "va >= TASK_SIZE" check is kind of insane. It makes very little semantic sense. The rewritten test can be seen as two independent tests that both make sense individually (the first checks for overflow, the second checks that the range isn't in kernel space). In fact, the overflow check could/should even be done in generic code, methinks. There's nothing architecture-specific about that. Linus -- 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/