linux-kernel - Re: [PATCH 4/5] ftrace/x86: Have arch x86_64 use breakpoints instead of stop machine

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]

Date:	Thu, 26 Apr 2012 19:43:46 +0900
From:	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...e.hu>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Frederic Weisbecker <fweisbec@...il.com>,
	"H. Peter Anvin" <hpa@...or.com>, yrl.pp-manager.tt@...achi.com
Subject: Re: [PATCH 4/5] ftrace/x86: Have arch x86_64 use breakpoints instead
 of stop machine

(2012/04/26 3:48), Steven Rostedt wrote:
> From: Steven Rostedt <srostedt@...hat.com>
> 
> This method changes x86 to add a breakpoint to the mcount locations
> instead of calling stop machine.
> 
> Now that iret can be handled by NMIs, we perform the following to
> update code:
> 
> 1) Add a breakpoint to all locations that will be modified
> 
> 2) Sync all cores
> 
> 3) Update all locations to be either a nop or call (except breakpoint
>    op)
> 
> 4) Sync all cores
> 
> 5) Remove the breakpoint with the new code.
> 
> 6) Sync all cores
> 
> [
>   Added updates that Masami suggested:
>    Use unlikely(modifying_ftrace_code) in int3 trap to keep kprobes efficient.
>    Don't use NOTIFY_* in ftrace handler in int3 as it is not a notifier.
> ]

I just have a few comments :)

[...]
> +int modifying_ftrace_code __read_mostly;
> +
> +/*
> + * A breakpoint was added to the code address we are about to
> + * modify, and this is the handle that will just skip over it.
> + * We are either changing a nop into a trace call, or a trace
> + * call to a nop. While the change is taking place, we treat
> + * it just like it was a nop.
> + */
> +int ftrace_int3_handler(int cmd, const char *str,
> +		 struct pt_regs *regs, long err, int trap, int sig)
> +{
> +	if (!modifying_ftrace_code || cmd != DIE_INT3 || !regs)
> +		return 0;
> +

At this moment, all of these checks are meaningless, because
this is not a handler but just a fixup code in int3 isr. :)
So, I think you can just define "int ftrace_int3_fixup(regs)"
for this purpose.

> +	if (!ftrace_location(regs->ip - 1))
> +		return 0;
> +
> +	regs->ip += MCOUNT_INSN_SIZE - 1;
> +
> +	return 1;
> +}

[...]

> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index ff9281f1..1712485 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -50,6 +50,7 @@
>  #include <asm/processor.h>
>  #include <asm/debugreg.h>
>  #include <linux/atomic.h>
> +#include <asm/ftrace.h>
>  #include <asm/traps.h>
>  #include <asm/desc.h>
>  #include <asm/i387.h>
> @@ -303,8 +304,14 @@ gp_in_kernel:
>  }
>  
>  /* May run on IST stack. */
> -dotraplinkage void __kprobes do_int3(struct pt_regs *regs, long error_code)
> +dotraplinkage void __kprobes notrace do_int3(struct pt_regs *regs, long error_code)
>  {
> +#ifdef CONFIG_FUNCTION_TRACER

I guess CONFIG_DYNAMIC_FTRACE ?

> +	/* ftrace must be first, everything else may cause a recursive crash */
> +	if (unlikely(modifying_ftrace_code) &&
> +	    ftrace_int3_handler(DIE_INT3, "int3", regs, error_code, 3, SIGTRAP))
> +		return;
> +#endif

Thank you!

-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@...achi.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/