Re: [PATCH v2 2/3] objtool: Handle secondary stack related instructions

From: Tiezhu Yang
Date: Mon Aug 05 2024 - 02:38:17 EST


On 08/05/2024 11:26 AM, Tiezhu Yang wrote:
After commit a0f7085f6a63 ("LoongArch: Add RANDOMIZE_KSTACK_OFFSET
support"), there is a new instruction "sub.d $sp, $sp, $t0" for the
secondary stack in do_syscall(), then there exists a objtool warning
"do_syscall+0x11c: return with modified stack frame" and there is no
handle_syscall() which is the previous frame of do_syscall() in the
call trace when executing the command "echo l > /proc/sysrq-trigger".

...

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 01237d167223..c7b9942fee29 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2993,6 +2993,28 @@ static int update_cfi_state(struct instruction *insn,
break;
}

+ if (op->dest.reg == CFI_BP && op->src.reg == CFI_SP) {
+ /* addi.d fp,sp,imm for the secondary stack on LoongArch */
+ if (cfa->base == CFI_SP && cfa->offset == op->src.offset) {
+ if (insn->sym->secondary_stack) {
+ cfa->base = CFI_BP;
+ cfa->offset = 0;
+ }
+ }
+ break;
+ }
+
+ if (op->dest.reg == CFI_SP && op->src.reg == CFI_BP) {
+ /* addi.d sp,fp,imm for the secondary stack on LoongArch */
+ if (cfa->base == CFI_FP && cfa->offset == 0) {

Here should be CFI_BP instead of CFI_FP which is only defined
for LoongArch.

+ if (insn->sym->secondary_stack) {
+ cfa->base = CFI_SP;
+ cfa->offset = -op->src.offset;
+ }
+ }
+ break;
+ }
+
if (op->dest.reg == CFI_SP && op->src.reg == CFI_BP) {

/* lea disp(%rbp), %rsp */

Oh, sorry, I forgot to test this change on x86.

Here is the test info on x86: the cfa->base is CFI_BP
or CFI_BP_INDIRECT and the cfa->offset is not 0
if (op->dest.reg == CFI_SP && op->src.reg == CFI_BP),
thus it can check the following condition
if(cfa->base == CFI_BP && cfa->offset == 0)
to distinguish x86 and LoongArch.

So the correct change should be something like this
to make sure it works well for both x86 and LoongArch:

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 01237d167223..0832d20c95d2 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2993,10 +2993,28 @@ static int update_cfi_state(struct instruction *insn,
break;
}

- if (op->dest.reg == CFI_SP && op->src.reg == CFI_BP) {
+ if (op->dest.reg == CFI_BP && op->src.reg == CFI_SP) {
+ /* addi.d fp,sp,imm for the secondary stack on LoongArch */
+ if (cfa->base == CFI_SP && cfa->offset == op->src.offset) {
+ if (insn->sym->secondary_stack) {
+ cfa->base = CFI_BP;
+ cfa->offset = 0;
+ }
+ }
+ break;
+ }

- /* lea disp(%rbp), %rsp */
- cfi->stack_size = -(op->src.offset + regs[CFI_BP].offset);
+ if (op->dest.reg == CFI_SP && op->src.reg == CFI_BP) {
+ /* addi.d sp,fp,imm for the secondary stack on LoongArch */
+ if (cfa->base == CFI_BP && cfa->offset == 0) {
+ if (insn->sym->secondary_stack) {
+ cfa->base = CFI_SP;
+ cfa->offset = -op->src.offset;
+ }
+ } else {
+ /* lea disp(%rbp), %rsp */
+ cfi->stack_size = -(op->src.offset + regs[CFI_BP].offset);
+ }
break;
}

I will wait for some days to get more review comments
and then send v3 later.

Thanks,
Tiezhu