LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Masami Hiramatsu <email@example.com>
To: Masami Hiramatsu <firstname.lastname@example.org>
Cc: Jisheng Zhang <Jisheng.Zhang@synaptics.com>,
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: Tue, 21 Jul 2020 22:24:55 +0900 [thread overview]
Message-ID: <firstname.lastname@example.org> (raw)
Would you be still working on this series?
If you are still want to put a probe on func+4, it is OK if you can
completely emulate the 1st instruction. (lr save on the stack and
change the regs->sp)
On Thu, 26 Dec 2019 18:26:07 +0900
Masami Hiramatsu <email@example.com> wrote:
> 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
> patching correctly)
> 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)
> Thank you,
> > 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
> > Thanks
> Masami Hiramatsu <firstname.lastname@example.org>
Masami Hiramatsu <email@example.com>
next prev parent reply other threads:[~2020-07-21 13:25 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
2020-07-21 13:24 ` Masami Hiramatsu [this message]
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).