Messages in this thread | | | Date | Thu, 6 Jul 2017 19:03:23 +0900 | From | Masami Hiramatsu <> | Subject | Re: [PATCH] jprobes: Ensure that the probepoint is at function entry |
| |
On Wed, 5 Jul 2017 12:42:16 +0200 Ingo Molnar <mingo@kernel.org> wrote:
> > * Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> 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 <naveen.n.rao@linux.vnet.ibm.com> > > --- > > 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 <mhiramat@kernel.org>
| |