RE: [PATCH] RISC-V: insn: Use a raw spinlock to protect TEXT_POKE*

[Anup Patel]

> -----Original Message-----
> From: Steven Rostedt <rostedt@xxxxxxxxxxx>
> Sent: 30 April 2021 08:18
> To: Changbin Du <changbin.du@xxxxxxxxx>
> Cc: Palmer Dabbelt <palmer@xxxxxxxxxxx>; linux-riscv@xxxxxxxxxxxxxxxxxxx;
> Paul Walmsley <paul.walmsley@xxxxxxxxxx>; aou@xxxxxxxxxxxxxxxxx;
> peterz@xxxxxxxxxxxxx; jpoimboe@xxxxxxxxxx; jbaron@xxxxxxxxxx;
> ardb@xxxxxxxxxx; Atish Patra <Atish.Patra@xxxxxxx>; Anup Patel
> <Anup.Patel@xxxxxxx>; akpm@xxxxxxxxxxxxxxxxxxxx; rppt@xxxxxxxxxx;
> mhiramat@xxxxxxxxxx; zong.li@xxxxxxxxxx; guoren@xxxxxxxxxxxxxxxxx;
> wangkefeng.wang@xxxxxxxxxx; 0x7f454c46@xxxxxxxxx;
> chenhuang5@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; kernel-
> team@xxxxxxxxxxx; Palmer Dabbelt <palmerdabbelt@xxxxxxxxxx>
> Subject: Re: [PATCH] RISC-V: insn: Use a raw spinlock to protect TEXT_POKE*
> 
> On Fri, 30 Apr 2021 05:54:51 +0800
> Changbin Du <changbin.du@xxxxxxxxx> wrote:
> 
> > The problem is that lockdep cannot handle locks across tasks since we
> > use stopmachine to patch code for risc-v. So there's a false positive report.
> > See privious disscussion here
> 
> > https://lkml.org/lkml/2021/4/29/63
> 
> Please use lore.kernel.org, lkml.org is highly unreliable, and is considered
> deprecated for use of referencing linux kernel archives.
> 
> Would the following patch work?

This patch only takes care of ftrace path.

The RISC-V instruction patching is used by RISC-V jump label implementation
as well and will called from various critical parts of core kernel.

The RAW spinlock approach allows same instruction patching to be used
for kprobes, ftrace, and jump label.

Regards,
Anup

> 
> (note, I did not even compile test it)
> 
> -- Steve
> 
> diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
> index 845002cc2e57..19acbb4aaeff 100644
> --- a/arch/riscv/include/asm/ftrace.h
> +++ b/arch/riscv/include/asm/ftrace.h
> @@ -25,6 +25,8 @@ struct dyn_arch_ftrace {  };  #endif
> 
> +extern int running_ftrace;
> +
>  #ifdef CONFIG_DYNAMIC_FTRACE
>  /*
>   * A general call in RISC-V is a pair of insts:
> diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c index
> 7f1e5203de88..834ab4fad637 100644
> --- a/arch/riscv/kernel/ftrace.c
> +++ b/arch/riscv/kernel/ftrace.c
> @@ -11,15 +11,19 @@
>  #include <asm/cacheflush.h>
>  #include <asm/patch.h>
> 
> +int running_ftrace;
> +
>  #ifdef CONFIG_DYNAMIC_FTRACE
>  int ftrace_arch_code_modify_prepare(void) __acquires(&text_mutex)  {
>  	mutex_lock(&text_mutex);
> +	running_ftrace = 1;
>  	return 0;
>  }
> 
>  int ftrace_arch_code_modify_post_process(void) __releases(&text_mutex)
> {
> +	running_ftrace = 0;
>  	mutex_unlock(&text_mutex);
>  	return 0;
>  }
> diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c index
> 0b552873a577..4cd1c79a9689 100644
> --- a/arch/riscv/kernel/patch.c
> +++ b/arch/riscv/kernel/patch.c
> @@ -12,6 +12,7 @@
>  #include <asm/cacheflush.h>
>  #include <asm/fixmap.h>
>  #include <asm/patch.h>
> +#include <asm/ftrace.h>
> 
>  struct patch_insn {
>  	void *addr;
> @@ -59,8 +60,13 @@ static int patch_insn_write(void *addr, const void
> *insn, size_t len)
>  	 * Before reaching here, it was expected to lock the text_mutex
>  	 * already, so we don't need to give another lock here and could
>  	 * ensure that it was safe between each cores.
> +	 *
> +	 * ftrace uses stop machine, and even though the text_mutex is
> +	 * held, the stop machine task that calls this function will not
> +	 * be the owner.
>  	 */
> -	lockdep_assert_held(&text_mutex);
> +	if (!running_ftrace)
> +		lockdep_assert_held(&text_mutex);
> 
>  	if (across_pages)
>  		patch_map(addr + len, FIX_TEXT_POKE1);