From: Florent Revest <revest@chromium.org> To: Steven Rostedt <rostedt@goodmis.org> Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org, bpf@vger.kernel.org, catalin.marinas@arm.com, will@kernel.org, mhiramat@kernel.org, mark.rutland@arm.com, ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org, kpsingh@kernel.org, jolsa@kernel.org, xukuohai@huaweicloud.com, lihuafei1@huawei.com, Xu Kuohai <xukuohai@huawei.com> Subject: Re: [PATCH v2 06/10] ftrace: selftest: remove broken trace_direct_tramp Date: Thu, 16 Mar 2023 16:41:08 +0100 [thread overview] Message-ID: <CABRcYmJYUVsZyRY2Bo1DDnJogkcasi=g7TCY07vb0DELH6Hy+A@mail.gmail.com> (raw) In-Reply-To: <20230315195136.2996b1dd@gandalf.local.home> On Thu, Mar 16, 2023 at 12:51 AM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Tue, 7 Feb 2023 19:21:31 +0100 > Florent Revest <revest@chromium.org> wrote: > > > From: Mark Rutland <mark.rutland@arm.com> > > > > The ftrace selftest code has a trace_direct_tramp() function which it > > uses as a direct call trampoline. This happens to work on x86, since the > > direct call's return address is in the usual place, and can be returned > > to via a RET, but in general the calling convention for direct calls is > > different from regular function calls, and requires a trampoline written > > in assembly. > > > > On s390, regular function calls place the return address in %r14, and an > > ftrace patch-site in an instrumented function places the trampoline's > > return address (which is within the instrumented function) in %r0, > > preserving the original %r14 value in-place. As a regular C function > > will return to the address in %r14, using a C function as the trampoline > > results in the trampoline returning to the caller of the instrumented > > function, skipping the body of the instrumented function. > > > > Note that the s390 issue is not detcted by the ftrace selftest code, as > > the instrumented function is trivial, and returning back into the caller > > happens to be equivalent. > > > > On arm64, regular function calls place the return address in x30, and > > an ftrace patch-site in an instrumented function saves this into r9 > > and places the trampoline's return address (within the instrumented > > function) in x30. A regular C function will return to the address in > > x30, but will not restore x9 into x30. Consequently, using a C function > > as the trampoline results in returning to the trampoline's return > > address having corrupted x30, such that when the instrumented function > > returns, it will return back into itself. > > > > To avoid future issues in this area, remove the trace_direct_tramp() > > function, and require that each architecture with direct calls provides > > a stub trampoline, named ftrace_stub_direct_tramp. This can be written > > to handle the architecture's trampoline calling convention, and in > > future could be used elsewhere (e.g. in the ftrace ops sample, to > > measure the overhead of direct calls), so we may as well always build it > > in. > > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > Cc: Li Huafei <lihuafei1@huawei.com> > > Cc: Xu Kuohai <xukuohai@huawei.com> > > Cc: Steven Rostedt (Google) <rostedt@goodmis.org> > > Cc: Florent Revest <revest@chromium.org> > > Signed-off-by: Florent Revest <revest@chromium.org> > > > Care to respin with my update requests? I can take up to this patch and > base it directly on v6.3-rc3 when it comes out. I'm expecting that to have > the fixes in other code that is breaking my tests. Okay! :) I'll send you a subset of this series (first 6 patches with your review addressed and rebased on v6.3-rc2 for now). > Then I'll push it out after it passes all my tests, and you can take it > and add the arm64 specific bits on top. I'm currently running these patches > as is on my tests to see if they fail (with a patched kernel for the other > code that's breaking my tests). > > Does that sound OK? Sounds good to me, yes!
WARNING: multiple messages have this Message-ID (diff)
From: Florent Revest <revest@chromium.org> To: Steven Rostedt <rostedt@goodmis.org> Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org, bpf@vger.kernel.org, catalin.marinas@arm.com, will@kernel.org, mhiramat@kernel.org, mark.rutland@arm.com, ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org, kpsingh@kernel.org, jolsa@kernel.org, xukuohai@huaweicloud.com, lihuafei1@huawei.com, Xu Kuohai <xukuohai@huawei.com> Subject: Re: [PATCH v2 06/10] ftrace: selftest: remove broken trace_direct_tramp Date: Thu, 16 Mar 2023 16:41:08 +0100 [thread overview] Message-ID: <CABRcYmJYUVsZyRY2Bo1DDnJogkcasi=g7TCY07vb0DELH6Hy+A@mail.gmail.com> (raw) In-Reply-To: <20230315195136.2996b1dd@gandalf.local.home> On Thu, Mar 16, 2023 at 12:51 AM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Tue, 7 Feb 2023 19:21:31 +0100 > Florent Revest <revest@chromium.org> wrote: > > > From: Mark Rutland <mark.rutland@arm.com> > > > > The ftrace selftest code has a trace_direct_tramp() function which it > > uses as a direct call trampoline. This happens to work on x86, since the > > direct call's return address is in the usual place, and can be returned > > to via a RET, but in general the calling convention for direct calls is > > different from regular function calls, and requires a trampoline written > > in assembly. > > > > On s390, regular function calls place the return address in %r14, and an > > ftrace patch-site in an instrumented function places the trampoline's > > return address (which is within the instrumented function) in %r0, > > preserving the original %r14 value in-place. As a regular C function > > will return to the address in %r14, using a C function as the trampoline > > results in the trampoline returning to the caller of the instrumented > > function, skipping the body of the instrumented function. > > > > Note that the s390 issue is not detcted by the ftrace selftest code, as > > the instrumented function is trivial, and returning back into the caller > > happens to be equivalent. > > > > On arm64, regular function calls place the return address in x30, and > > an ftrace patch-site in an instrumented function saves this into r9 > > and places the trampoline's return address (within the instrumented > > function) in x30. A regular C function will return to the address in > > x30, but will not restore x9 into x30. Consequently, using a C function > > as the trampoline results in returning to the trampoline's return > > address having corrupted x30, such that when the instrumented function > > returns, it will return back into itself. > > > > To avoid future issues in this area, remove the trace_direct_tramp() > > function, and require that each architecture with direct calls provides > > a stub trampoline, named ftrace_stub_direct_tramp. This can be written > > to handle the architecture's trampoline calling convention, and in > > future could be used elsewhere (e.g. in the ftrace ops sample, to > > measure the overhead of direct calls), so we may as well always build it > > in. > > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > Cc: Li Huafei <lihuafei1@huawei.com> > > Cc: Xu Kuohai <xukuohai@huawei.com> > > Cc: Steven Rostedt (Google) <rostedt@goodmis.org> > > Cc: Florent Revest <revest@chromium.org> > > Signed-off-by: Florent Revest <revest@chromium.org> > > > Care to respin with my update requests? I can take up to this patch and > base it directly on v6.3-rc3 when it comes out. I'm expecting that to have > the fixes in other code that is breaking my tests. Okay! :) I'll send you a subset of this series (first 6 patches with your review addressed and rebased on v6.3-rc2 for now). > Then I'll push it out after it passes all my tests, and you can take it > and add the arm64 specific bits on top. I'm currently running these patches > as is on my tests to see if they fail (with a patched kernel for the other > code that's breaking my tests). > > Does that sound OK? Sounds good to me, yes! _______________________________________________ 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:[~2023-03-16 15:42 UTC|newest] Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-02-07 18:21 [PATCH v2 00/10] Add ftrace direct call for arm64 Florent Revest 2023-02-07 18:21 ` Florent Revest 2023-02-07 18:21 ` [PATCH v2 01/10] ftrace: Replace uses of _ftrace_direct APIs with _ftrace_direct_multi Florent Revest 2023-02-07 18:21 ` Florent Revest 2023-03-15 23:33 ` Steven Rostedt 2023-03-15 23:33 ` Steven Rostedt 2023-03-16 15:40 ` Florent Revest 2023-03-16 15:40 ` Florent Revest 2023-02-07 18:21 ` [PATCH v2 02/10] ftrace: Remove the legacy _ftrace_direct API Florent Revest 2023-02-07 18:21 ` Florent Revest 2023-02-07 18:21 ` [PATCH v2 03/10] ftrace: Rename _ftrace_direct_multi APIs to _ftrace_direct APIs Florent Revest 2023-02-07 18:21 ` Florent Revest 2023-02-07 18:21 ` [PATCH v2 04/10] ftrace: Store direct called addresses in their ops Florent Revest 2023-02-07 18:21 ` Florent Revest 2023-03-15 23:43 ` Steven Rostedt 2023-03-15 23:43 ` Steven Rostedt 2023-03-16 15:40 ` Florent Revest 2023-03-16 15:40 ` Florent Revest 2023-03-16 15:45 ` Steven Rostedt 2023-03-16 15:45 ` Steven Rostedt 2023-03-16 16:15 ` Florent Revest 2023-03-16 16:15 ` Florent Revest 2023-02-07 18:21 ` [PATCH v2 05/10] ftrace: Make DIRECT_CALLS work WITH_ARGS and !WITH_REGS Florent Revest 2023-02-07 18:21 ` Florent Revest 2023-02-07 18:21 ` [PATCH v2 06/10] ftrace: selftest: remove broken trace_direct_tramp Florent Revest 2023-02-07 18:21 ` Florent Revest 2023-03-15 23:51 ` Steven Rostedt 2023-03-15 23:51 ` Steven Rostedt 2023-03-16 15:41 ` Florent Revest [this message] 2023-03-16 15:41 ` Florent Revest 2023-02-07 18:21 ` [PATCH v2 07/10] arm64: ftrace: Add direct call support Florent Revest 2023-02-07 18:21 ` Florent Revest 2023-02-07 18:21 ` [PATCH v2 08/10] arm64: ftrace: Simplify get_ftrace_plt Florent Revest 2023-02-07 18:21 ` Florent Revest 2023-02-07 18:21 ` [PATCH v2 09/10] arm64: ftrace: Add direct call trampoline samples support Florent Revest 2023-02-07 18:21 ` Florent Revest 2023-02-07 18:21 ` [PATCH v2 10/10] selftests/bpf: Update the tests deny list on aarch64 Florent Revest 2023-02-07 18:21 ` Florent Revest
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='CABRcYmJYUVsZyRY2Bo1DDnJogkcasi=g7TCY07vb0DELH6Hy+A@mail.gmail.com' \ --to=revest@chromium.org \ --cc=andrii@kernel.org \ --cc=ast@kernel.org \ --cc=bpf@vger.kernel.org \ --cc=catalin.marinas@arm.com \ --cc=daniel@iogearbox.net \ --cc=jolsa@kernel.org \ --cc=kpsingh@kernel.org \ --cc=lihuafei1@huawei.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-trace-kernel@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=mhiramat@kernel.org \ --cc=rostedt@goodmis.org \ --cc=will@kernel.org \ --cc=xukuohai@huawei.com \ --cc=xukuohai@huaweicloud.com \ /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.