From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752733AbdGEKmV (ORCPT ); Wed, 5 Jul 2017 06:42:21 -0400 Received: from mail-wm0-f65.google.com ([74.125.82.65]:32771 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751626AbdGEKmU (ORCPT ); Wed, 5 Jul 2017 06:42:20 -0400 Date: Wed, 5 Jul 2017 12:42:16 +0200 From: Ingo Molnar To: "Naveen N. Rao" Cc: Masami Hiramatsu , Ananth N Mavinakayanahalli , linux-kernel@vger.kernel.org Subject: Re: [PATCH] jprobes: Ensure that the probepoint is at function entry Message-ID: <20170705104216.674ejyueqquctsn5@gmail.com> References: <20170629145655.5282-1-naveen.n.rao@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170629145655.5282-1-naveen.n.rao@linux.vnet.ibm.com> User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * 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. > > 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)) { > 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. 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. All of these cleanups should be in separate patches - with the fix in the end. Thanks, Ingo