From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751607AbdK3T7m (ORCPT ); Thu, 30 Nov 2017 14:59:42 -0500 Received: from mx1.redhat.com ([209.132.183.28]:51316 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750761AbdK3T7k (ORCPT ); Thu, 30 Nov 2017 14:59:40 -0500 Date: Thu, 30 Nov 2017 13:59:39 -0600 From: Josh Poimboeuf To: Jiri Slaby Cc: mingo@redhat.com, linux-kernel@vger.kernel.org, Thomas Gleixner , "H. Peter Anvin" , x86@kernel.org, live-patching@vger.kernel.org Subject: Re: [PATCH 1/2] x86/stacktrace: do not fail when regs on stack for ORC Message-ID: <20171130195939.gx2qlewwjprfql42@treble> References: <20171130080325.24785-1-jslaby@suse.cz> <20171130195710.cu2lsa47kwnow2hi@treble> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20171130195710.cu2lsa47kwnow2hi@treble> User-Agent: Mutt/1.6.0.1 (2016-04-01) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Thu, 30 Nov 2017 19:59:40 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Nov 30, 2017 at 01:57:10PM -0600, Josh Poimboeuf wrote: > On Thu, Nov 30, 2017 at 09:03:24AM +0100, Jiri Slaby wrote: > > save_stack_trace_reliable now returns "non reliable" when there are > > kernel pt_regs on stack. This means an interrupt or exception happened. > > Somewhere down the route. It is a problem for frame pointer unwinder, > > because the frame might now have been set up yet when the irq happened, > > so it might fail to unwind from the interrupted function. > > > > With ORC, this is not a problem, as ORC has out-of-band data. We can > > find ORC data even for the IP in interrupted function and always unwind > > one level up. > > > > So introduce `unwind_regs_reliable' which decides if this is an issue > > for the currently selected unwinder at all and change the code > > accordingly. > > Thanks. I'm thinking there a few ways we can simplify things. (Most of > these are actually issues with the existing code.) > > - Currently we check to make sure that there's no frame *after* the user > space regs. I think there's no way that could ever happen and the > check is overkill. > > - We should probably remove the STACKTRACE_DUMP_ONCE() warnings. There > are some known places where a stack trace will fail, particularly with > generated code. I wish we had a DEBUG_WARN_ON() macro which used > pr_debug(), but oh well. At least the livepatch code has some helpful > pr_warn()s, those are probably good enough. ^^^^^^^ meant to say pr_debug()s. Also adding the live patching mailing list as an FYI. > > - The unwind->error checks are superfluous. The only errors we need to > check for are (a) whether the FP unwinder encountered a kernel irq and > b) whether we reached the final user regs frame. So I think > unwind->error can be removed altogether. > > So with those changes in mind, how about something like this (plus > comments)? > > for (unwind_start(&state, task, NULL, NULL); !unwind_done(&state); > unwind_next_frame(&state)) { > > regs = unwind_get_entry_regs(&state); > if (regs) { > if (user_mode(regs)) > goto success; > > if (IS_ENABLED(CONFIG_FRAME_POINTER)) > return -EINVAL; > } > > addr = unwind_get_return_address(&state); > if (!addr) > return -EINVAL; > > if (save_stack_address(trace, addr, false)) > return -EINVAL; > } > > return -EINVAL; > > success: > if (trace->nr_entries < trace->max_entries) > trace->entries[trace->nr_entries++] = ULONG_MAX; > > return 0; > > After these changes I believe we can enable > CONFIG_HAVE_RELIABLE_STACKTRACE for ORC. > > Also, when you post the next version, please cc the live patching > mailing list, since this is directly relevant to livepatch. > > Thanks! -- Josh