LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Masami Hiramatsu <firstname.lastname@example.org>
To: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
Cc: Mark Rutland <email@example.com>,
Jonathan Corbet <firstname.lastname@example.org>,
Catalin Marinas <email@example.com>,
Anil S Keshavamurthy <firstname.lastname@example.org>,
Ingo Molnar <email@example.com>,
Steven Rostedt <firstname.lastname@example.org>,
"Naveen N. Rao" <email@example.com>,
Will Deacon <firstname.lastname@example.org>,
"David S. Miller" <email@example.com>,
Subject: Re: [PATCH v7 3/3] arm64: implement KPROBES_ON_FTRACE
Date: Thu, 26 Dec 2019 18:26:07 +0900 [thread overview]
Message-ID: <firstname.lastname@example.org> (raw)
On Thu, 26 Dec 2019 04:25:24 +0000
Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote:
> > > > +/*
> > > > + * In arm64 FTRACE_WITH_REGS implementation, we patch two nop instructions:
> > > > + * the lr saver and bl ftrace-entry. Both these instructions are claimed
> > > > + * by ftrace and we should allow probing on either instruction.
> > >
> > > No, the 2nd bl ftrace-entry must not be probed.
> > > The pair of lr-saver and bl ftrace-entry is tightly coupled. You can not
> > > decouple it.
> > This is the key. different viewing of this results in different implementation.
> > I'm just wondering why are the two instructions considered as coupled. I think
> > here we met similar situation as powerpc: https://lkml.org/lkml/2019/6/18/646
> > the "mflr r0" equals to lr-saver here, branch to _mcount equals to bl ftrace-entry
> > could you please kindly comment more?
> > Thanks in advance
> hmm, I think I may get some part of your opinion. In v7 implementation:
> if probe on func+4, that's bl ftrace-entry, similar as mcount call on
> other architectures, we allow this probe as normal.
> if probe on func+0, the first param ip in kprobe_ftrace_handler() points
> to func+4(this is adjusted by ftrace), regs->ip points to func+8, so in
> kprobe_ftrace_handler() we modify regs->ip to func+0 to call kprobe
> pre handler, then modify regs->ip to func+8 to call kprobe post handler.
> As can be seen, the first two instructions are considered as a virtual
> mcount call. From this point of view, lr saver and the bl <ftrace-entry>
> is coupled.
Yes, this is good. But probing on func+4 is meaningless. Both func+0 and
func+4 call a handler with same pt_regs. And it should have the stack
pointer which is NOT modified by lr-saver and regs->lr must point original
call address. (ftrace regs caller must do this fixup for supporting live
And in this case, func+4 has fake pt_regs because it skips lr-saver's
And even if you fixed up the pt_regs, there is another problem of what
user expects on the target instructions.
As you know, dynamic ftrace will fill the instruction with NOP (2 NOPs
in arm64), in this case, maybe pt_regs are same except pc on func+0 and
func+4. But if ftrace already enabled on the function, user will see
there are lr-saver and bl, oops. In this case we have to change pt_regs
between func+0 and func+4. So it depends on the current mode.
However, IMHO, it is not worth to pay such simulation cost. No one want
to probe such simulated intermediate address. It is easy to expect the
result from the code. Moreover, the func+4 will not appear on debuginfo
because those 2 special insturctions are just appended by the compiler,
not generated by the code.
So I don't think we need to support func+4. We only need func+0, or func+8
(this must be same as func+0 except regs->pc anyway)
> If we split patch3 into two:
> one to support kprobes func+4
> the second to support kprobe on func+0
> it would be much clearer.
> Then the key here is whether we could allow both kprobes on func+0 and func+4
Masami Hiramatsu <email@example.com>
next prev parent reply other threads:[~2019-12-26 9:26 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-25 9:40 [PATCH v7 0/3] " Jisheng Zhang
2019-12-25 9:42 ` [PATCH v7 1/3] kprobes/ftrace: Use ftrace_location() when [dis]arming probes Jisheng Zhang
2019-12-25 9:46 ` Jisheng Zhang
2019-12-25 9:42 ` [PATCH v7 2/3] ftrace: introduce FTRACE_IP_EXTENSION Jisheng Zhang
2019-12-26 2:45 ` Masami Hiramatsu
2020-01-08 0:05 ` Steven Rostedt
2019-12-25 9:44 ` [PATCH v7 3/3] arm64: implement KPROBES_ON_FTRACE Jisheng Zhang
2019-12-26 2:57 ` Masami Hiramatsu
2019-12-26 3:18 ` Jisheng Zhang
2019-12-26 4:25 ` Jisheng Zhang
2019-12-26 9:26 ` Masami Hiramatsu [this message]
2020-07-21 13:24 ` Masami Hiramatsu
2020-07-24 7:06 ` Jisheng Zhang
2020-07-24 16:54 ` Masami Hiramatsu
2020-02-28 15:31 ` Mark Rutland
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--subject='Re: [PATCH v7 3/3] arm64: implement KPROBES_ON_FTRACE' \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).