From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751418AbdKUOr3 (ORCPT ); Tue, 21 Nov 2017 09:47:29 -0500 Received: from mail.kernel.org ([198.145.29.99]:35254 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751221AbdKUOr1 (ORCPT ); Tue, 21 Nov 2017 09:47:27 -0500 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org ABEC82192B Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=jeyu@kernel.org Date: Tue, 21 Nov 2017 15:47:18 +0100 From: Jessica Yu To: Masami Hiramatsu Cc: Steven Rostedt , Ananth N Mavinakayanahalli , Anil S Keshavamurthy , "David S . Miller" , Ingo Molnar , Petr Mladek , Josh Poimboeuf , Joe Lawrence , Jiri Kosina , Miroslav Benes , live-patching@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: kprobes: propagate error from arm_kprobe_ftrace() Message-ID: <20171121144717.flsludxvjeplekx6@redbean> References: <20171102163334.3947-1-jeyu@kernel.org> <20171102163334.3947-2-jeyu@kernel.org> <20171103100317.01db273b@vmware.local.home> <20171107171455.iegqpye6wpvhztd5@redbean> <20171109093550.30f9bc1f00d84db0f453e2dc@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <20171109093550.30f9bc1f00d84db0f453e2dc@kernel.org> X-OS: Linux redbean 4.13.10-200.fc26.x86_64 x86_64 User-Agent: NeoMutt/20171027 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org +++ Masami Hiramatsu [09/11/17 09:35 +0900]: >On Tue, 7 Nov 2017 18:14:56 +0100 >Jessica Yu wrote: > >> +++ Steven Rostedt [03/11/17 10:03 -0400]: >> >On Thu, 2 Nov 2017 17:33:33 +0100 >> >Jessica Yu wrote: >> > >> >> Improve error handling when arming ftrace-based kprobes. Specifically, if >> >> we fail to arm a ftrace-based kprobe, register_kprobe()/enable_kprobe() >> >> should report an error instead of success. Previously, this has lead to >> >> confusing situations where register_kprobe() would return 0 indicating >> >> success, but the kprobe would not be functional if ftrace registration >> >> during the kprobe arming process had failed. We should therefore take any >> >> errors returned by ftrace into account and propagate this error so that we >> >> do not register/enable kprobes that cannot be armed. This can happen if, >> >> for example, register_ftrace_function() finds an IPMODIFY conflict (since >> >> kprobe_ftrace_ops has this flag set) and returns an error. Such a conflict >> >> is possible since livepatches also set the IPMODIFY flag for their ftrace_ops. >> >> >> >> arm_all_kprobes() keeps its current behavior and attempts to arm all >> >> kprobes. It returns the last encountered error and gives a warning if >> >> not all kprobes could be armed. >> >> >> >> This patch is 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. >> >> >> >> Based-on-patches-by: Petr Mladek >> >> Signed-off-by: Jessica Yu >> >> --- >> >> kernel/kprobes.c | 88 ++++++++++++++++++++++++++++++++++++++++---------------- >> >> 1 file changed, 63 insertions(+), 25 deletions(-) >> >> >> >> diff --git a/kernel/kprobes.c b/kernel/kprobes.c >> >> index da2ccf142358..f4a094007cb5 100644 >> >> --- a/kernel/kprobes.c >> >> +++ b/kernel/kprobes.c >> >> @@ -978,18 +978,27 @@ static int prepare_kprobe(struct kprobe *p) >> >> } >> >> >> >> /* Caller must lock kprobe_mutex */ >> >> -static void arm_kprobe_ftrace(struct kprobe *p) >> >> +static int arm_kprobe_ftrace(struct kprobe *p) >> >> { >> >> - int ret; >> >> + int ret = 0; >> >> >> >> ret = ftrace_set_filter_ip(&kprobe_ftrace_ops, >> >> (unsigned long)p->addr, 0, 0); >> >> - WARN(ret < 0, "Failed to arm kprobe-ftrace at %p (%d)\n", p->addr, ret); >> >> - kprobe_ftrace_enabled++; >> >> - if (kprobe_ftrace_enabled == 1) { >> >> + if (WARN(ret < 0, "Failed to arm kprobe-ftrace at %p (%d)\n", p->addr, ret)) >> >> + return ret; >> >> + >> >> + if (kprobe_ftrace_enabled == 0) { >> >> ret = register_ftrace_function(&kprobe_ftrace_ops); >> >> - WARN(ret < 0, "Failed to init kprobe-ftrace (%d)\n", ret); >> >> + if (WARN(ret < 0, "Failed to init kprobe-ftrace (%d)\n", ret)) >> >> + goto err_ftrace; >> >> } >> >> + >> >> + kprobe_ftrace_enabled++; >> >> + return ret; >> >> + >> >> +err_ftrace: >> >> + ftrace_set_filter_ip(&kprobe_ftrace_ops, (unsigned long)p->addr, 1, 0); >> > >> >Hmm, this could have a very nasty side effect. If you remove a function >> >from the ops, and it was the last function, an empty ops means to trace >> >*all* functions. >> >> Good point, and yes, normally this would be the (undesirable) outcome. >> >> However in this case, kprobes_ftrace_ops has the IPMODIFY flag set, so >> ftrace_set_filter_ip() will not allow the removal of a function if it >> is the very last function, and it will return an error in >> __ftrace_hash_update_ipmodify(). The comment there explains, if >> IPMODIFY is set, "return -EINVAL if the new_hash tries to trace all >> recs". So I think we are safe here... >> >> >Perhaps you want to add it to the "notrace" list. Which would require >> >implementing a ftrace_set_notrace_ip() function. Which I believe is >> >what you want. Any function in the notrace hash will have the same >> >functions in the filter hash be ignored. >> >> I think this would've been a good alternative if we wanted to protect >> against the empty ops case, but IPMODIFY is also incompatible with notrace.. >> (See: commit f8b8be8a310 and this comment here: >> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg688632.html) >> >> Speaking of IPMODIFY, a question for Masami - is this flag still >> relevant/needed for kprobes, since jprobes has been deprecated >> recently? IIRC, IPMODIFY was needed in the first place because jprobes >> and livepatch were in direct conflict, but I recall some work being >> done in the past to remove the IPMODIFY flag from kprobes, but I don't >> think this was ever upstreamed. (See: https://patchwork.kernel.org/patch/5352481/) > >Hmm, good point. I just want to make kprobes transparently using ftrace. >This means if someone writes a kernel module which uses kprobes to >change IP address, which should work with/without CONFIG_FTRACE enabled. >kprobes itself supports to modify regs->ip (under some special settings, >see Documentation/kprobes.txt:Note for geeks), so we can not remove it. OK, makes sense. If kprobes keeps the ftrace IPMODIFY flag, then I believe we are safe from the empty ops case Steven pointed out before, since ftrace ops with IPMODIFY set require at least one entry in the filter hash (an error is returned if an attempt is made to remove the last function from the ops). I can add a comment noting this in the error path since it doesn't look like this is explicitly documented. Thanks for the comments, will send v3 shortly after the merge window. Jessica