linux-kernel - Re: [PATCH 2/2] LoongArch: uprobe: Remove redundant code about resume

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <bbd7e5ad-c353-6916-cf9e-8f3aa3fefda9@loongson.cn>
Date: Wed, 14 May 2025 17:32:56 +0800
From: Tiezhu Yang <yangtiezhu@...ngson.cn>
To: Huacai Chen <chenhuacai@...nel.org>
Cc: loongarch@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] LoongArch: uprobe: Remove redundant code about
 resume_era

On 05/13/2025 11:13 PM, Huacai Chen wrote:
> Hi, Tiezhu,
>
> On Tue, May 13, 2025 at 5:21 PM Tiezhu Yang <yangtiezhu@...ngson.cn> wrote:
>>
>> arch_uprobe_skip_sstep() returns true if instruction was emulated,
>> that is to say, there is no need to single step for the emulated
>> instructions, it will point to the destination address directly
>> after the exception, so the resume_era related code is redundant,
>> just remove them.
>>
>> Fixes: 19bc6cb64092 ("LoongArch: Add uprobes support")
>> Signed-off-by: Tiezhu Yang <yangtiezhu@...ngson.cn>
>> ---
>>  arch/loongarch/include/asm/uprobes.h | 1 -
>>  arch/loongarch/kernel/uprobes.c      | 7 +------
>>  2 files changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/arch/loongarch/include/asm/uprobes.h b/arch/loongarch/include/asm/uprobes.h
>> index 99a0d198927f..025fc3f0a102 100644
>> --- a/arch/loongarch/include/asm/uprobes.h
>> +++ b/arch/loongarch/include/asm/uprobes.h
>> @@ -15,7 +15,6 @@ typedef u32 uprobe_opcode_t;
>>  #define UPROBE_XOLBP_INSN      __emit_break(BRK_UPROBE_XOLBP)
>>
>>  struct arch_uprobe {
>> -       unsigned long   resume_era;
>>         u32     insn[2];
>>         u32     ixol[2];
>>         bool    simulate;
>> diff --git a/arch/loongarch/kernel/uprobes.c b/arch/loongarch/kernel/uprobes.c
>> index 0ab9d8d631c4..6022eb0f71db 100644
>> --- a/arch/loongarch/kernel/uprobes.c
>> +++ b/arch/loongarch/kernel/uprobes.c
>> @@ -52,11 +52,7 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
>>
>>         WARN_ON_ONCE(current->thread.trap_nr != UPROBE_TRAP_NR);
>>         current->thread.trap_nr = utask->autask.saved_trap_nr;
>> -
>> -       if (auprobe->simulate)
>> -               instruction_pointer_set(regs, auprobe->resume_era);
>> -       else
>> -               instruction_pointer_set(regs, utask->vaddr + LOONGARCH_INSN_SIZE);
>> +       instruction_pointer_set(regs, utask->vaddr + LOONGARCH_INSN_SIZE);
> This seems wrong. If in the simulate case, regs->csr_era has already
> pointed to the correct destination address, then here we should only
> handle the non-simulate case.

What is wrong with this code? AFAICT, the code is right.

Here are the call chains in the generic code of uprobe:

handle_swbp()
   arch_uprobe_skip_sstep()
   pre_ssout()
     arch_uprobe_pre_xol()
handle_singlestep()
   arch_uprobe_post_xol()

arch_uprobe_post_xol() only handles the instruction that is not emulated
because if arch_uprobe_skip_sstep() returns true, arch_uprobe_post_xol()
will not be called, it will be called only if arch_uprobe_skip_sstep()
returns false.

Thanks,
Tiezhu