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] [day] [month] [year] [list]

Message-ID: <CAAhV-H6HTx3qgoUHKC0nB0LTqN9o9t=P3QuD7PumacaOe8pjEw@mail.gmail.com>
Date: Wed, 14 May 2025 22:51:08 +0800
From: Huacai Chen <chenhuacai@...nel.org>
To: Tiezhu Yang <yangtiezhu@...ngson.cn>
Cc: loongarch@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] LoongArch: uprobe: Remove redundant code about resume_era

On Wed, May 14, 2025 at 5:33 PM Tiezhu Yang <yangtiezhu@...ngson.cn> wrote:
>
> 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.
OK, I know.

Huacai

>
> Thanks,
> Tiezhu
>