Re: [PATCH v3 22/28] x86: Use RCU in all users of __module_address().

[Michal Pecio <michal.pecio@gmail.com>]

> x86: Use RCU in all users of __module_address().
>
> __module_address() can be invoked within a RCU section, there is no
> requirement to have preemption disabled.
> 
> Replace the preempt_disable() section around __module_address() with
> RCU.
> 
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Josh Poimboeuf <jpoimboe@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: x86@kernel.org
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  arch/x86/kernel/callthunks.c | 3 +--
>  arch/x86/kernel/unwind_orc.c | 4 +---
>  2 files changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kernel/callthunks.c b/arch/x86/kernel/callthunks.c
> index f17d166078823..276b5368ff6b0 100644
> --- a/arch/x86/kernel/callthunks.c
> +++ b/arch/x86/kernel/callthunks.c
> @@ -98,11 +98,10 @@ static inline bool within_module_coretext(void *addr)
>  #ifdef CONFIG_MODULES
>  	struct module *mod;
>  
> -	preempt_disable();
> +	guard(rcu)();
>  	mod = __module_address((unsigned long)addr);
>  	if (mod && within_module_core((unsigned long)addr, mod))
>  		ret = true;
> -	preempt_enable();
>  #endif
>  	return ret;
>  }
> diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c
> index d4705a348a804..977ee75e047c8 100644
> --- a/arch/x86/kernel/unwind_orc.c
> +++ b/arch/x86/kernel/unwind_orc.c
> @@ -476,7 +476,7 @@ bool unwind_next_frame(struct unwind_state *state)
>  		return false;
>  
>  	/* Don't let modules unload while we're reading their ORC data. */
> -	preempt_disable();
> +	guard(rcu)();
>  
>  	/* End-of-stack check for user tasks: */
>  	if (state->regs && user_mode(state->regs))
> @@ -669,14 +669,12 @@ bool unwind_next_frame(struct unwind_state *state)
>  		goto err;
>  	}
>  
> -	preempt_enable();
>  	return true;

Hi,

There is a regression report on a distribution forum which involves
an out of tree module on a patched kernel (yes, I know) calling
stack_trace_save() in task context, which arrives here and apparently
calls the various deref_stack_xxx() functions with preemption enabled,
which in turn call stack_access_ok() leading to a BUG:

Nov 02 21:44:30 ArchBasement kernel: BUG: using smp_processor_id() in preemptible [00000000] code: Xorg/1183
Nov 02 21:44:30 ArchBasement kernel: caller is in_entry_stack+0x11/0x60
Nov 02 21:44:30 ArchBasement kernel: CPU: 0 UID: 1000 PID: 1183 Comm: Xorg Tainted: P           OE       6.16.12-hardened1-1-hardened #1 PREEMPT(full)  6edb90a7a07fab33bbee72d6d5ef53ba6eec3b9c
Nov 02 21:44:30 ArchBasement kernel: Tainted: [P]=PROPRIETARY_MODULE, [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
Nov 02 21:44:30 ArchBasement kernel: Hardware name: ASUS All Series/Z97-E, BIOS 0803 02/23/2016
Nov 02 21:44:30 ArchBasement kernel: Call Trace:
Nov 02 21:44:30 ArchBasement kernel:  <TASK>
Nov 02 21:44:30 ArchBasement kernel:  dump_stack_lvl+0x5d/0x80
Nov 02 21:44:30 ArchBasement kernel:  check_preemption_disabled+0xe5/0xf0
Nov 02 21:44:30 ArchBasement kernel:  in_entry_stack+0x11/0x60
Nov 02 21:44:30 ArchBasement kernel:  get_stack_info+0x2c/0x80
Nov 02 21:44:30 ArchBasement kernel:  stack_access_ok+0x51/0xa0
Nov 02 21:44:30 ArchBasement kernel:  unwind_next_frame+0x1cb/0x7b0
Nov 02 21:44:30 ArchBasement kernel:  ? _nv003168kms+0x42/0x50 [nvidia_modeset 90775ea8a26c5e58b97ef4b3f46eb45efa040eb2]
Nov 02 21:44:30 ArchBasement kernel:  ? __pfx_stack_trace_consume_entry+0x10/0x10
Nov 02 21:44:30 ArchBasement kernel:  arch_stack_walk+0xa6/0x110
Nov 02 21:44:30 ArchBasement kernel:  ? _nv003168kms+0x42/0x50 [nvidia_modeset 90775ea8a26c5e58b97ef4b3f46eb45efa040eb2]
Nov 02 21:44:30 ArchBasement kernel:  stack_trace_save+0x4d/0x70

Is this nvidia doing something wrong, or a problem with this commit?

The removed code suggests that preemption is allowed here, and as far
as I see, this call trace is still possible on vanilla 6.18. Perhaps
preempt_disable() needs to be restored around this code?

Regards,
Michal