[PATCH v4] RISC-V: Don't check text_mutex during stop_machine

[Conor Dooley]

On Wed, Mar 01, 2023 at 10:38:53PM +0000, Conor Dooley wrote:
> From: Palmer Dabbelt <palmerdabbelt at google.com>
> 
> We're currently using stop_machine() to update ftrace & kprobes, which
> means that the thread that takes text_mutex during may not be the same
> as the thread that eventually patches the code.  This isn't actually a
> race because the lock is still held (preventing any other concurrent
> accesses) and there is only one thread running during stop_machine(),
> but it does trigger a lockdep failure.
> 
> This patch just elides the lockdep check during stop_machine.
> 
> Fixes: c15ac4fd60d5 ("riscv/ftrace: Add dynamic function tracer support")
> Suggested-by: Steven Rostedt <rostedt at goodmis.org>
> Reported-by: Changbin Du <changbin.du at gmail.com>
> Signed-off-by: Palmer Dabbelt <palmerdabbelt at google.com>
> Signed-off-by: Palmer Dabbelt <palmer at rivosinc.com>
> Signed-off-by: Conor Dooley <conor.dooley at microchip.com>
> ---
> Changes since v3 [<20230215164317.727657-1-conor at kernel.org>]:
> * rename the flag to riscv_patch_in_stop_machine as it is being used for
>   kprobes & ftrace, and just looked a bit odd.
> * implement Changbin's suggestion of checking the lock is held in
>   patch_text(), rather than set the flag in callers of patch_text().
> 
> Changes since v2 [<20220322022331.32136-1-palmer at rivosinc.com>]:
> * rebase on riscv/for-next as it as been a year.
> * incorporate Changbin's suggestion that init_nop should take the lock
>   rather than call prepare() & post_process().
> 
> Changes since v1 [<20210506071041.417854-1-palmer at dabbelt.com>]:
> * Use ftrace_arch_ocde_modify_{prepare,post_process}() to set the flag.
>   I remember having a reason I wanted the function when I wrote the v1,
>   but it's been almost a year and I forget what that was -- maybe I was
>   just crazy, the patch was sent at midnight.
> * Fix DYNAMIC_FTRACE=n builds.

> @@ -121,13 +129,25 @@ NOKPROBE_SYMBOL(patch_text_cb);
>  
>  int patch_text(void *addr, u32 insn)
>  {
> +	int ret;
>  	struct patch_insn patch = {
>  		.addr = addr,
>  		.insn = insn,
>  		.cpu_count = ATOMIC_INIT(0),
>  	};
>  
> -	return stop_machine_cpuslocked(patch_text_cb,
> -				       &patch, cpu_online_mask);
> +	/*
> +	 * kprobes takes text_mutex, before calling patch_text(), but as we call
> +	 * calls stop_machine(), the lockdep assertion in patch_insn_write()
> +	 * gets confused by the context in which the lock is taken.
> +	 * Instead, ensure the lock is held before calling stop_machine(), and
> +	 * set riscv_patch_in_stop_machine to skip the check in
> +	 * patch_insn_write().
> +	 */
> +	lockdep_assert_held(&text_mutex);
> +	riscv_patch_in_stop_machine = true;
> +	ret = stop_machine_cpuslocked(patch_text_cb, &patch, cpu_online_mask);
> +	riscv_patch_in_stop_machine = false;
> +	return ret;

*sigh*, automation reports that this is not possible:
../arch/riscv/kernel/patch.c:153:30: error: expression is not assignable
        riscv_patch_in_stop_machine = true;
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
../arch/riscv/kernel/patch.c:155:30: error: expression is not assignable
        riscv_patch_in_stop_machine = false;
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^

The rest of the patch (in the commit context) uses this variable inside
ftrace code, but as this one is in patch.c, it's compiled in whether or
not we have ftrace etc. On rv32 & nommu defconfigs, that results in a
build failure:
https://patchwork.kernel.org/project/linux-riscv/patch/20230301223853.1444332-1-conor@kernel.org/

A respin is in order...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-riscv/attachments/20230303/29625d48/attachment.sig>