From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753961AbcAOGG7 (ORCPT ); Fri, 15 Jan 2016 01:06:59 -0500 Received: from mx1.redhat.com ([209.132.183.28]:56208 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750850AbcAOGG5 (ORCPT ); Fri, 15 Jan 2016 01:06:57 -0500 Date: Fri, 15 Jan 2016 00:06:52 -0600 From: Josh Poimboeuf To: Ingo Molnar Cc: Borislav Petkov , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , x86@kernel.org, linux-kernel@vger.kernel.org, live-patching@vger.kernel.org, Michal Marek , Peter Zijlstra , Andy Lutomirski , Linus Torvalds , Andi Kleen , Pedro Alves , Namhyung Kim , Bernd Petrovitsch , Chris J Arges , Andrew Morton , Jiri Slaby , Arnaldo Carvalho de Melo Subject: Re: [PATCH v15 13/25] x86/reboot: Add ljmp instructions to stacktool whitelist Message-ID: <20160115060652.GA16760@treble.redhat.com> References: <20160112164711.GD22699@pd.tnic> <20160112174301.GD310@treble.redhat.com> <20160113105503.GB11575@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20160113105503.GB11575@gmail.com> User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jan 13, 2016 at 11:55:03AM +0100, Ingo Molnar wrote: > > * Josh Poimboeuf wrote: > > > > Well, I can't say that I'm crazy about all those new tools adding markers to > > > unrelated kernel code. > > > > > > Can't you teach stacktool to ignore the whole machine_real_restart() function > > > simply? > > > > Well, these STACKTOOL_IGNORE whitelist markers are only needed in a handful of > > places, and only for code that does very weird things. Yes, they're a bit ugly, > > but IMO they also communicate valuable information: "be careful, this code does > > something very weird." > > How common are these markers? Like with lockdep, it all depends on magnitude: > > - If it's less than 10 I'd say it's OK. > > - If it's dozens then it's ho-hum. > > - If certain types of annotations can go over 100, then they are unacceptable. > > all such in-code overhead has to be balanced against the utility of the tooling. Sounds reasonable. After looking at all the warnings on an allyesconfig kernel, I'm pretty sure it'll be less than 10. Most of the cases are mentioned below. > > As for whether to put the whitelist info in the code vs hard-coding it in > > stacktool, I think it's clearer and less "magical" to put them directly in the > > code. > > That's true - but I think Boris tried to ask something slightly different: can > stacktool be taught to detect weird signatures automatically, and to ignore them > automatically? > > Stuff like 16-bit code sure wounds 'weird' and the tool could detect that? Some of the 'weird' cases: - Some functions use 'ljmp' or 'lret'. After looking at this some more, I think it would be safe for stacktool to translate the use of these instructions to mean "I know what I'm doing" and just ignore the function. 100% of the functions are safe to ignore anyway. So we can get rid of those markers and just make stacktool smarter. - xen_cpuid() uses some custom xen instructions which start with XEN_EMULATE_PREFIX. It corresponds to the following x86 instructions: ffffffff8107e572: 0f 0b ud2 ffffffff8107e574: 78 65 js ffffffff8107e5db ffffffff8107e576: 6e outsb %ds:(%rsi),(%dx) Apparently(?) xen treats the ud2 special when it's followed by "78 65 6e". This is confusing for stacktool because ud2 is normally a dead end, and it thinks the instructions after it will never run. (In theory stacktool could be taught to understand this hack, but that's a bad idea IMO) - The error path in arch/x86/net/bpf_jit.S uses 'leaveq' to do a double return so that it returns from its caller's context. stacktool doesn't know how to distinguish this from a frame pointer programming bug. I think the only way to avoid a whitelist marker here would be to rewrite the bpf code to conform with more traditional rbp usage (but I don't know if that would really be a good idea because it would probably result in slower/more code). - __bpf_prog_run() uses a jump table: goto *jumptable[insn->code]; stacktool doesn't have an x86 emulator, so it doesn't know how to deterministically follow all possible branches for a dynamic jump. - schedule() mucks with the frame pointer which is normally not allowed. -- Josh