Re: [PATCH] kmsan: fix false warnings in return_address on s390

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Brittle backtrace logic in return_address_noinstr dependent on unpredictable compiler stack frame allocation.

Pre-existing issues:
- [High] Preexisting off-by-one error in return_address() resolution on s390x.
--

commit 6db102bd4515b51f9b3acc19ab3e650b460d0d09
Author: Aleksei Nikiforov <aleksei.nikiforov@linux.ibm.com>

kmsan: fix false warnings in return_address on s390

This commit fixes false KMSAN warnings in the s390 return_address logic
by marking a wrapper function as noinstr. It manually accounts for the
additional wrapper function's stack frame by incrementing the backtrace
depth.

> diff --git a/arch/s390/include/asm/ftrace.h b/arch/s390/include/asm/ftrace.h
> index 692c484ec1630..9a9f7f2f2c369 100644
> --- a/arch/s390/include/asm/ftrace.h
> +++ b/arch/s390/include/asm/ftrace.h
> @@ -23,7 +23,13 @@ static __always_inline unsigned long return_address(unsigned int n)
>  	} while (--n);
>  	return sf->gprs[8];

[Severity: High]
This isn't a bug introduced by this patch, but is this preexisting
off-by-one behavior correct when resolving caller IPs?

On s390x, a function's prologue saves its registers, including the return
address, into its caller's stack frame at gprs[8]. In return_address, when
n=1, the loop executes exactly once, advancing the stack frame pointer to
the immediate caller's frame. It then returns its gprs[8].

Because of the stack frame layout, this yields the instruction pointer of
the immediate caller (CALLER_ADDR0) instead of the parent caller. To
correctly return the parent caller (CALLER_ADDR1), shouldn't the loop
advance n+1 times? Lockdep and tracing paths rely on differentiating
CALLER_ADDR0 from CALLER_ADDR1, and this currently causes CALLER_ADDR1 to
evaluate to the exact same instruction pointer as CALLER_ADDR0.

>  }
> +
> +#ifdef CONFIG_KMSAN
> +unsigned long return_address_noinstr(unsigned int n);
> +#define ftrace_return_address(n) return_address_noinstr(n)
> +#else
>  #define ftrace_return_address(n) return_address(n)
> +#endif
>  
>  void ftrace_caller(void);

[ ... ]

> diff --git a/arch/s390/kernel/stacktrace.c b/arch/s390/kernel/stacktrace.c
> index 18520d3330581..db458050d8bf3 100644
> --- a/arch/s390/kernel/stacktrace.c
> +++ b/arch/s390/kernel/stacktrace.c
> @@ -146,3 +146,11 @@ void arch_stack_walk_user(stack_trace_consume_fn consume_entry, void *cookie,
>  {
>  	arch_stack_walk_user_common(consume_entry, cookie, NULL, regs, false);
>  }
> +
> +#ifdef CONFIG_KMSAN
> +noinstr unsigned long return_address_noinstr(unsigned int n)
> +{
> +	/* Add 1 to account for call of uninlined function return_address_noinstr */
> +	return return_address(n+1);

[Severity: Medium]
Is it guaranteed that the compiler will allocate a stack frame for this
function?

This wrapper is a noinstr leaf function that only calls the
__always_inline return_address function. If the compiler optimizes it
as a standard leaf function without a stack frame, current_frame_address
inside return_address will yield the caller's stack frame instead.

If that happens, the manual +1 adjustment here will skip an extra frame
unintentionally. Does the compiler consistently allocate a stack frame
here due to sanitizer flags forcing frame pointers, or is this backtrace
logic vulnerable to unpredictable compiler heuristics?

> +}
> +#endif

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831152811.3817938-2-aleksei.nikiforov@linux.ibm.com?part=1