From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752114AbdGFKDc (ORCPT ); Thu, 6 Jul 2017 06:03:32 -0400 Received: from mail.kernel.org ([198.145.29.99]:57692 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751868AbdGFKDb (ORCPT ); Thu, 6 Jul 2017 06:03:31 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org F411C22B52 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: Thu, 6 Jul 2017 19:03:23 +0900 From: Masami Hiramatsu To: Ingo Molnar Cc: "Naveen N. Rao" , Masami Hiramatsu , Ananth N Mavinakayanahalli , linux-kernel@vger.kernel.org Subject: Re: [PATCH] jprobes: Ensure that the probepoint is at function entry Message-Id: <20170706190323.8892576e53af1be29f6cf8f4@kernel.org> In-Reply-To: <20170705104216.674ejyueqquctsn5@gmail.com> References: <20170629145655.5282-1-naveen.n.rao@linux.vnet.ibm.com> <20170705104216.674ejyueqquctsn5@gmail.com> X-Mailer: Sylpheed 3.5.0 (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 On Wed, 5 Jul 2017 12:42:16 +0200 Ingo Molnar wrote: > > * Naveen N. Rao wrote: > > > Similar to commit 90ec5e89e393c ("kretprobes: Ensure probe location is > > at function entry"), ensure that the jprobe probepoint is at function > > entry. Sorry I missed it. > > > > Signed-off-by: Naveen N. Rao > > --- > > kernel/kprobes.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c > > index adfe3b4cfe05..950018609339 100644 > > --- a/kernel/kprobes.c > > +++ b/kernel/kprobes.c > > @@ -1776,9 +1776,14 @@ int register_jprobes(struct jprobe **jps, int num) > > jp = jps[i]; > > addr = arch_deref_entry_point(jp->entry); > > > > - /* Verify probepoint is a function entry point */ > > + /* > > + * Verify probepoint as well as the jprobe handler are > > + * function entry points. > > + */ > > if (kallsyms_lookup_size_offset(addr, NULL, &offset) && > > - offset == 0) { > > + offset == 0 && > > + function_offset_within_entry(jp->kp.addr, > > + jp->kp.symbol_name, jp->kp.offset)) { Here, you are agressively use tab, please align the indent to same level of kallsyms_lookup_size_offset? > > jp->kp.pre_handler = setjmp_pre_handler; > > jp->kp.break_handler = longjmp_break_handler; > > ret = register_kprobe(&jp->kp); > > Yeah, so I agree with the fix, but the line breaks there are disgusting. > > One solution would be to split out the iterator into a register_jprobe() function. > Also, introduce a 'kp' temporary variable to simplify and shorten usage. > Agreed. > Also, 'function_offset_within_entry' is way too long a name, and it's also a > minomer I think. The purpose of this function is to enforce that the relative > 'offset' of a new probe is at the standard function entry offset: i.e. 0 on most > architectures, and some ABI dependent constant on PowerPC, right? > > That's not at all clear from that name, plus it's a global namespace symbol, yet > has no 'kprobes' prefix. So it should be named something like > 'kprobe_offset_valid()' or such, with an arch_kprobe_offset_valid() counterpart. Hmm, I would rather like kprobe_within_entry(), since offset != 0 is actually valid for normal kprobe, that is kretprobe and jprobe limitation. Thank you, > > All of these cleanups should be in separate patches - with the fix in the end. > > Thanks, > > Ingo -- Masami Hiramatsu