From: Steven Rostedt <rostedt@goodmis.org>
To: Peter Zijlstra <peterz@infradead.org>
Cc: linux-kernel@vger.kernel.org, Ingo Molnar <mingo@kernel.org>,
Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [for-next][PATCH 04/18] x86/ftrace: Do not jump to direct code in created trampolines
Date: Mon, 13 Jul 2020 23:24:59 -0400 [thread overview]
Message-ID: <20200713232459.039683cc@oasis.local.home> (raw)
In-Reply-To: <20200703081000.GT4800@hirez.programming.kicks-ass.net>
On Fri, 3 Jul 2020 10:10:00 +0200
Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, Jul 02, 2020 at 05:58:16PM -0400, Steven Rostedt wrote:
>
> > + /* No need to test direct calls on created trampolines */
> > + if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {
> > + /* NOP the jnz 1f; but make sure it's a 2 byte jnz */
> > + ip = trampoline + (jmp_offset - start_offset);
> > + if (WARN_ON(*(char *)ip != 0x75))
> > + goto fail;
> > + ret = copy_from_kernel_nofault(ip, ideal_nops[2], 2);
>
> I really don't get this paranoia, what's wrong with memcpy() ?
Habit. As when ftrace was introduced, it was extremely careful about
touching memory like this. And even with all of that extra care, we
still broke NICs (actually, some of the reason those NICs broke was
because of the extra care we took :-p)
>
> > + if (ret < 0)
> > + goto fail;
> > + }
>
> How about something like this?
>
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -359,17 +359,11 @@ create_trampoline(struct ftrace_ops *ops
> npages = DIV_ROUND_UP(*tramp_size, PAGE_SIZE);
>
> /* Copy ftrace_caller onto the trampoline memory */
> - ret = copy_from_kernel_nofault(trampoline, (void *)start_offset, size);
> - if (WARN_ON(ret < 0))
> - goto fail;
> -
> - ip = trampoline + size;
> + memcpy(trampoline, (void *)start_offset, size);
>
> /* The trampoline ends with ret(q) */
> - retq = (unsigned long)ftrace_stub;
> - ret = copy_from_kernel_nofault(ip, (void *)retq, RET_SIZE);
> - if (WARN_ON(ret < 0))
> - goto fail;
> + ip = trampoline + size;
> + memcpy(ip, text_gen_insn(RET_INSN_OPCODE, NULL, NULL), RET_INSN_SIZE);
>
> /* No need to test direct calls on created trampolines */
> if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {
> @@ -377,9 +371,7 @@ create_trampoline(struct ftrace_ops *ops
> ip = trampoline + (jmp_offset - start_offset);
> if (WARN_ON(*(char *)ip != 0x75))
> goto fail;
> - ret = copy_from_kernel_nofault(ip, ideal_nops[2], 2);
> - if (ret < 0)
> - goto fail;
> + memcpy(ip, ideal_nops[2], 2);
If you want to add this change on top of this, then I'm fine with that.
If it breaks something, I can at least point the blame at you ;-)
-- Steve
> }
>
> /*
next prev parent reply other threads:[~2020-07-14 3:25 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-02 21:58 [for-next][PATCH 00/18] tracing: Updatse for 5.9 Steven Rostedt
2020-07-02 21:58 ` [for-next][PATCH 01/18] tracing: Only allow trace_array_printk() to be used by instances Steven Rostedt
2020-07-02 21:58 ` [for-next][PATCH 02/18] x86/ftrace: Make non direct case the default in ftrace_regs_caller Steven Rostedt
2020-07-02 21:58 ` [for-next][PATCH 03/18] x86/ftrace: Only have the builtin ftrace_regs_caller call direct hooks Steven Rostedt
2020-07-02 21:58 ` [for-next][PATCH 04/18] x86/ftrace: Do not jump to direct code in created trampolines Steven Rostedt
2020-07-03 8:10 ` Peter Zijlstra
2020-07-14 3:24 ` Steven Rostedt [this message]
2020-07-02 21:58 ` [for-next][PATCH 05/18] tracing: not necessary to undefine DEFINE_EVENT again Steven Rostedt
2020-07-02 21:58 ` [for-next][PATCH 06/18] tracing: not necessary re-define DEFINE_EVENT_PRINT Steven Rostedt
2020-07-02 21:58 ` [for-next][PATCH 07/18] tracing: define DEFINE_EVENT_PRINT not related to DEFINE_EVENT Steven Rostedt
2020-07-02 21:58 ` [for-next][PATCH 08/18] tracing: not necessary to define DEFINE_EVENT_PRINT to be empty again Steven Rostedt
2020-07-02 21:58 ` [for-next][PATCH 09/18] tracing: Move pipe reference to trace array instead of current_tracer Steven Rostedt
2020-07-31 19:33 ` dann frazier
2020-07-31 21:16 ` Steven Rostedt
2020-07-31 21:17 ` dann frazier
2020-07-02 21:58 ` [for-next][PATCH 10/18] ring-buffer: Have nested events still record running time stamp Steven Rostedt
2020-07-02 21:58 ` [for-next][PATCH 11/18] ring-buffer: Incorporate absolute timestamp into add_timestamp logic Steven Rostedt
2020-07-02 21:58 ` [for-next][PATCH 12/18] ring-buffer: Add rb_time_t 64 bit operations for speeding up 32 bit Steven Rostedt
2020-07-02 21:58 ` [for-next][PATCH 13/18] ring-buffer: speed up buffer resets by avoiding synchronize_rcu for each CPU Steven Rostedt
2020-07-02 21:58 ` [for-next][PATCH 14/18] ring-buffer: Mark the !tail (crossing a page) as unlikely Steven Rostedt
2020-07-02 21:58 ` [for-next][PATCH 15/18] ring-buffer: Consolidate add_timestamp to remove some branches Steven Rostedt
2020-07-02 21:58 ` [for-next][PATCH 16/18] ring-buffer: Move the add_timestamp into its own function Steven Rostedt
2020-07-02 21:58 ` [for-next][PATCH 17/18] ring-buffer: Call trace_clock_local() directly for RETPOLINE kernels Steven Rostedt
2020-07-02 21:58 ` [for-next][PATCH 18/18] ring-buffer: Do not trigger a WARN if clock going backwards is detected Steven Rostedt
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=20200713232459.039683cc@oasis.local.home \
--to=rostedt@goodmis.org \
--cc=akpm@linux-foundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=peterz@infradead.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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox