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. This caused the runtime behavior wrong.
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.
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 Wed, 27 Sept 2023 at 09:44, Aiqun(Maria) Yu <[email protected]> wrote:
>
> 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].
>
OK, I understand now, thanks.
Would it help if we just always enabled frame pointers for this source file?
--- a/arch/arm/probes/kprobes/Makefile
+++ b/arch/arm/probes/kprobes/Makefile
@@ -9,6 +9,7 @@ test-kprobes-objs := test-core.o
ifdef CONFIG_THUMB2_KERNEL
obj-$(CONFIG_KPROBES) += actions-thumb.o checkers-thumb.o
test-kprobes-objs += test-thumb.o
+KBUILD_CFLAGS += -fno-omit-frame-pointer
else
obj-$(CONFIG_KPROBES) += actions-arm.o checkers-arm.o
obj-$(CONFIG_OPTPROBES) += opt-arm.o
On 9/27/2023 6:11 PM, Ard Biesheuvel wrote:
> On Wed, 27 Sept 2023 at 09:44, Aiqun(Maria) Yu <[email protected]> wrote:
>>
>> 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].
>>
>
> OK, I understand now, thanks.
>
> Would it help if we just always enabled frame pointers for this source file?
>
> --- a/arch/arm/probes/kprobes/Makefile
> +++ b/arch/arm/probes/kprobes/Makefile
> @@ -9,6 +9,7 @@ test-kprobes-objs := test-core.o
> ifdef CONFIG_THUMB2_KERNEL
> obj-$(CONFIG_KPROBES) += actions-thumb.o checkers-thumb.o
> test-kprobes-objs += test-thumb.o
> +KBUILD_CFLAGS += -fno-omit-frame-pointer
I've tried this before, but it didn't work.
The compiler(clang 17 here I am using) will still complain for "inline
asm clobber list contains reserved registers" when ftrace enabled.
> else
> obj-$(CONFIG_KPROBES) += actions-arm.o checkers-arm.o
> obj-$(CONFIG_OPTPROBES) += opt-arm.o
--
Thx and BRs,
Aiqun(Maria) Yu
On 9/28/2023 8:40 AM, Aiqun(Maria) Yu wrote:
> On 9/27/2023 6:11 PM, Ard Biesheuvel wrote:
>> On Wed, 27 Sept 2023 at 09:44, Aiqun(Maria) Yu
>> <[email protected]> wrote:
>>>
>>> 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].
>>>
>>
>> OK, I understand now, thanks.
>>
>> Would it help if we just always enabled frame pointers for this source
>> file?
The compiler(clang 17 here I am using) will still complain for "inline
asm clobber list contains reserved registers" when ftrace enabled.
More over, the local variable have fixed registers was align with
current solution as well. we referenced other asm inlined function
inside kprobe folder, like __kprobes functions inside actions-common.c
files, it is also have similar solution to fixed register usage for
local variables which will be used inside similar inline asm code.
>>
>> --- a/arch/arm/probes/kprobes/Makefile
>> +++ b/arch/arm/probes/kprobes/Makefile
>> @@ -9,6 +9,7 @@ test-kprobes-objs := test-core.o
>> ifdef CONFIG_THUMB2_KERNEL
>> obj-$(CONFIG_KPROBES) += actions-thumb.o checkers-thumb.o
>> test-kprobes-objs += test-thumb.o
>> +KBUILD_CFLAGS += -fno-omit-frame-pointer
> I've tried this before, but it didn't work.
> The compiler(clang 17 here I am using) will still complain for "inline
> asm clobber list contains reserved registers" when ftrace enabled.
>> else
>> obj-$(CONFIG_KPROBES) += actions-arm.o checkers-arm.o
>> obj-$(CONFIG_OPTPROBES) += opt-arm.o
>
--
Thx and BRs,
Aiqun(Maria) Yu
On Thu, 28 Sept 2023 at 00:46, Aiqun(Maria) Yu <[email protected]> wrote:
>
> On 9/28/2023 8:40 AM, Aiqun(Maria) Yu wrote:
> > On 9/27/2023 6:11 PM, Ard Biesheuvel wrote:
> >> On Wed, 27 Sept 2023 at 09:44, Aiqun(Maria) Yu
> >> <[email protected]> wrote:
> >>>
> >>> 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].
> >>>
> >>
> >> OK, I understand now, thanks.
> >>
> >> Would it help if we just always enabled frame pointers for this source
> >> file?
> The compiler(clang 17 here I am using) will still complain for "inline
> asm clobber list contains reserved registers" when ftrace enabled.
>
> More over, the local variable have fixed registers was align with
> current solution as well. we referenced other asm inlined function
> inside kprobe folder, like __kprobes functions inside actions-common.c
> files, it is also have similar solution to fixed register usage for
> local variables which will be used inside similar inline asm code.
I tested the approach below, and it seems to work fine both with
ftrace enabled and disabled. R7 is no longer selected by the compiler
as an inline asm argument, but is still in charge of the register
allocation.
With your approach, the compiler is still free to use R7, and we have
had issues in the past where the compiler does not honour the register
asm ("rX") assignment strictly.
Could you please explain how you produced the "inline asm clobber list
contains reserved registers" with this change?
--- a/arch/arm/probes/kprobes/Makefile
+++ b/arch/arm/probes/kprobes/Makefile
@@ -8,6 +8,7 @@ test-kprobes-objs := test-core.o
ifdef CONFIG_THUMB2_KERNEL
obj-$(CONFIG_KPROBES) += actions-thumb.o checkers-thumb.o
+CFLAGS_actions-thumb.o += -fno-omit-frame-pointer
test-kprobes-objs += test-thumb.o
else
obj-$(CONFIG_KPROBES) += actions-arm.o checkers-arm.o
On 9/30/2023 3:38 AM, Ard Biesheuvel wrote:
> On Thu, 28 Sept 2023 at 00:46, Aiqun(Maria) Yu <[email protected]> wrote:
>>
>> On 9/28/2023 8:40 AM, Aiqun(Maria) Yu wrote:
>>> On 9/27/2023 6:11 PM, Ard Biesheuvel wrote:
>>>> On Wed, 27 Sept 2023 at 09:44, Aiqun(Maria) Yu
>>>> <[email protected]> wrote:
>>>>>
>>>>> 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].
>>>>>
>>>>
>>>> OK, I understand now, thanks.
>>>>
>>>> Would it help if we just always enabled frame pointers for this source
>>>> file?
>> The compiler(clang 17 here I am using) will still complain for "inline
>> asm clobber list contains reserved registers" when ftrace enabled.
>>
>> More over, the local variable have fixed registers was align with
>> current solution as well. we referenced other asm inlined function
>> inside kprobe folder, like __kprobes functions inside actions-common.c
>> files, it is also have similar solution to fixed register usage for
>> local variables which will be used inside similar inline asm code.
>
> I tested the approach below, and it seems to work fine both with
> ftrace enabled and disabled. R7 is no longer selected by the compiler
> as an inline asm argument, but is still in charge of the register
> allocation.
Thx for the testing.
I have re-test with the clean change only, and it is working same as
your result.
R7 can be reserved when with "-fno-omit-frame-pointer" added as CFLAG
for this file.
The side effect of "-fno-omit-frame-pointer" is that, the additional
instruction is added for those functions in this file, for example:
with "-fno-omit-frame-pointer", instructions will be like:
push {r4, r5, r6, r7, lr}
add r7, sp, #12
stmdb sp!, {r8, r9, sl, fp}
with "-fomit-frame-pointer", instructions will be like:
stmdb sp!, {r4, r5, r6, r7, r8, r9, fp, lr}
My worry is about the unnecessary code size and performance impact.
While myself is not deny the solution of "-fno-omit-frame-pointer",
since in kernel there is other example use the similar solution.
>
> With your approach, the compiler is still free to use R7, and we have
> had issues in the past where the compiler does not honour the register
> asm ("rX") assignment strictly.
If the compiler does not honour the register strictly, and don't have a
specific rule, it is a problem in existing similar functionality as
well. There is the register asm ("rX") usage across different similar
functions.
example code like emulate_generic_r0_12_noflags() which is used similar
specify fn regist in file actions-common.c:
register void *rregs asm("r1") = regs;
register void *rfn asm("lr") = asi->insn_fn; /**if the compiler
choosing r0-r12, it will be a runtime error. **/
__asm__ __volatile__ (
"stmdb sp!, {%[regs], r11} \n\t"
"ldmia %[regs], {r0-r12} \n\t"
...
"blx %[fn] \n\t"
>
> Could you please explain how you produced the "inline asm clobber list
> contains reserved registers" with this change?
>
I was forget to remove "r7" in the clobber list and test along with this
change and that's why the compiler is complaining "inline asm clobber
list contains reserved registers".
Thx for the correction.
>
>
>
> --- a/arch/arm/probes/kprobes/Makefile
> +++ b/arch/arm/probes/kprobes/Makefile
> @@ -8,6 +8,7 @@ test-kprobes-objs := test-core.o
>
> ifdef CONFIG_THUMB2_KERNEL
> obj-$(CONFIG_KPROBES) += actions-thumb.o checkers-thumb.o
> +CFLAGS_actions-thumb.o += -fno-omit-frame-pointer
> test-kprobes-objs += test-thumb.o
> else
> obj-$(CONFIG_KPROBES) += actions-arm.o checkers-arm.o
Just come back from long vacation, so delayed response. Thx for the
consideration.
--
Thx and BRs,
Aiqun(Maria) Yu