Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Message-ID: <22c91d69-328b-8221-f0fa-d6e1663ce074@quicinc.com>
Date:   Thu, 28 Sep 2023 08:45:46 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.0
Subject: Re: [PATCH] ARM: kprobes: Explicitly assign register for local
 variables
From:   "Aiqun(Maria) Yu" <quic_aiquny@quicinc.com>
To:     Ard Biesheuvel <ardb@kernel.org>
CC:     <linux@armlinux.org.uk>, <mhiramat@kernel.org>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-kernel@vger.kernel.org>, <kernel@quicinc.com>,
        <quic_lijuang@quicinc.com>, <linux-arm-msm@vger.kernel.org>
References: <20230927055954.3755-1-quic_aiquny@quicinc.com>
 <CAMj1kXEh=yQ12v1Ht6=-vgXVe8VCQTPdUD8AowkKcpyDXh_0mg@mail.gmail.com>
 <af59ec04-aa68-5a22-0f3f-f4872cefeb46@quicinc.com>
 <CAMj1kXEUTOpATv_jbimGktSXBBweyfVNPpqWWJaJNp0U-zvg+w@mail.gmail.com>
 <29bf8f4c-44b8-ab22-aa66-3e3040012ee5@quicinc.com>
In-Reply-To: <29bf8f4c-44b8-ab22-aa66-3e3040012ee5@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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 
>> <quic_aiquny@quicinc.com> wrote:
>>>
>>> On 9/27/2023 5:26 PM, Ard Biesheuvel wrote:
>>>> Hello Maria,
>>>>
>>>> On Wed, 27 Sept 2023 at 06:00, Maria Yu <quic_aiquny@quicinc.com> 
>>>> 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