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
