From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753799AbeASHGT (ORCPT ); Fri, 19 Jan 2018 02:06:19 -0500 Received: from mail.kernel.org ([198.145.29.99]:57850 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750757AbeASHGM (ORCPT ); Fri, 19 Jan 2018 02:06:12 -0500 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5234E21717 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=mhiramat@kernel.org Date: Fri, 19 Jan 2018 16:06:07 +0900 From: Masami Hiramatsu To: Jessica Yu , Ingo Molnar Cc: mhiramat@kernel.org, Ananth N Mavinakayanahalli , Anil S Keshavamurthy , "David S . Miller" , Ingo Molnar , Petr Mladek , Josh Poimboeuf , Joe Lawrence , Jiri Kosina , Miroslav Benes , Steven Rostedt , live-patching@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v5 0/2] kprobes: improve error handling when arming/disarming kprobes Message-Id: <20180119160607.5ed19168be13d27cf7f94531@kernel.org> In-Reply-To: <20180109235124.30886-1-jeyu@kernel.org> References: <20180109235124.30886-1-jeyu@kernel.org> X-Mailer: Sylpheed 3.5.1 (GTK+ 2.24.31; x86_64-redhat-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Ingo, Could you pick this to tip tree? Thank you, On Wed, 10 Jan 2018 00:51:22 +0100 Jessica Yu wrote: > Hi, > > This patchset attempts to improve error handling when arming or disarming > ftrace-based kprobes. The current behavior is to simply WARN when ftrace > (un-)registration fails, without propagating the error code. This can lead > to confusing situations where, for example, register_kprobe()/enable_kprobe() > would return 0 indicating success even if arming via ftrace had failed. In > this scenario we'd end up with a non-functioning kprobe even though kprobe > registration (or enablement) returned success. In this patchset, we take > errors from ftrace into account and propagate the error when we cannot arm > or disarm a kprobe. > > Below is an example that illustrates the problem using livepatch and > systemtap (which uses kprobes underneath). Both livepatch and kprobes use > ftrace ops with the IPMODIFY flag set, so registration at the same > function entry is limited to only one ftrace user. > > Before > ------ > # modprobe livepatch-sample # patches cmdline_proc_show, ftrace ops has IPMODIFY set > # stap -e 'probe kernel.function("cmdline_proc_show").call { printf ("cmdline_proc_show\n"); }' > > .. (nothing prints after reading /proc/cmdline) .. > > The systemtap handler doesn't execute due to a kprobe arming failure caused > by a ftrace IPMODIFY conflict with livepatch, and there isn't an obvious > indication of error from systemtap (because register_kprobe() returned > success) unless the user inspects dmesg. > > After > ----- > # modprobe livepatch-sample > # stap -e 'probe kernel.function("cmdline_proc_show").call { printf ("cmdline_proc_show\n"); }' > WARNING: probe kernel.function("cmdline_proc_show@/home/jeyu/work/linux-next/fs/proc/cmdline.c:6").call (address 0xffffffffa82fe910) registration error (rc -16) > > Although the systemtap handler doesn't execute (as it shouldn't), the > ftrace error is propagated and now systemtap prints a visible error message > stating that (kprobe) registration had failed (because register_kprobe() > returned an error), along with the propagated error code. > > This patchset was based on Petr Mladek's original patchset (patches 2 and 3) > back in 2015, which improved kprobes error handling, found here: > > https://lkml.org/lkml/2015/2/26/452 > > However, further work on this had been paused since then and the patches > were not upstreamed. > > This patchset has been lightly sanity-tested (on linux-next) with kprobes, > kretprobes, and optimized kprobes. It passes the kprobes smoke test, but > more testing is greatly appreciated. > > Changes from v4: > - Switch from WARN() to pr_debug() in arm_kprobe_ftrace() so the stack > dumps don't pollute dmesg, as IPMODIFY conflicts can occur in normal usage > - Added Masami's ack to the first patch > > Changes from v3: > - Have (dis)arm_kprobe_ftrace() return -ENODEV instead of 0 in case of > !CONFIG_KPROBES_ON_FTRACE > - Add total count of all probes tried in (dis)arm_all_kprobes() > > Changes from v2: > - Add missing synchronize rcu in register_aggr_kprobe() > - s/kprobes/probes/ on error message in (dis)arm_all_kprobes() > > Changes from v1: > - Don't arm the kprobe before adding it to the kprobe table, otherwise > we'll temporarily see a stray breakpoint. > - Remove kprobe from the kprobe_table and call synchronize_sched() if > arming during register_kprobe() fails. > - add Masami's ack on the 2nd patch (unchanged from v1) > > --- > Jessica Yu (2): > kprobes: propagate error from arm_kprobe_ftrace() > kprobes: propagate error from disarm_kprobe_ftrace() > > kernel/kprobes.c | 178 +++++++++++++++++++++++++++++++++++++++---------------- > 1 file changed, 128 insertions(+), 50 deletions(-) > > -- > 2.13.6 > -- Masami Hiramatsu