On Fri, Dec 13, 2024 at 02:48:00PM +0100, Thomas Weißschuh wrote: SNIP > > +static int tramp_mremap(const struct vm_special_mapping *sm, struct vm_area_struct *new_vma) > > +{ > > + return -EPERM; > > +} > > + > > +static struct vm_special_mapping tramp_mapping = { > > + .name = "[uprobes-trampoline]", > > + .mremap = tramp_mremap, > > +}; > > + > > +SYSCALL_DEFINE0(uprobe) > > +{ > > + struct pt_regs *regs = task_pt_regs(current); > > + struct vm_area_struct *vma; > > + unsigned long bp_vaddr; > > + int err; > > + > > + err = copy_from_user(&bp_vaddr, (void __user *)regs->sp + 3*8, sizeof(bp_vaddr)); > > A #define for the magic values would be nice. the 3*8 is to skip 3 values pushed on stack and get the return ip value, I'd prefer to keep 3*8 but it's definitely missing explaining comment above, wdyt? > > > + if (err) { > > + force_sig(SIGILL); > > + return -1; > > + } > > + > > + /* Allow execution only from uprobe trampolines. */ > > + vma = vma_lookup(current->mm, regs->ip); > > + if (!vma || vma->vm_private_data != (void *) &tramp_mapping) { > > vma_is_special_mapping() did not know about this function, thanks > > > + force_sig(SIGILL); > > + return -1; > > + } > > + > > + handle_syscall_uprobe(regs, bp_vaddr - 5); > > + return 0; > > +} > > + > > +asm ( > > + ".pushsection .rodata\n" > > + ".global uprobe_trampoline_entry\n" > > + "uprobe_trampoline_entry:\n" > > + "endbr64\n" > > + "push %rcx\n" > > + "push %r11\n" > > + "push %rax\n" > > + "movq $" __stringify(__NR_uprobe) ", %rax\n" > > + "syscall\n" > > + "pop %rax\n" > > + "pop %r11\n" > > + "pop %rcx\n" > > + "ret\n" > > + ".global uprobe_trampoline_end\n" > > + "uprobe_trampoline_end:\n" > > + ".popsection\n" > > +); > > + > > +extern __visible u8 uprobe_trampoline_entry[]; > > +extern __visible u8 uprobe_trampoline_end[]; > > + > > +const struct vm_special_mapping *arch_uprobe_trampoline_mapping(void) > > +{ > > + struct pt_regs *regs = task_pt_regs(current); > > + > > + return user_64bit_mode(regs) ? &tramp_mapping : NULL; > > +} > > + > > +static int __init arch_uprobes_init(void) > > +{ > > + unsigned long size = uprobe_trampoline_end - uprobe_trampoline_entry; > > + static struct page *pages[2]; > > + struct page *page; > > + > > + page = alloc_page(GFP_HIGHUSER); > > That page could be in static memory, removing the need for the explicit > allocation. It could also be __ro_after_init. > Then tramp_mapping itself can be const. hum, how would that look like? I think that to get proper page object you have to call alloc_page or some other page alloc family function.. what do I miss? > > Also this seems to waste the page on 32bit kernels. it's inside CONFIG_X86_64 ifdef > > > + if (!page) > > + return -ENOMEM; > > + pages[0] = page; > > + tramp_mapping.pages = (struct page **) &pages; > > tramp_mapping.pages = pages; ? I think the compiler will cry about *pages[2] vs **pages types mismatch, but I'll double check that thanks, jirka > > > + arch_uprobe_copy_ixol(page, 0, uprobe_trampoline_entry, size); > > + return 0; > > +} > > + > > +late_initcall(arch_uprobes_init); > > + > > /* > > * If arch_uprobe->insn doesn't use rip-relative addressing, return > > * immediately. Otherwise, rewrite the instruction so that it accesses > > [..]