From: Masami Hiramatsu <mhiramat@kernel.org> To: Jisheng Zhang <Jisheng.Zhang@synaptics.com> Cc: Mark Rutland <mark.rutland@arm.com>, Jonathan Corbet <corbet@lwn.net>, Catalin Marinas <catalin.marinas@arm.com>, "linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>, Ingo Molnar <mingo@redhat.com>, Steven Rostedt <rostedt@goodmis.org>, "Naveen N. Rao" <naveen.n.rao@linux.ibm.com>, Will Deacon <will@kernel.org>, "David S. Miller" <davem@davemloft.net>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org> Subject: Re: [PATCH v7 3/3] arm64: implement KPROBES_ON_FTRACE Date: Thu, 26 Dec 2019 18:26:07 +0900 [thread overview] Message-ID: <20191226182607.06770598a00507090a046951@kernel.org> (raw) In-Reply-To: <20191226121108.0cd1b078@xhacker.debian> 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 effects. 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 <mhiramat@kernel.org>
WARNING: multiple messages have this Message-ID (diff)
From: Masami Hiramatsu <mhiramat@kernel.org> To: Jisheng Zhang <Jisheng.Zhang@synaptics.com> Cc: Mark Rutland <mark.rutland@arm.com>, "linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>, Catalin Marinas <catalin.marinas@arm.com>, Jonathan Corbet <corbet@lwn.net>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>, Ingo Molnar <mingo@redhat.com>, Steven Rostedt <rostedt@goodmis.org>, "Naveen N. Rao" <naveen.n.rao@linux.ibm.com>, Will Deacon <will@kernel.org>, "David S. Miller" <davem@davemloft.net>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org> Subject: Re: [PATCH v7 3/3] arm64: implement KPROBES_ON_FTRACE Date: Thu, 26 Dec 2019 18:26:07 +0900 [thread overview] Message-ID: <20191226182607.06770598a00507090a046951@kernel.org> (raw) In-Reply-To: <20191226121108.0cd1b078@xhacker.debian> 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 effects. 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 <mhiramat@kernel.org> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-12-26 9:26 UTC|newest] Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-12-25 9:40 [PATCH v7 0/3] arm64: implement KPROBES_ON_FTRACE Jisheng Zhang 2019-12-25 9:40 ` 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:42 ` Jisheng Zhang 2019-12-25 9:46 ` 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-25 9:42 ` Jisheng Zhang 2019-12-26 2:45 ` Masami Hiramatsu 2019-12-26 2:45 ` Masami Hiramatsu 2020-01-08 0:05 ` Steven Rostedt 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-25 9:44 ` Jisheng Zhang 2019-12-26 2:57 ` Masami Hiramatsu 2019-12-26 2:57 ` Masami Hiramatsu 2019-12-26 3:18 ` Jisheng Zhang 2019-12-26 3:18 ` Jisheng Zhang 2019-12-26 4:25 ` Jisheng Zhang 2019-12-26 4:25 ` Jisheng Zhang 2019-12-26 9:26 ` Masami Hiramatsu [this message] 2019-12-26 9:26 ` Masami Hiramatsu 2020-07-21 13:24 ` Masami Hiramatsu 2020-07-21 13:24 ` Masami Hiramatsu 2020-07-24 7:06 ` Jisheng Zhang 2020-07-24 7:06 ` Jisheng Zhang 2020-07-24 16:54 ` Masami Hiramatsu 2020-07-24 16:54 ` Masami Hiramatsu 2020-02-28 15:31 ` Mark Rutland 2020-02-28 15:31 ` Mark Rutland
Reply instructions: 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: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20191226182607.06770598a00507090a046951@kernel.org \ --to=mhiramat@kernel.org \ --cc=Jisheng.Zhang@synaptics.com \ --cc=anil.s.keshavamurthy@intel.com \ --cc=catalin.marinas@arm.com \ --cc=corbet@lwn.net \ --cc=davem@davemloft.net \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-doc@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=mingo@redhat.com \ --cc=naveen.n.rao@linux.ibm.com \ --cc=rostedt@goodmis.org \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.