Hello Maria,
On Wed, 27 Sept 2023 at 06:00, Maria Yu <[email protected]> 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 <[email protected]>
> ---
> 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
>
On 9/27/2023 5:26 PM, Ard Biesheuvel wrote:
> Hello Maria,
>
> On Wed, 27 Sept 2023 at 06:00, Maria Yu <[email protected]> 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.
That is the asm is updating r7 purposely and compiler still choose r7
for the asm local varialbe and use it inside the asm code.
So the change is to fix the issue when "r7 is removed from the clobber
list of current asm code while actually r7 shouldn't be choosed for the
current asm local variables".
The issue is only reproducible when ftrace is not enabled, and r7 is
removed from the current clobber list.
Let me have the assemble code that will make you understand better.
--the original code:
"mov r11, r7 \n\t"
...
"ldmia %[regs], {r0-r7} \n\t"
"blx %[fn] \n\t"
...
"mov r7, r11 \n\t"
--After compile to choose register for [fn] and [regs].
mov r11, r7
ldr r7, [r1, #16] //r7 used for store asi->insn_fn
...
ldmia.w ip, {r0, r1, r2, r3, r4, r5, r6, r7}
blx r7
...
mov r7,r11
The current change is to avoid by fix the registers for local variable
usage and not choose r7 for [fn].
>
>> 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?
explained above.
>
>> 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?
R7 needed to be prepared for use when blx to the final fn for kprobe
functional as my understanding.
could you elaborate more on this question?
>
>> Fixes: dd12e97f3c72 ("ARM: kprobes: treat R7 as the frame pointer register in Thumb2 builds")
>> Signed-off-by: Maria Yu <[email protected]>
>> ---
>> 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)
We have encounter the runtime issue when compiler choose r7 for store
"asi->insn_fn" here for example above.
>> : "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
>>
--
Thx and BRs,
Aiqun(Maria) Yu