On Tue, Aug 19, 2025 at 09:15:15PM +0200, Peter Zijlstra wrote: > On Sun, Jul 20, 2025 at 01:21:20PM +0200, Jiri Olsa wrote: > > > +static bool __is_optimized(uprobe_opcode_t *insn, unsigned long vaddr) > > +{ > > + struct __packed __arch_relative_insn { > > + u8 op; > > + s32 raddr; > > + } *call = (struct __arch_relative_insn *) insn; > > Not something you need to clean up now I suppose, but we could do with > unifying this thing. we have a bunch of instances around. ok, I noticed, will send patch for that > > > + > > + if (!is_call_insn(insn)) > > + return false; > > + return __in_uprobe_trampoline(vaddr + 5 + call->raddr); > > +} > > > +void arch_uprobe_optimize(struct arch_uprobe *auprobe, unsigned long vaddr) > > +{ > > + struct mm_struct *mm = current->mm; > > + uprobe_opcode_t insn[5]; > > + > > + /* > > + * Do not optimize if shadow stack is enabled, the return address hijack > > + * code in arch_uretprobe_hijack_return_addr updates wrong frame when > > + * the entry uprobe is optimized and the shadow stack crashes the app. > > + */ > > + if (shstk_is_enabled()) > > + return; > > Kernel should be able to fix up userspace shadow stack just fine. ok, will send follow up fix > > > + if (!should_optimize(auprobe)) > > + return; > > + > > + mmap_write_lock(mm); > > + > > + /* > > + * Check if some other thread already optimized the uprobe for us, > > + * if it's the case just go away silently. > > + */ > > + if (copy_from_vaddr(mm, vaddr, &insn, 5)) > > + goto unlock; > > + if (!is_swbp_insn((uprobe_opcode_t*) &insn)) > > + goto unlock; > > + > > + /* > > + * If we fail to optimize the uprobe we set the fail bit so the > > + * above should_optimize will fail from now on. > > + */ > > + if (__arch_uprobe_optimize(auprobe, mm, vaddr)) > > + set_bit(ARCH_UPROBE_FLAG_OPTIMIZE_FAIL, &auprobe->flags); > > + > > +unlock: > > + mmap_write_unlock(mm); > > +} > > + > > +static bool can_optimize(struct arch_uprobe *auprobe, unsigned long vaddr) > > +{ > > + if (memcmp(&auprobe->insn, x86_nops[5], 5)) > > + return false; > > + /* We can't do cross page atomic writes yet. */ > > + return PAGE_SIZE - (vaddr & ~PAGE_MASK) >= 5; > > +} > > This seems needlessly restrictive. Something like: > > is_nop5(const char *buf) > { > struct insn insn; > > ret = insn_decode_kernel(&insn, buf) > if (ret < 0) > return false; > > if (insn.length != 5) > return false; > > if (insn.opcode[0] != 0x0f || > insn.opcode[1] != 0x1f) > return false; > > return true; > } > > Should do I suppose. ok, looks good, should I respin with this, or is follow up ok? > Anyway, I think something like: > > f0 0f 1f 44 00 00 lock nopl 0(%eax, %eax, 1) > > is a valid NOP5 at +1 and will 'optimize' and result in: > > f0 e8 disp32 lock call disp32 > > which will #UD. > > But this is nearly unfixable. Just doing my best to find weirdo cases > ;-) nice, but I think if user puts not-optimized uprobe in the middle of the instruction like to lock-nop5 + 1 the app would crash as well thanks, jirka