On Thu, Nov 14, 2024 at 03:44:14PM -0800, Andrii Nakryiko wrote:
> On Tue, Nov 5, 2024 at 5:35 AM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
> >
> > Adding interface to add special mapping for user space page that will be
> > used as place holder for uprobe trampoline in following changes.
> >
> > The get_tramp_area(vaddr) function either finds 'callable' page or create
> > new one. The 'callable' means it's reachable by call instruction (from
> > vaddr argument) and is decided by each arch via new arch_uprobe_is_callable
> > function.
> >
> > The put_tramp_area function either drops refcount or destroys the special
> > mapping and all the maps are clean up when the process goes down.
> >
> > Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> > ---
> > include/linux/uprobes.h | 12 ++++
> > kernel/events/uprobes.c | 141 ++++++++++++++++++++++++++++++++++++++++
> > kernel/fork.c | 2 +
> > 3 files changed, 155 insertions(+)
> >
> > diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> > index be306028ed59..222d8e82cee2 100644
> > --- a/include/linux/uprobes.h
> > +++ b/include/linux/uprobes.h
> > @@ -172,6 +172,15 @@ struct xol_area;
> >
> > struct uprobes_state {
> > struct xol_area *xol_area;
> > + struct hlist_head tramp_head;
> > + struct mutex tramp_mutex;
> > +};
> > +
> > +struct tramp_area {
> > + unsigned long vaddr;
> > + struct page *page;
> > + struct hlist_node node;
> > + refcount_t ref;
>
> nit: any reason we are unnecessarily trying to save 4 bytes on
> refcount (and we don't actually, due to padding)
hum, I'm not sure what you mean.. what's the alternative?
>
> > };
> >
> > extern void __init uprobes_init(void);
> > @@ -219,6 +228,9 @@ extern int uprobe_verify_opcode(struct page *page, unsigned long vaddr, uprobe_o
> > extern int arch_uprobe_verify_opcode(struct page *page, unsigned long vaddr,
> > uprobe_opcode_t *new_opcode, void *data);
> > extern bool arch_uprobe_is_register(uprobe_opcode_t *insn, int len, void *data);
> > +struct tramp_area *get_tramp_area(unsigned long vaddr);
>
> uprobe_get_tramp_area() to make it clear this is uprobe specific,
> given this is exposed function?
>
> and add that extern like we do for other functions?
ok
>
> > +void put_tramp_area(struct tramp_area *area);
>
> uprobe_put_tramp_area() ?
ok
>
> > +bool arch_uprobe_is_callable(unsigned long vtramp, unsigned long vaddr);
> > #else /* !CONFIG_UPROBES */
> > struct uprobes_state {
> > };
> > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > index 944d9df1f081..a44305c559a4 100644
> > --- a/kernel/events/uprobes.c
> > +++ b/kernel/events/uprobes.c
> > @@ -616,6 +616,145 @@ set_orig_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long v
> > (uprobe_opcode_t *)&auprobe->insn, UPROBE_SWBP_INSN_SIZE, NULL);
> > }
> >
> > +bool __weak arch_uprobe_is_callable(unsigned long vtramp, unsigned long vaddr)
> > +{
> > + return false;
> > +}
> > +
> > +static unsigned long find_nearest_page(unsigned long vaddr)
> > +{
> > + struct mm_struct *mm = current->mm;
> > + struct vm_area_struct *vma, *prev;
> > + VMA_ITERATOR(vmi, mm, 0);
> > +
> > + prev = vma_next(&vmi);
> > + vma = vma_next(&vmi);
> > + while (vma) {
> > + if (vma->vm_start - prev->vm_end >= PAGE_SIZE &&
> > + arch_uprobe_is_callable(prev->vm_end, vaddr))
> > + return prev->vm_end;
>
> shouldn't we try both `prev->vm_end` and `vma->vm_start - PAGE_SIZE`
> as two possible places
right, we should do that
SNIP
> > +static struct tramp_area *create_tramp_area(unsigned long vaddr)
> > +{
> > + struct mm_struct *mm = current->mm;
> > + struct vm_area_struct *vma;
> > + struct tramp_area *area;
> > +
> > + vaddr = find_nearest_page(vaddr);
> > + if (!vaddr)
> > + return NULL;
> > +
> > + area = kzalloc(sizeof(*area), GFP_KERNEL);
> > + if (unlikely(!area))
> > + return NULL;
> > +
> > + area->page = alloc_page(GFP_HIGHUSER);
> > + if (!area->page)
> > + goto free_area;
> > +
> > + refcount_set(&area->ref, 1);
> > + area->vaddr = vaddr;
> > +
> > + vma = _install_special_mapping(mm, area->vaddr, PAGE_SIZE,
> > + VM_READ|VM_EXEC|VM_MAYEXEC|VM_MAYREAD|VM_DONTCOPY|VM_IO,
> > + &tramp_mapping);
> > + if (!IS_ERR(vma))
> > + return area;
>
> please keep a pattern, it's less surprising that way
>
> if (IS_ERR(vma))
> goto free_page;
>
> return area;
>
> free_page:
ok
SNIP
> > +static void destroy_tramp_area(struct tramp_area *area)
> > +{
> > + hlist_del(&area->node);
> > + put_page(area->page);
> > + kfree(area);
> > +}
> > +
> > +void put_tramp_area(struct tramp_area *area)
> > +{
> > + struct mm_struct *mm = current->mm;
> > + struct uprobes_state *state = &mm->uprobes_state;
> > +
> > + if (area == NULL)
>
> nit: !area
ok
thanks,
jirka
