linux-kernel - Re: [PATCH 1/2] ftrace: Introduce ftrace_need_init

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: <20210730220741.4da6fdf6@oasis.local.home>
Date:   Fri, 30 Jul 2021 22:07:41 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Ilya Leoshkevich <iii@...ux.ibm.com>
Cc:     Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org,
        Heiko Carstens <hca@...ux.ibm.com>,
        Sven Schnelle <svens@...ux.ibm.com>,
        Vasily Gorbik <gor@...ux.ibm.com>, linux-s390@...r.kernel.org
Subject: Re: [PATCH 1/2] ftrace: Introduce ftrace_need_init_nop()

On Wed, 28 Jul 2021 23:25:45 +0200
Ilya Leoshkevich <iii@...ux.ibm.com> wrote:

> Implementing live patching on s390 requires each function's prologue to
> contain a very special kind of nop, which gcc and clang don't generate.
> However, the current code assumes that if CC_USING_NOP_MCOUNT is
> defined, then whatever the compiler generates is good enough.
> 
> Move the CC_USING_NOP_MCOUNT check into the new ftrace_need_init_nop()
> macro, that the architectures can override.
> 
> An alternative solution is to disable using -mnop-mcount in the
> Makefile, however, this makes the build logic (even) more complicated
> and forces the arch-specific code to deal with the useless __fentry__
> symbol.
> 
> Signed-off-by: Ilya Leoshkevich <iii@...ux.ibm.com>

I'm fine with this patch, and also you taking it in via the s390 tree.

Reviewed-by: Steven Rostedt (VMware) <rostedt@...dmis.org>

-- Steve


> ---
>  include/linux/ftrace.h | 16 ++++++++++++++++
>  kernel/trace/ftrace.c  |  4 ++--
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index a69f363b61bf..f1ea8988a045 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -643,6 +643,22 @@ static inline int ftrace_disable_ftrace_graph_caller(void) { return 0; }
>  extern int ftrace_make_nop(struct module *mod,
>  			   struct dyn_ftrace *rec, unsigned long addr);
>  
> +/**
> + * ftrace_need_init_nop - return whether nop call sites should be initialized
> + *
> + * Normally the compiler's -mnop-mcount generates suitable nops, so we don't
> + * need to call ftrace_init_nop() if the code is built with that flag.
> + * Architectures where this is not always the case may define their own
> + * condition.
> + *
> + * Return must be:
> + *  0       if ftrace_init_nop() should be called
> + *  Nonzero if ftrace_init_nop() should not be called
> + */
> +
> +#ifndef ftrace_need_init_nop
> +#define ftrace_need_init_nop() (!__is_defined(CC_USING_NOP_MCOUNT))
> +#endif
>  
>  /**
>   * ftrace_init_nop - initialize a nop call site
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 7b180f61e6d3..7efbc8aaf7f6 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -3100,6 +3100,7 @@ ops_references_rec(struct ftrace_ops *ops, struct dyn_ftrace *rec)
>  
>  static int ftrace_update_code(struct module *mod, struct ftrace_page *new_pgs)
>  {
> +	bool init_nop = ftrace_need_init_nop();
>  	struct ftrace_page *pg;
>  	struct dyn_ftrace *p;
>  	u64 start, stop;
> @@ -3138,8 +3139,7 @@ static int ftrace_update_code(struct module *mod, struct ftrace_page *new_pgs)
>  			 * Do the initial record conversion from mcount jump
>  			 * to the NOP instructions.
>  			 */
> -			if (!__is_defined(CC_USING_NOP_MCOUNT) &&
> -			    !ftrace_nop_initialize(mod, p))
> +			if (init_nop && !ftrace_nop_initialize(mod, p))
>  				break;
>  
>  			update_cnt++;