Re: [PATCH 4/4] arm64: alternatives: Make branch and ADRP offset helpers noinst

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] arm64: insn: branch profiling instrumentation in noinstr
  offset helpers
--

commit b08429609214cdffaa257c9ef953b28e179973db
Author: Jisheng Zhang <jszhang@kernel.org>

arm64: alternatives: Make branch and ADRP offset helpers noinst

This commit marks the branch and ADRP offset helpers noinstr and introduces
__always_inline immediate encoding and decoding helpers so that
patch_alternative() does not call instrumentable code while modifying
instructions.

> diff --git a/arch/arm64/lib/insn.c b/arch/arm64/lib/insn.c
> index e70ac02385153..b98610fcab32e 100644
> --- a/arch/arm64/lib/insn.c
> +++ b/arch/arm64/lib/insn.c

[ ... ]

> @@ -1299,23 +1316,23 @@ u32 aarch64_insn_gen_adr(unsigned long pc, unsigned long addr,
>   * signed value (so it can be used when computing a new branch
>   * target).
>   */
> -s32 aarch64_get_branch_offset(u32 insn)
> +s32 noinstr aarch64_get_branch_offset(u32 insn)
>  {
>  	s32 imm;
>  
>  	if (aarch64_insn_is_b(insn) || aarch64_insn_is_bl(insn)) {
> -		imm = aarch64_insn_decode_immediate(AARCH64_INSN_IMM_26, insn);
> +		imm = __aarch64_insn_decode_immediate(AARCH64_INSN_IMM_26, insn);
>  		return (imm << 6) >> 4;
>  	}
>  
>  	if (aarch64_insn_is_cbz(insn) || aarch64_insn_is_cbnz(insn) ||
>  	    aarch64_insn_is_bcond(insn)) {
> -		imm = aarch64_insn_decode_immediate(AARCH64_INSN_IMM_19, insn);
> +		imm = __aarch64_insn_decode_immediate(AARCH64_INSN_IMM_19, insn);
>  		return (imm << 13) >> 11;
>  	}
>  
>  	if (aarch64_insn_is_tbz(insn) || aarch64_insn_is_tbnz(insn)) {
> -		imm = aarch64_insn_decode_immediate(AARCH64_INSN_IMM_14, insn);
> +		imm = __aarch64_insn_decode_immediate(AARCH64_INSN_IMM_14, insn);
>  		return (imm << 18) >> 16;
>  	}
>  
> @@ -1327,35 +1344,35 @@ s32 aarch64_get_branch_offset(u32 insn)
>   * Encode the displacement of a branch in the imm field and return the
>   * updated instruction.
>   */
> -u32 aarch64_set_branch_offset(u32 insn, s32 offset)
> +u32 noinstr aarch64_set_branch_offset(u32 insn, s32 offset)
>  {
>  	if (aarch64_insn_is_b(insn) || aarch64_insn_is_bl(insn))
> -		return aarch64_insn_encode_immediate(AARCH64_INSN_IMM_26, insn,
> +		return __aarch64_insn_encode_immediate(AARCH64_INSN_IMM_26, insn,
>  						     offset >> 2);
>  
>  	if (aarch64_insn_is_cbz(insn) || aarch64_insn_is_cbnz(insn) ||
>  	    aarch64_insn_is_bcond(insn))
> -		return aarch64_insn_encode_immediate(AARCH64_INSN_IMM_19, insn,
> +		return __aarch64_insn_encode_immediate(AARCH64_INSN_IMM_19, insn,
>  						     offset >> 2);
>  
>  	if (aarch64_insn_is_tbz(insn) || aarch64_insn_is_tbnz(insn))
> -		return aarch64_insn_encode_immediate(AARCH64_INSN_IMM_14, insn,
> +		return __aarch64_insn_encode_immediate(AARCH64_INSN_IMM_14, insn,
>  						     offset >> 2);
>  
>  	/* Unhandled instruction */
>  	BUG();
>  }
>  
> -s32 aarch64_insn_adrp_get_offset(u32 insn)
> +s32 noinstr aarch64_insn_adrp_get_offset(u32 insn)
>  {
>  	BUG_ON(!aarch64_insn_is_adrp(insn));
> -	return aarch64_insn_decode_immediate(AARCH64_INSN_IMM_ADR, insn) << 12;
> +	return __aarch64_insn_decode_immediate(AARCH64_INSN_IMM_ADR, insn) << 12;
>  }
>  
> -u32 aarch64_insn_adrp_set_offset(u32 insn, s32 offset)
> +u32 noinstr aarch64_insn_adrp_set_offset(u32 insn, s32 offset)
>  {
>  	BUG_ON(!aarch64_insn_is_adrp(insn));
> -	return aarch64_insn_encode_immediate(AARCH64_INSN_IMM_ADR, insn,
> +	return __aarch64_insn_encode_immediate(AARCH64_INSN_IMM_ADR, insn,
>  						offset >> 12);
>  }

[Severity: Medium]
Can these four noinstr helpers still call into ftrace_likely_update() when
CONFIG_TRACE_BRANCH_PROFILING is enabled?

Although f22c81bebf8b ("arm64: Disable branch profiling for all arm64 code")
added -DDISABLE_BRANCH_PROFILING to arch/arm64/Kbuild, arch/arm64/lib/ is
built via libs-y in arch/arm64/Makefile rather than arch/arm64/Kbuild, so
arch/arm64/lib/insn.o is compiled without DISABLE_BRANCH_PROFILING.

As a result, the generic BUG_ON() in aarch64_insn_adrp_get_offset() and
aarch64_insn_adrp_set_offset() expands to unlikely(), and the if statements
in aarch64_get_branch_offset() and aarch64_set_branch_offset() are
instrumented under CONFIG_PROFILE_ALL_BRANCHES.  When patch_alternative()
calls these helpers while patching live instructions before cache
maintenance completes, they call the instrumentable ftrace_likely_update().

Passing -DDISABLE_BRANCH_PROFILING in arch/arm64/lib/Makefile (or defining
DISABLE_BRANCH_PROFILING in arch/arm64/lib/insn.c) would prevent branch
profiling instrumentation here.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107071339.17467-1-jszhang@kernel.org?part=4