On Sat, Nov 16, 2024 at 1:44 PM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
>
> 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?
atomic64_t ?
>
> >
> > > };
> > >
> > > 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
