linux-kernel - Re: [PATCH v2 06/10] ftrace: selftest: remove broken trace_direct

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CABRcYmJYUVsZyRY2Bo1DDnJogkcasi=g7TCY07vb0DELH6Hy+A@mail.gmail.com>
Date:   Thu, 16 Mar 2023 16:41:08 +0100
From:   Florent Revest <revest@...omium.org>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-trace-kernel@...r.kernel.org, bpf@...r.kernel.org,
        catalin.marinas@....com, will@...nel.org, mhiramat@...nel.org,
        mark.rutland@....com, ast@...nel.org, daniel@...earbox.net,
        andrii@...nel.org, kpsingh@...nel.org, jolsa@...nel.org,
        xukuohai@...weicloud.com, lihuafei1@...wei.com,
        Xu Kuohai <xukuohai@...wei.com>
Subject: Re: [PATCH v2 06/10] ftrace: selftest: remove broken trace_direct_tramp

On Thu, Mar 16, 2023 at 12:51 AM Steven Rostedt <rostedt@...dmis.org> wrote:
>
> On Tue,  7 Feb 2023 19:21:31 +0100
> Florent Revest <revest@...omium.org> wrote:
>
> > From: Mark Rutland <mark.rutland@....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@....com>
> > Cc: Li Huafei <lihuafei1@...wei.com>
> > Cc: Xu Kuohai <xukuohai@...wei.com>
> > Cc: Steven Rostedt (Google) <rostedt@...dmis.org>
> > Cc: Florent Revest <revest@...omium.org>
> > Signed-off-by: Florent Revest <revest@...omium.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!