From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753243AbbCSQ5R (ORCPT ); Thu, 19 Mar 2015 12:57:17 -0400 Received: from smtprelay0218.hostedemail.com ([216.40.44.218]:59121 "EHLO smtprelay.hostedemail.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752676AbbCSQ5L (ORCPT ); Thu, 19 Mar 2015 12:57:11 -0400 X-Session-Marker: 726F737465647440676F6F646D69732E6F7267 X-Spam-Summary: 40,2.5,0,,d41d8cd98f00b204,rostedt@goodmis.org,:::::::::::::::::,RULES_HIT:41:355:379:541:599:800:960:968:973:988:989:1260:1277:1311:1313:1314:1345:1359:1437:1515:1516:1518:1534:1543:1593:1594:1605:1711:1730:1747:1777:1792:2194:2199:2393:2553:2559:2562:2693:2899:2918:3138:3139:3140:3141:3142:3421:3622:3865:3866:3867:3868:3870:3871:3872:3874:4184:4250:4321:4361:4362:5007:6117:6119:6261:7875:7903:8957:9010:9108:10011:10400:10848:10967:11026:11232:11473:11658:11914:12043:12291:12295:12296:12438:12517:12519:12555:12740:13161:13229:13972:21060:21080,0,RBL:none,CacheIP:none,Bayesian:0.5,0.5,0.5,Netcheck:none,DomainCache:0,MSF:not bulk,SPF:fn,MSBL:0,DNSBL:none,Custom_rules:0:1:0 X-HE-Tag: move73_206085d491946 X-Filterd-Recvd-Size: 4656 Date: Thu, 19 Mar 2015 12:57:01 -0400 From: Steven Rostedt To: Petr Mladek Cc: Masami Hiramatsu , "David S. Miller" , Anil S Keshavamurthy , Ananth NMavinakayanahalli , Frederic Weisbecker , Ingo Molnar , Jiri Kosina , linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] kprobes: Disable Kprobe when ftrace arming fails Message-ID: <20150319125701.4202ee77@gandalf.local.home> In-Reply-To: <1426519667-7321-1-git-send-email-pmladek@suse.cz> References: <1426519667-7321-1-git-send-email-pmladek@suse.cz> X-Mailer: Claws Mail 3.11.1 (GTK+ 2.24.25; x86_64-pc-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 On Mon, 16 Mar 2015 16:27:47 +0100 Petr Mladek wrote: > arm_kprobe_ftrace() could fail, especially after introducing ftrace IPMODIFY > flag and LifePatching. But this situation is not properly handled. > This patch adds the most important changes. > > First, it does not make sense to register "kprobe_ftrace_ops" if the filter was > not set. > > Second, we should remove the filter if the registration of "kprobe_ftrace_ops" > fails. The failure might be caused by conflict between the Kprobe and > a life patch via the IPMODIFY flag. If we remove the filter, we will allow > to register "kprobe_ftrace_ops" for another non-conflicting Kprobe later. > > Third, we need to make sure that "kprobe_ftrace_enabled" is incremented only > when "kprobe_ftrace_ops" is successfully registered. Otherwise, another > Kprobe will not try to register it again. Note that we could move the > manipulation with this counter because it is accessed only under "kprobe_mutex". > > Four, we should mark the probe as disabled if the ftrace stuff is not usable. > It will be the correct status. Also it will prevent the unregistration code > from producing another failure. > > It looks more safe to disable the Kprobe directly in "kprobe_ftrace_ops". Note > that we need to disable also all listed Kprobes in case of an aggregated probe. > It would be enough to disable only the new one but we do not know which one it > was. They should be in sync anyway. > I can pull this into my tree. But there's one little nit below. > Signed-off-by: Petr Mladek > Acked-by: Masami Hiramatsu > --- > Hi, > > I resend this patch separately and have just added the acked by Masami. > > Best Regards, > Petr > > kernel/kprobes.c | 25 +++++++++++++++++++++---- > 1 file changed, 21 insertions(+), 4 deletions(-) > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c > index c90e417bb963..54b22db084f3 100644 > --- a/kernel/kprobes.c > +++ b/kernel/kprobes.c > @@ -932,16 +932,33 @@ static int prepare_kprobe(struct kprobe *p) > /* Caller must lock kprobe_mutex */ > static void arm_kprobe_ftrace(struct kprobe *p) > { > + struct kprobe *kp; > int ret; > > 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). The kprobe gets disabled.\n", > + p->addr, ret)) > + goto err_filter; > + > + if (!kprobe_ftrace_enabled) { > 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). The probe at %p gets disabled\n", > + ret, p->addr)) > + goto err_function; > } > + kprobe_ftrace_enabled++; > + return; > + > +err_function: > + ftrace_set_filter_ip(&kprobe_ftrace_ops, (unsigned long)p->addr, 1, 0); > +err_filter: > + p->flags |= KPROBE_FLAG_DISABLED; > + if (kprobe_aggrprobe(p)) > + list_for_each_entry_rcu(kp, &p->list, list) > + kp->flags |= KPROBE_FLAG_DISABLED; The if statement should have brackets. No bracket if and for statements should only be used when there's a singe simple non complex line below. This is not the case. Thanks, -- Steve > } > > /* Caller must lock kprobe_mutex */