From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755520AbcHWUmD (ORCPT ); Tue, 23 Aug 2016 16:42:03 -0400 Received: from mail.kernel.org ([198.145.29.136]:54446 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752910AbcHWUl7 (ORCPT ); Tue, 23 Aug 2016 16:41:59 -0400 MIME-Version: 1.0 In-Reply-To: References: <62fab36288792edae0181274641d6b4c62157fea.1471525031.git.jpoimboe@redhat.com> From: Andy Lutomirski Date: Tue, 23 Aug 2016 13:31:20 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to use the new unwinder To: Linus Torvalds Cc: Frederic Weisbecker , Thomas Gleixner , Kees Cook , Josh Poimboeuf , "H . Peter Anvin" , Nilay Vaish , "the arch/x86 maintainers" , Steven Rostedt , Ingo Molnar , Linux Kernel Mailing List , Brian Gerst , Byungchul Park , Peter Zijlstra Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Aug 23, 2016 12:11 AM, "Linus Torvalds" wrote: > > On Thu, Aug 18, 2016 at 6:06 AM, Josh Poimboeuf wrote: > > Convert arch_within_stack_frames() to use the new unwinder. > > Please don't do this. > > There's no real reason to unwind the stack frame. If it's not on the > current stack page, it shouldn't be a valid source anyway, so > unwidning things just seems entirely pointless. > > Quite frankly, I think the whole "look at the stack frames" logic > should be removed from this. It's classic crap that external patches > do. How many call-sites does it actually check, and how many of them > aren't already checked by the existing static checks for constant > addresses within existing objects? > I'm a bit confused by what you're objecting to. If I write: char buf[123]; func(buf, size); And func eventually does some usercopy to buf, the idea is to check that size is in bounds. Now admittedly this kind of code should be quite rare in the kernel, and it should be even rarer for the buffer to be more than a frame or two up the stack. So the fact that this seems to have any significant effect on performance suggests to me that it's being run unnecessarily or that somehow we're walking all the way to the top of the stack in cases where we shouldn't have done so. Josh, can you see an example call site in a profile of your test to find out what this code is doing? All that being said, Linus, assuming that Josh's new unwinder can be made reasonably performant, I don't understand your objection to this patch in particular. Josh isn't changing the way that usercopy hardening works -- he's just changing the stack walking implementation. It seems that you're objecting to this code in general, but that predates Josh's patch, no? --Andy