Re: [RFC PATCH v2 20/24] unwind_user/eh_frame/x86: Handle PLT expressions

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The patch claims to support i386 PLT unwinding but fundamentally breaks it by hardcoding x86-64 DWARF register macros.
--

--- Patch [20]: [RFC PATCH v2 20/24] unwind_user/eh_frame/x86: Handle PLT expressions ---
commit 75d2af7b6df62f61cc44905f96f37d91b80e466a
Author: Jens Remus <[email protected]>

unwind_user/eh_frame/x86: Handle PLT expressions

This commit adds x86-specific support for handling the CFA expression
found in GNU linker generated .eh_frame for PLT. It intends to enable
unwinding through GNU linker generated PLT entries on both i386 and
x86-64.

> diff --git a/arch/x86/include/asm/unwind_user_eh_frame.h b/arch/x86/include/asm/unwind_user_eh_frame.h
> index fdccbda2fe4b1..8268eb1a0ff5a 100644
> --- a/arch/x86/include/asm/unwind_user_eh_frame.h
> +++ b/arch/x86/include/asm/unwind_user_eh_frame.h
[ ... ]
> @@ -15,7 +17,71 @@
>  #define EH_FRAME_MIN_DATA_ALIGN -8
>  #define EH_FRAME_MAX_DATA_ALIGN -1
>  
> -#endif
> +#endif /* CONFIG_X86_64 */
[ ... ]
> +static inline int eh_frame_do_def_cfa_expression(const char *expr,
> +						 int size,
> +						 unsigned long ip,
> +						 struct eh_frame_reg_state *reg_state)
> +{
[ ... ]
> +		if (!DW_OP_is_breg(sp_op) || !DW_OP_is_breg(ip_op))
> +			return -EOPNOTSUPP;
> +
> +		sp_reg = DW_OP_breg_register(sp_op);
> +		ip_reg = DW_OP_breg_register(ip_op);
> +		if (sp_reg != EH_FRAME_REG_SP || ip_reg != EH_FRAME_REG_RA)
> +			return -EOPNOTSUPP;

[Severity: Medium]
The commit message mentions enabling unwinding for both i386 and x86-64,
but does this check inadvertently break i386 support?

EH_FRAME_REG_SP and EH_FRAME_REG_RA are defined under CONFIG_X86_64 and
correspond to x86-64 DWARF register numbers (7 and 16). For i386, the DWARF
register numbers are different (ESP is 4, EIP is 8).

When unwinding an i386 PLT expression, such as for a 32-bit compat process
running under a 64-bit kernel, will this check unconditionally reject the
expression and return -EOPNOTSUPP because 4 does not equal 7?

Also, if this code is compiled natively for 32-bit, would this cause a build
failure because the EH_FRAME_REG_SP and EH_FRAME_REG_RA macros are not
defined outside of the CONFIG_X86_64 block?

> +
> +		/* CFA = (SP + SP_offset) + (((IP & 0xf) >= N) << shift) */
> +		reg_state->cfa_rule = CFA_REG_OFFSET;
> +		reg_state->cfa_regnum = EH_FRAME_REG_SP;
> +		reg_state->cfa_offset = sp_offset + (((ip & 15) >= n) << shift);
> +		return 0;
> +	}
> +
> +	return -EOPNOTSUPP;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=20