Re: [RFC PATCH v2 11/20] objtool: arm64: Walk instructions and compute CFI for each instruction

From: Chen Zhongjin <chenzhongjin@huawei.com>
To: <madvenka@linux.microsoft.com>, <jpoimboe@redhat.com>,
	<peterz@infradead.org>, <mark.rutland@arm.com>,
	<broonie@kernel.org>, <nobuta.keiya@fujitsu.com>,
	<sjitindarsingh@gmail.com>, <catalin.marinas@arm.com>,
	<will@kernel.org>, <jamorris@linux.microsoft.com>,
	<linux-arm-kernel@lists.infradead.org>,
	<live-patching@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH v2 11/20] objtool: arm64: Walk instructions and compute CFI for each instruction
Date: Tue, 24 May 2022 21:45:19 +0800	[thread overview]
Message-ID: <767e814c-b3cc-73a1-d283-82dbc0287cde@huawei.com> (raw)
In-Reply-To: <20220524001637.1707472-12-madvenka@linux.microsoft.com>

Hi,

On 2022/5/24 8:16, madvenka@linux.microsoft.com wrote:
> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
> 
> Implement arch_initial_func_cfi_state() to initialize the CFI for a
> function.
> 
> Add code to fpv_decode() to walk the instructions in every function and
> compute the CFI information for each instruction.
> 
> Implement special handling for cases like jump tables.
> 
> Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
> ---
>  tools/objtool/arch/arm64/decode.c |  15 +++
>  tools/objtool/fpv.c               | 204 ++++++++++++++++++++++++++++++
>  2 files changed, 219 insertions(+)
...
> +static void update_cfi_state(struct cfi_state *cfi, struct stack_op *op)
> +{
> +	struct cfi_reg *cfa = &cfi->cfa;
> +	struct cfi_reg *regs = cfi->regs;
> +
> +	if (op->src.reg == CFI_SP) {
> +		if (op->dest.reg == CFI_SP)
> +			cfa->offset -= op->src.offset;
> +		else
> +			regs[CFI_FP].offset = -cfa->offset + op->src.offset;
Seems wrong here, we don't have any op->src.offset for [mov x29, sp] so here we
get: fp->offset = -cfa->offset. The dumped info also proves this.

> +	case UNWIND_HINT_TYPE_CALL:
> +		/* Normal call */
> +		frame->cfa += orc->sp_offset;
> +		fp = frame->cfa + orc->fp_offset;
> +		break;
Obviously this is not conform to the reliability check because we get
frame->cfa == fp here.

IIUC your sp_offset equals to stack length, and fp_offset is offset from next
x29 to next CFA. So maybe here we should have
regs[CFI_FP].offset = regs[CFI_SP].offset for [mov x29, sp].

Anyway, in original objtool sp_offset and fp_offset both represents the offset
from CFA to REGs. I think it's better not spoiling their original meaning and
just extending.