Hello Maria,
On Wed, 27 Sept 2023 at 06:00, Maria Yu <quic_aiquny@xxxxxxxxxxx> wrote:
>
> Registers r7 is removed in clobber list, so compiler may choose r7 for
> local variables usage, while r7 will be actually updated by the inline asm
> code.
The inline asm does not update R7, it preserves and restores it.
> This caused the runtime behavior wrong.
Could you explain how, exactly? In which cases is the preserve/restore
of R7 failing to achieve the intended result?
> While those kind of reserved registers cannot be set to clobber list
> because of error like "inline asm clobber list contains reserved
> registers".
> To both working for reserved register case and non-reserved register case,
> explicitly assign register for local variables which will be used as asm
> input.
>
If we make this change, could we remove the references to R7 altogether?
> Fixes: dd12e97f3c72 ("ARM: kprobes: treat R7 as the frame pointer register in Thumb2 builds")
> Signed-off-by: Maria Yu <quic_aiquny@xxxxxxxxxxx>
> ---
> arch/arm/probes/kprobes/actions-thumb.c | 32 ++++++++++++++++---------
> 1 file changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/arch/arm/probes/kprobes/actions-thumb.c b/arch/arm/probes/kprobes/actions-thumb.c
> index 51624fc263fc..f667b2f00b3e 100644
> --- a/arch/arm/probes/kprobes/actions-thumb.c
> +++ b/arch/arm/probes/kprobes/actions-thumb.c
> @@ -442,8 +442,10 @@ static unsigned long __kprobes
> t16_emulate_loregs(probes_opcode_t insn,
> struct arch_probes_insn *asi, struct pt_regs *regs)
> {
> - unsigned long oldcpsr = regs->ARM_cpsr;
> - unsigned long newcpsr;
> + register unsigned long oldcpsr asm("r8") = regs->ARM_cpsr;
> + register unsigned long newcpsr asm("r9");
> + register void *rregs asm("r10") = regs;
> + register void *rfn asm("lr") = asi->insn_fn;
>
> __asm__ __volatile__ (
> "msr cpsr_fs, %[oldcpsr] \n\t"
> @@ -454,10 +456,10 @@ t16_emulate_loregs(probes_opcode_t insn,
> "mov r7, r11 \n\t"
> "mrs %[newcpsr], cpsr \n\t"
> : [newcpsr] "=r" (newcpsr)
> - : [oldcpsr] "r" (oldcpsr), [regs] "r" (regs),
> - [fn] "r" (asi->insn_fn)
> + : [oldcpsr] "r" (oldcpsr), [regs] "r" (rregs),
> + [fn] "r" (rfn)
> : "r0", "r1", "r2", "r3", "r4", "r5", "r6", "r11",
> - "lr", "memory", "cc"
> + "memory", "cc"
> );
>
> return (oldcpsr & ~APSR_MASK) | (newcpsr & APSR_MASK);
> @@ -525,6 +527,9 @@ static void __kprobes
> t16_emulate_push(probes_opcode_t insn,
> struct arch_probes_insn *asi, struct pt_regs *regs)
> {
> + register void *rfn asm("lr") = asi->insn_fn;
> + register void *rregs asm("r10") = regs;
> +
> __asm__ __volatile__ (
> "mov r11, r7 \n\t"
> "ldr r9, [%[regs], #13*4] \n\t"
> @@ -534,9 +539,9 @@ t16_emulate_push(probes_opcode_t insn,
> "str r9, [%[regs], #13*4] \n\t"
> "mov r7, r11 \n\t"
> :
> - : [regs] "r" (regs), [fn] "r" (asi->insn_fn)
> + : [regs] "r" (rregs), [fn] "r" (rfn)
> : "r0", "r1", "r2", "r3", "r4", "r5", "r6", "r8", "r9", "r11",
> - "lr", "memory", "cc"
> + "memory", "cc"
> );
> }
>
> @@ -561,6 +566,9 @@ static void __kprobes
> t16_emulate_pop_nopc(probes_opcode_t insn,
> struct arch_probes_insn *asi, struct pt_regs *regs)
> {
> + register void *rfn asm("lr") = asi->insn_fn;
> + register void *rregs asm("r8") = regs;
> +
> __asm__ __volatile__ (
> "mov r11, r7 \n\t"
> "ldr r9, [%[regs], #13*4] \n\t"
> @@ -570,9 +578,9 @@ t16_emulate_pop_nopc(probes_opcode_t insn,
> "str r9, [%[regs], #13*4] \n\t"
> "mov r7, r11 \n\t"
> :
> - : [regs] "r" (regs), [fn] "r" (asi->insn_fn)
> + : [regs] "r" (rregs), [fn] "r" (rfn)
> : "r0", "r1", "r2", "r3", "r4", "r5", "r6", "r9", "r11",
> - "lr", "memory", "cc"
> + "memory", "cc"
> );
> }
>
> @@ -581,6 +589,8 @@ t16_emulate_pop_pc(probes_opcode_t insn,
> struct arch_probes_insn *asi, struct pt_regs *regs)
> {
> register unsigned long pc asm("r8");
> + register void *rfn asm("lr") = asi->insn_fn;
> + register void *rregs asm("r10") = regs;
>
> __asm__ __volatile__ (
> "mov r11, r7 \n\t"
> @@ -591,9 +601,9 @@ t16_emulate_pop_pc(probes_opcode_t insn,
> "str r9, [%[regs], #13*4] \n\t"
> "mov r7, r11 \n\t"
> : "=r" (pc)
> - : [regs] "r" (regs), [fn] "r" (asi->insn_fn)
> + : [regs] "r" (rregs), [fn] "r" (rfn)
> : "r0", "r1", "r2", "r3", "r4", "r5", "r6", "r9", "r11",
> - "lr", "memory", "cc"
> + "memory", "cc"
> );
>
> bx_write_pc(pc, regs);
>
> base-commit: 6465e260f48790807eef06b583b38ca9789b6072
> --
> 2.17.1
>
