| Message ID | CAA3XUr0+u9YCDhRx5xZgKC3GH1Y0Xiwh2731QWkWn0rGu7T2Gg@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
From: Victor Kamensky <victor.kamensky@linaro.org> Date: Tue, 15 Apr 2014 18:42:39 -0700 > On 15 April 2014 12:53, David Miller <davem@davemloft.net> wrote: >> From: David Long <dave.long@linaro.org> >> Date: Tue, 15 Apr 2014 15:39:18 -0400 >> >>> Yes, I have that coded for 14 out of 24 architectures (the easy >>> ones). The remaining ones need more work. Some may prove problematic. >>> The whole approach could yet prove impractical. More recent suggested >>> approaches may be better too (or Linus's relatively simple change). >> >> BTW, another reason perhaps to write directly to userspace and have >> a uprobes_*() specific interface is that you'll only need to do >> 3 architectures, the ones that support uprobes. >> >> That way, this is just part of what every arch needs to implement in >> order to support uprobes. > > David, Russell, I hope I correctly understood your idea of writing > directly into user space. Please check patch below. Just FYI, your mail client corrupted the patch, at a minimum chopping up long lines.
From: Victor Kamensky <victor.kamensky@linaro.org> Date: Tue, 15 Apr 2014 18:42:39 -0700 > +void __weak arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr, > + void *src, unsigned long len) > +{ > + /* Initialize the slot */ > + copy_to_page(page, vaddr, src, len); > + > + /* > + * We probably need flush_icache_user_range() but it needs vma. > + * This should work on most of architectures by default. If > + * architecture needs to do something different it can define > + * its own version of the function. > + */ > + flush_dcache_page(page); > +} > + I would say that, if anything, flush_dcache_page() is unnecessary if you just copy straight to userspace. The default implementation should be copy_to_user(), and that's what every architecture can use if it needs no I-cache flushing.
On 15 April 2014 19:24, David Miller <davem@davemloft.net> wrote: > From: Victor Kamensky <victor.kamensky@linaro.org> > Date: Tue, 15 Apr 2014 18:42:39 -0700 > >> +void __weak arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr, >> + void *src, unsigned long len) >> +{ >> + /* Initialize the slot */ >> + copy_to_page(page, vaddr, src, len); >> + >> + /* >> + * We probably need flush_icache_user_range() but it needs vma. >> + * This should work on most of architectures by default. If >> + * architecture needs to do something different it can define >> + * its own version of the function. >> + */ >> + flush_dcache_page(page); >> +} >> + > > I would say that, if anything, flush_dcache_page() is unnecessary > if you just copy straight to userspace. > The default implementation should be copy_to_user(), and that's what > every architecture can use if it needs no I-cache flushing. OK, got it. I tried not to touch existing cases (x86 and ppc), but yes it would benefit here as well. But don't you think that writable and executable uprobes xol page is show stopper for this approach? Thanks, Victor
From: Victor Kamensky <victor.kamensky@linaro.org> Date: Tue, 15 Apr 2014 20:06:57 -0700 > But don't you think that writable and executable uprobes xol page is > show stopper for this approach? Not really.
diff --git a/arch/arm/kernel/uprobes.c b/arch/arm/kernel/uprobes.c index f9bacee..4836e54 100644 --- a/arch/arm/kernel/uprobes.c +++ b/arch/arm/kernel/uprobes.c @@ -113,6 +113,14 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, return 0; } +void arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr, + void *src, unsigned long len) +{ + if (!__copy_to_user((void *) vaddr, src, len)) + flush_cache_user_range(vaddr, len); +} + + int arch_uprobe_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs) { struct uprobe_task *utask = current->utask; diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h index edff2b9..c52f827 100644 --- a/include/linux/uprobes.h +++ b/include/linux/uprobes.h @@ -32,6 +32,7 @@ struct vm_area_struct; struct mm_struct; struct inode; struct notifier_block; +struct page; #define UPROBE_HANDLER_REMOVE 1 #define UPROBE_HANDLER_MASK 1 @@ -127,6 +128,8 @@ extern int arch_uprobe_exception_notify(struct notifier_block *self, unsigned l extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs); extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr, struct pt_regs *regs); extern bool __weak arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs); +extern void __weak arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr, + void *src, unsigned long len); #else /* !CONFIG_UPROBES */ struct uprobes_state { }; diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c index 04709b6..9e22002 100644 --- a/kernel/events/uprobes.c +++ b/kernel/events/uprobes.c @@ -1149,7 +1149,7 @@ static int xol_add_vma(struct mm_struct *mm, struct xol_area *area) } ret = install_special_mapping(mm, area->vaddr, PAGE_SIZE, - VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO, &area->page); + VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO|VM_WRITE, &area->page); if (ret) goto fail;