From mboxrd@z Thu Jan 1 00:00:00 1970 Sender: Ingo Molnar Date: Mon, 16 Jul 2018 12:13:38 +0200 From: Ingo Molnar Subject: Re: [PATCH v14 0/6] Introduce the STACKLEAK feature and a test for it Message-ID: <20180716101337.GA30279@gmail.com> References: <1531341400-12077-1-git-send-email-alex.popov@linux.com> <20180712135944.GB6199@gmail.com> <20180712205016.GA20649@gmail.com> <50c690fc-c967-8376-8524-ac9c1be722bc@linux.com> <20180715224446.GA29087@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: To: Alexander Popov Cc: Kees Cook , Linus Torvalds , Kernel Hardening , Pax Team , Brad Spengler , Andrew Lutomirski , Tycho Andersen , Laura Abbott , Mark Rutland , Ard Biesheuvel , Borislav Petkov , Richard Sandiford , Thomas Gleixner , Peter Anvin , Peter Zijlstra , "Dmitry V. Levin" , Emese Revfy , Jonathan Corbet , Andrey Ryabinin , "Kirill A. Shutemov" , Thomas Garnier , Andrew Morton , Alexei Starovoitov , Josef Bacik , Masami Hiramatsu , Nick Piggin , Al Viro , David Miller , dingtianhong , David Woodhouse , Josh Poimboeuf , Steven Rostedt , Dominik Brodowski , =?iso-8859-1?Q?J=FCrgen_Gro=DF?= , Greg Kroah-Hartman , Dan Williams , Dave Hansen , Mathias Krause , Vikas Shivappa , Kyle Huey , Dmitry Safonov , Will Deacon , Arnd Bergmann , Florian Weimer , Boris Lukashev , Andrey Konovalov , the arch/x86 maintainers , Linux Kernel Mailing List List-ID: * Alexander Popov wrote: > On 16.07.2018 01:44, Ingo Molnar wrote: > > > > * Kees Cook wrote: > > > >> On Thu, Jul 12, 2018 at 2:22 PM, Alexander Popov wrote: > >>> On 12.07.2018 23:50, Ingo Molnar wrote: > >>>> Let's make sure informed users have an easy runtime way out > >>>> from the worst of the overhead that doesn't involve "recompile your distro > >>>> kernel". Also, make it easier to measure and fingerpoint the overhead... > >>> > >>> Would you like the following solution? > >>> > >>> I'll create the CONFIG_STACKLEAK_RUNTIME_DISABLE config option, which would be > >>> similar to CONFIG_SECURITY_SELINUX_DISABLE. That option will provide a sysctl > >>> switch disabling stackleak_erase(), which provides the most of performance penalty. > >> > >> I don't think CONFIG/sysctl is the way to go. I'd recommend making it > >> a boot arg and using a static key, similar to what's happening to > >> hardened_usercopy: > > > > Why no sysctl? It's a PITA to reboot systems just to turn a stupid knob off. > > > > Also, it's _much_ easier to measure performance impact when there's a sysctl. > > Yes, you are right. > > But I looked carefully and now see the troubles which sysctl would bring us. > Each 'task_struct' has 'lowest_stack', which must be initialized before enabling > STACKLEAK. So runtime enabling via sysctl is not plain and may bring race > conditions. Firstly, a sysctl could still allow it to be *disabled*, once - which is the most important usecase of the sysctl anyway. Secondly, in the first iteration this could be kept included unconditionally: current->lowest_stack = current_top_of_stack() - THREAD_SIZE/64; ... which would keep it initialized and wouldn't be racy, right? I.e. only the most expensive part of the function, the scanning, would be turned off via the sysctl. I submit that this will avoid all measurable aspects of the 1% kbuild performance overhead. A more involved approach can be done in the future if warranted, and the feature could be disabled/enabled more thoroughly - but the runtime sysctl would be acceptable for me for now, as a first iteration. > On the other hand, a boot param + static key that can only disable STACKLEAK > during __init are much more robust. I'm preparing the patch and performance > measurements. A boot parameter does *not* address the concern I outlined. Why force admins reboot a box just to disable something that causes overhead? Thanks, Ingo