On Tue, Apr 22, 2025 at 05:04:03PM -0700, Andrii Nakryiko wrote: SNIP
> > arch/x86/include/asm/uprobes.h | 7 + > > arch/x86/kernel/uprobes.c | 281 ++++++++++++++++++++++++++++++++- > > include/linux/uprobes.h | 6 +- > > kernel/events/uprobes.c | 15 +- > > 4 files changed, 301 insertions(+), 8 deletions(-) > > > > just minor nits, LGTM > > Acked-by: Andrii Nakryiko <[email protected]> > > > +int set_swbp(struct arch_uprobe *auprobe, struct vm_area_struct *vma, > > + unsigned long vaddr) > > +{ > > + if (should_optimize(auprobe)) { > > + bool optimized = false; > > + int err; > > + > > + /* > > + * We could race with another thread that already optimized > > the probe, > > + * so let's not overwrite it with int3 again in this case. > > + */ > > + err = is_optimized(vma->vm_mm, vaddr, &optimized); > > + if (err || optimized) > > + return err; > > IMO, this is a bit too clever, I'd go with plain > > if (err) > return err; > if (optimized) > return 0; /* we are done */ > ok > (and mirror set_orig_insn() structure, consistently) set_orig_insn does that already, right? > > > > + } > > + return uprobe_write_opcode(vma, vaddr, UPROBE_SWBP_INSN, true); > > +} > > + > > +int set_orig_insn(struct arch_uprobe *auprobe, struct vm_area_struct *vma, > > + unsigned long vaddr) > > +{ > > + if (test_bit(ARCH_UPROBE_FLAG_CAN_OPTIMIZE, &auprobe->flags)) { > > + struct mm_struct *mm = vma->vm_mm; > > + bool optimized = false; > > + int err; > > + > > + err = is_optimized(mm, vaddr, &optimized); > > + if (err) > > + return err; > > + if (optimized) > > + WARN_ON_ONCE(swbp_unoptimize(auprobe, vma, vaddr)); > > + } > > + return uprobe_write_opcode(vma, vaddr, *(uprobe_opcode_t > > *)&auprobe->insn, false); > > +} > > + > > +static int __arch_uprobe_optimize(struct mm_struct *mm, unsigned long > > vaddr) > > +{ > > + struct uprobe_trampoline *tramp; > > + struct vm_area_struct *vma; > > + int err = 0; > > + > > + vma = find_vma(mm, vaddr); > > + if (!vma) > > + return -1; > > this is EPERM, will be confusing to debug... why not -EINVAL? > > > + tramp = uprobe_trampoline_get(vaddr); > > + if (!tramp) > > + return -1; > > ditto so the error value is not exposed to user space in this case, we try to optimize in the first hit with: handle_swbp() { arch_uprobe_optimize() { if (__arch_uprobe_optimize(mm, vaddr)) set_bit(ARCH_UPROBE_FLAG_OPTIMIZE_FAIL, &auprobe->flags); } } and set ARCH_UPROBE_FLAG_OPTIMIZE_FAIL flags bit in case of error, plus there's WARN for swbp_optimize which should pass in case we get that far thanks, jirka > > > + err = swbp_optimize(vma, vaddr, tramp->vaddr); > > + if (WARN_ON_ONCE(err)) > > + uprobe_trampoline_put(tramp); > > + return err; > > +} > > + > > [...]
