ARM: uprobes need icache flush after xol write

After instruction write into xol area, on ARM V7 architecture code need to flush dcache and icache to sync them up for given set of addresses. Having just 'flush_dcache_page(page)' call is not enough - it is possible to have stale instruction sitting in icache for given xol area slot address.

Introduce arch_uprobe_flush_xol_access weak function that by default calls 'flush_dcache_page(page)' and on ARM define new one that calls flush_uprobe_xol_access function.

flush_uprobe_xol_access function shares/reuses implementation with/of flush_ptrace_access function and takes care of writing instruction to user land address space on given variety of different cache types on ARM CPUs. Because flush_uprobe_xol_access does not have vma around flush_ptrace_access was split into two parts. First that retrieves set of condition from vma and common that receives those conditions as flags.

Because on ARM cache flush function need kernel address through which instruction write happened, changed xol_get_insn_slot to explicitly map page and do memcpy rather than use copy_to_page helper. In this case xol_get_insn_slot knows kernel address and passes it to arch_uprobe_flush_xol_access function.

Signed-off-by Victor Kamensky <[email protected]>

---
  arch/arm/include/asm/cacheflush.h |  2 ++
  arch/arm/kernel/uprobes.c         |  6 ++++++
  arch/arm/mm/flush.c               | 41 +++++++++++++++++++++++++++++++++------
  include/linux/uprobes.h           |  3 +++
  kernel/events/uprobes.c           | 33 +++++++++++++++++++++++++------
  5 files changed, 73 insertions(+), 12 deletions(-)

arch/arm/include/asm/cacheflush.h +2 -0

index 8b8b616..e02712a 100644
@@ -487,4 +487,6 @@ int set_memory_rw(unsigned long addr, int numpages);
  int set_memory_x(unsigned long addr, int numpages);
  int set_memory_nx(unsigned long addr, int numpages);
 
+void flush_uprobe_xol_access(struct page *page, unsigned long uaddr,
+			     void *kaddr, unsigned long len);
  #endif

arch/arm/kernel/uprobes.c +6 -0

index f9bacee..a0339a6 100644
@@ -113,6 +113,12 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
  	return 0;
  }
 
+void arch_uprobe_flush_xol_access(struct page *page, unsigned long vaddr,
+				  void *kaddr, unsigned long len)
+{
+	flush_uprobe_xol_access(page, vaddr, kaddr, len);
+}
+
  int arch_uprobe_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
  {
  	struct uprobe_task *utask = current->utask;

arch/arm/mm/flush.c +25 -6

index 3387e60..69a0bd08 100644
@@ -104,17 +104,21 @@ void flush_cache_page(struct vm_area_struct *vma, unsigned long user_addr, unsig
  #define flush_icache_alias(pfn,vaddr,len)	do { } while (0)
  #endif
 
+#define FLAG_UA_IS_EXEC 1
+#define FLAG_UA_CORE_IN_MM 2
+#define FLAG_UA_BROADCAST 4
+
  static void flush_ptrace_access_other(void *args)
  {
  	__flush_icache_all();
  }
 
-static
-void flush_ptrace_access(struct vm_area_struct *vma, struct page *page,
-			 unsigned long uaddr, void *kaddr, unsigned long len)
+static inline
+void __flush_ptrace_access(struct page *page, unsigned long uaddr, void *kaddr,
+			   unsigned long len, unsigned int flags)
  {
  	if (cache_is_vivt()) {
-		if (cpumask_test_cpu(smp_processor_id(), mm_cpumask(vma->vm_mm))) {
+		if (flags & FLAG_UA_CORE_IN_MM) {
  			unsigned long addr = (unsigned long)kaddr;
  			__cpuc_coherent_kern_range(addr, addr + len);
  		}

@@ -128,18 +132,43 @@ void flush_ptrace_access(struct vm_area_struct *vma, struct page *page,
  	}
 
  	/* VIPT non-aliasing D-cache */
-	if (vma->vm_flags & VM_EXEC) {
+	if (flags & FLAG_UA_IS_EXEC) {
  		unsigned long addr = (unsigned long)kaddr;
  		if (icache_is_vipt_aliasing())
  			flush_icache_alias(page_to_pfn(page), uaddr, len);
  		else
  			__cpuc_coherent_kern_range(addr, addr + len);
-		if (cache_ops_need_broadcast())
+		if (flags & FLAG_UA_BROADCAST)
  			smp_call_function(flush_ptrace_access_other,
  					  NULL, 1);
  	}
  }
 
+static
+void flush_ptrace_access(struct vm_area_struct *vma, struct page *page,
+			 unsigned long uaddr, void *kaddr, unsigned long len)
+{
+	unsigned int flags = 0;
+	if (cpumask_test_cpu(smp_processor_id(), mm_cpumask(vma->vm_mm))) {
+		flags |= FLAG_UA_CORE_IN_MM;
+	}
+	if (vma->vm_flags & VM_EXEC) {
+		flags |= FLAG_UA_IS_EXEC;
+	}
+	if (cache_ops_need_broadcast()) {
+		flags |= FLAG_UA_BROADCAST;
+	}
+	__flush_ptrace_access(page, uaddr, kaddr, len, flags);

arch/arm/mm/flush.c +0 -0

  /*
   * Copy user data from/to a page which is mapped into a different
   * processes address space.  Really, we want to allow our "user

include/linux/uprobes.h +3 -0

index edff2b9..534e083 100644
@@ -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_flush_xol_access(struct page *page, unsigned long vaddr,
+						void *kaddr, unsigned long len);
  #else /* !CONFIG_UPROBES */
  struct uprobes_state {
  };

kernel/events/uprobes.c +20 -6

index 04709b6..b9142d5 100644
@@ -1287,6 +1287,7 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe)
  {
  	struct xol_area *area;
  	unsigned long xol_vaddr;
+	void *xol_page_kaddr;
 
  	area = get_xol_area();
  	if (!area)

@@ -1296,14 +1297,22 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe)
  	if (unlikely(!xol_vaddr))
  		return 0;
 
-	/* Initialize the slot */
-	copy_to_page(area->page, xol_vaddr,
-			&uprobe->arch.ixol, sizeof(uprobe->arch.ixol));
  	/*
-	 * We probably need flush_icache_user_range() but it needs vma.
-	 * This should work on supported architectures too.
+	 * We don't use copy_to_page here because we need kernel page
+	 * addr to invalidate caches correctly
  	 */
-	flush_dcache_page(area->page);
+	xol_page_kaddr = kmap_atomic(area->page);
+
+	/* Initialize the slot */
+	memcpy(xol_page_kaddr + (xol_vaddr & ~PAGE_MASK),
+	       &uprobe->arch.ixol,
+	       sizeof(uprobe->arch.ixol));
+
+	arch_uprobe_flush_xol_access(area->page, xol_vaddr,
+				     xol_page_kaddr + (xol_vaddr & ~PAGE_MASK),
+				     sizeof(uprobe->arch.ixol));
+
+	kunmap_atomic(xol_page_kaddr);
 
  	return xol_vaddr;
  }

@@ -1346,6 +1355,18 @@ static void xol_free_insn_slot(struct task_struct *tsk)
  	}
  }
 
+void __weak arch_uprobe_flush_xol_access(struct page *page, unsigned long vaddr,
+					 void *kaddr, unsigned long len)
+{
+	/*
+	 * We probably need flush_icache_user_range() but it needs vma.

Because on ARM cache flush function need kernel address

kernel/events/uprobes.c +16 -6

@@ -1287,6 +1287,7 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe)
 {
 	struct xol_area *area;
 	unsigned long xol_vaddr;
+	void *xol_page_kaddr;
 
 	area = get_xol_area();
 	if (!area)

@@ -1296,14 +1297,22 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe)
 	if (unlikely(!xol_vaddr))
 		return 0;
 
-	/* Initialize the slot */
-	copy_to_page(area->page, xol_vaddr,
-			&uprobe->arch.ixol, sizeof(uprobe->arch.ixol));
 	/*
-	 * We probably need flush_icache_user_range() but it needs vma.
-	 * This should work on supported architectures too.
+	 * We don't use copy_to_page here because we need kernel page
+	 * addr to invalidate caches correctly
 	 */
-	flush_dcache_page(area->page);
+	xol_page_kaddr = kmap_atomic(area->page);
+
+	/* Initialize the slot */
+	memcpy(xol_page_kaddr + (xol_vaddr & ~PAGE_MASK),
+	       &uprobe->arch.ixol,
+	       sizeof(uprobe->arch.ixol));
+
+	arch_uprobe_flush_xol_access(area->page, xol_vaddr,
+				     xol_page_kaddr + (xol_vaddr & ~PAGE_MASK),
+				     sizeof(uprobe->arch.ixol));
+
+	kunmap_atomic(xol_page_kaddr);
 
 	return xol_vaddr;
 }

@@ -1346,6 +1355,18 @@ static void xol_free_insn_slot(struct task_struct *tsk)
 	}
 }
 
+void __weak arch_uprobe_flush_xol_access(struct page *page, unsigned long vaddr,

Victor, sorry, I can't even read this thread now, will try to reply tomorrow... But at first glance,

Quoted 2 lines

Because on ARM cache flush function need kernel address

...

Quoted 44 lines

kernel/events/uprobes.c +16 -6

@@ -1287,6 +1287,7 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe)
 {
      struct xol_area *area;
      unsigned long xol_vaddr;
+     void *xol_page_kaddr;
 
      area = get_xol_area();
      if (!area)

@@ -1296,14 +1297,22 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe)
      if (unlikely(!xol_vaddr))
              return 0;
 
-     /* Initialize the slot */
-     copy_to_page(area->page, xol_vaddr,
-                     &uprobe->arch.ixol, sizeof(uprobe->arch.ixol));
      /*
-      * We probably need flush_icache_user_range() but it needs vma.
-      * This should work on supported architectures too.
+      * We don't use copy_to_page here because we need kernel page
+      * addr to invalidate caches correctly
       */
-     flush_dcache_page(area->page);
+     xol_page_kaddr = kmap_atomic(area->page);
+
+     /* Initialize the slot */
+     memcpy(xol_page_kaddr + (xol_vaddr & ~PAGE_MASK),
+            &uprobe->arch.ixol,
+            sizeof(uprobe->arch.ixol));
+
+     arch_uprobe_flush_xol_access(area->page, xol_vaddr,
+                                  xol_page_kaddr + (xol_vaddr & ~PAGE_MASK),
+                                  sizeof(uprobe->arch.ixol));
+
+     kunmap_atomic(xol_page_kaddr);
 
      return xol_vaddr;
 }

@@ -1346,6 +1355,18 @@ static void xol_free_insn_slot(struct task_struct *tsk)
      }
 }
 
+void __weak arch_uprobe_flush_xol_access(struct page *page, unsigned long vaddr,

kernel/events/uprobes.c static void xol_free_insn_slot(struct task_struct *tsk) · line 1368

Quoted 10 lines

kernel/events/uprobes.c +10 -0

+                                      void *kaddr, unsigned long 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);
+}

In this case I'd suggest to move this kmap/memcpy horror into arch_ helper. IOW, something like below.

If arm needs the kernel address we are writing to, let it do kmap/etc in its implementation.

Oleg.

kernel/events/uprobes.c +7 -9

@@ -1291,18 +1291,16 @@ static unsigned long xol_get_insn_slot(s
        if (unlikely(!xol_vaddr))
                return 0;
 
-       /* Initialize the slot */
-       copy_to_page(area->page, xol_vaddr,
-                       &uprobe->arch.ixol, sizeof(uprobe->arch.ixol));
-       /*
-        * We probably need flush_icache_user_range() but it needs vma.
-        * This should work on supported architectures too.
-        */
-       flush_dcache_page(area->page);
-
+       arch_uprobe_xxx(area->page, xol_vaddr, ...);
        return xol_vaddr;
 }
 
+void __weak arch_uprobe_xxx(page, xol_vaddr, ...)
+{
+       copy_to_page(page, xol_vaddr, ...)
+       flush_dcache_page(page);
+}
+
 /*
  * xol_free_insn_slot - If slot was earlier allocated by
  * @xol_get_insn_slot(), make the slot available for

Fair enough. Can do that, as well as address Dave's comment about checkpatch.

But before I do that let's reach some conclusion wrt latest Russell's remarks about copy_{to,from}_user_page usage in uprobes. It is bigger question covering more than ARM issue and possibly having bigger implication on uprobes design. I have some comments/thoughts about it, but since I am not uprobes maintainer, will wait for you to address it first.

Replace memcpy and dcache flush in generic uprobes with a call to copy_to_user_page(), which will do a proper flushing of kernel and user cache. Also modify the inmplementation of copy_to_user_page to assume a NULL vma pointer means the user icache corresponding to this right is stale and needs to be flushed. Note that this patch does not fix copy_to_user page for the sh, alpha, sparc, or mips architectures (which do not currently support uprobes).

Signed-off-by David A. Long <[email protected]>

From: David Long <[email protected]>
Date: Thu, 10 Apr 2014 23:45:31 -0400

Quoted 8 lines

Replace memcpy and dcache flush in generic uprobes with a call to copy_to_user_page(), which will do a proper flushing of kernel and user cache. Also modify the inmplementation of copy_to_user_page to assume a NULL vma pointer means the user icache corresponding to this right is stale and needs to be flushed. Note that this patch does not fix copy_to_user page for the sh, alpha, sparc, or mips architectures (which do not currently support uprobes).

Quoted 1 line

Signed-off-by David A. Long <[email protected]>

You really need to pass the proper VMA down to the call site rather than pass NULL, that's extremely ugly and totally unnecesary.

From 4a6a9043e0910041dd8842835a528cbdc39fad34 Mon Sep 17 00:00:00 2001

Quoted 3 lines · David Miller

You really need to pass the proper VMA down to the call site rather than pass NULL, that's extremely ugly and totally unnecesary.

Agreed that VMA is really needed.

index 04709b6..1ae4563 100644

kernel/events/uprobes.c +1 -0

@@ -117,6 +117,7 @@ struct xol_area {
      * the vma go away, and we must handle that reasonably gracefully.
      */
     unsigned long         vaddr;        /* Page(s) of instruction slots */
+    struct vm_area_struct    *vma;        /* VMA that holds above address */
 };
 
 /*

Quoted 11 lines · David Miller

From: David Long <[email protected]>
Date: Thu, 10 Apr 2014 23:45:31 -0400

Quoted 8 lines

Replace memcpy and dcache flush in generic uprobes with a call to copy_to_user_page(), which will do a proper flushing of kernel and user cache. Also modify the inmplementation of copy_to_user_page to assume a NULL vma pointer means the user icache corresponding to this right is stale and needs to be flushed. Note that this patch does not fix copy_to_user page for the sh, alpha, sparc, or mips architectures (which do not currently support uprobes).

Quoted 5 lines

Quoted 1 line

Signed-off-by David A. Long <[email protected]>

You really need to pass the proper VMA down to the call site rather than pass NULL, that's extremely ugly and totally unnecesary.

Agreed that VMA is really needed.

Here is variant that I tried while waiting for Oleg's response:

From 4a6a9043e0910041dd8842835a528cbdc39fad34 Mon Sep 17 00:00:00 2001
From: Victor Kamensky <[email protected]>
Date: Thu, 10 Apr 2014 17:06:39 -0700
Subject: [PATCH] uprobes: use copy_to_user_page function to copy instr to xol
area

Use copy_to_user_page function to copy instruction into xol area. copy_to_user_page function guarantee that all caches are correctly flushed during such write (including icache as well if needed).

Because copy_to_user_page needs vm_area_struct, vma field was added into struct xol_area. It holds cached vma value for xol_area.

Also using copy_to_user_page we make sure that we use the same code that ptrace write code uses.

Signed-off-by Victor Kamensky <[email protected]>

---
 kernel/events/uprobes.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

kernel/events/uprobes.c +3 -0

index 04709b6..1ae4563 100644
@@ -117,6 +117,7 @@ struct xol_area {
      * the vma go away, and we must handle that reasonably gracefully.
      */
     unsigned long         vaddr;        /* Page(s) of instruction slots */
+    struct vm_area_struct    *vma;        /* VMA that holds above address */
 };
 
 /*

@@ -1150,6 +1151,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);
+    area->vma = find_vma(mm, area->vaddr);
     if (ret)
         goto fail;

@@ -1287,6 +1289,7 @@ static unsigned long xol_get_insn_slot(struct
 uprobe *uprobe)
 {
     struct xol_area *area;
     unsigned long xol_vaddr;
+    void *xol_page_kaddr;
 
     area = get_xol_area();

     if (!area)

kernel/events/uprobes.c +5 -2

@@ -1297,8 +1300,11 @@ static unsigned long xol_get_insn_slot(struct
 uprobe *uprobe)
         return 0;
 
     /* Initialize the slot */
-    copy_to_page(area->page, xol_vaddr,
-            &uprobe->arch.ixol, sizeof(uprobe->arch.ixol));
+    xol_page_kaddr = kmap_atomic(area->page);
+    copy_to_user_page(area->vma, area->page, xol_vaddr,
+              xol_page_kaddr + (xol_vaddr & ~PAGE_MASK),
+              &uprobe->arch.ixol, sizeof(uprobe->arch.ixol));
+    kunmap_atomic(xol_page_kaddr);
     /*
      * We probably need flush_icache_user_range() but it needs vma.

* This should work on supported architectures too.

--
1.8.1.4

Now while waiting for Oleg's response I am trying to run some performance tests between different solution to understand situation better.

One of my concerns was ARM smp function call that could be reached from copy_to_user_page. But I have hard time finding CPU that really doing it ... It does not look such call will happen on relatively modern ARM CPUs (tried Arndale, and Pandaboard ES). So my issues with use
of copy_to_user_page are quite elevated.

Thanks,
Victor

From: David Long <[email protected]>
Date: Thu, 10 Apr 2014 23:45:31 -0400

Quoted 8 lines

Replace memcpy and dcache flush in generic uprobes with a call to copy_to_user_page(), which will do a proper flushing of kernel and user cache. Also modify the inmplementation of copy_to_user_page to assume a NULL vma pointer means the user icache corresponding to this right is stale and needs to be flushed. Note that this patch does not fix copy_to_user page for the sh, alpha, sparc, or mips architectures (which do not currently support uprobes).

Quoted 1 line

Signed-off-by David A. Long <[email protected]>

You really need to pass the proper VMA down to the call site rather than pass NULL, that's extremely ugly and totally unnecesary.

Can't we do _something_
like below?

Quoted 3 lines · Oleg Nesterov

Can't we do _something_
like below?

If not, I'd propose the patch below.

I can be easily wrong, but it seems that arch/arm can reimplement arch_uprobe_flush_xol_icache() and do flush_ptrace_access()-like code. It needs kaddr, but this is not a problem.

Btw. From arch/arm/include/asm/cacheflush.h

	#define flush_icache_user_range(vma,page,addr,len) \
		flush_dcache_page(page)

but it has no users?

And I am just curious, why arm's copy_to_user_page() disables premption before memcpy?

On Fri, Apr 11, 2014 at 05:22:07PM +0200, Oleg Nesterov wrote:

I wonder whether you've read this yet:

	http://lkml.iu.edu//hypermail/linux/kernel/1404.1/00725.html

where I proposed removing flush_icache_user_range() since it's not used on a great many architectures.

Quoted 2 lines

And I am just curious, why arm's copy_to_user_page() disables premption before memcpy?

flush_ptrace_access() needs to run on the CPU which ended up with the dirty cache line(s) to cope with those which do not have hardware broadcasting of cache maintanence operations.

This is why the hacks that you're doing are just that - they're hacks and are all broken in some way.

I fail to see what your problem is with keeping the vma around,

Let's not go inventing a whole new interface
solving the same problem.

kernel/events/uprobes.c +15 -0

+static void arch_uprobe_copy_ixol(struct xol_area *area, unsigned long vaddr,
+				 	struct arch_uprobe *auprobe)
+{
+#ifndef ARCH_UPROBE_XXX
+	copy_to_page(area->page, vaddr, &auprobe->ixol, sizeof(&auprobe->ixol));
+	/*
+	 * We probably need flush_icache_user_range() but it needs vma.
+	 * If this doesn't work define ARCH_UPROBE_XXX.
+	 */
+	flush_dcache_page(area->page);
+#else
+	struct mm_struct *mm = current->mm;
+	struct vm_area_struct *vma;
+
+	down_read(&mm->mmap_sem);

And perhaps the patch is not complete. "if (vma)" is not enough, a probed task can mmap something else at this vaddr.

copy_to_user_page() should only change the contents of area->page, so memcpy should be fine. But I am not sure that flush_icache_user_range() or flush_ptrace_access() is always safe on every arch if "struct page *page" doesn't match vma.

From: Oleg Nesterov <[email protected]>
Date: Fri, 11 Apr 2014 19:38:53 +0200

Quoted 7 lines

And perhaps the patch is not complete. "if (vma)" is not enough, a probed task can mmap something else at this vaddr.

copy_to_user_page() should only change the contents of area->page, so memcpy should be fine. But I am not sure that flush_icache_user_range() or flush_ptrace_access() is always safe on every arch if "struct page *page" doesn't match vma.

The architectures want the VMA for two reasons:

1) To get at the 'mm'. The 'mm' is absolutely essential so that we can look at the MM cpumask and therefore determine what cpus this address space has executed upon, and therefore what cpus need the flush broadcast to.

2) To determine if the VMA is executable, in order to avoid the I-cache flush if possible.

I think you can get at the 'mm' trivially in this uprobes path,

and we can just
as well assume that the VMA is executable since this thing is always writing instructions.

So we could create a __copy_to_user_page() that takes an 'mm' and a boolean 'executable' which uprobes could unconditionally set true, and copy_to_user_page() would then be implemented in terms of __copy_to_user_page().

kernel/events/uprobes.c +15 -0

+static void arch_uprobe_copy_ixol(struct xol_area *area, unsigned long vaddr,
+                                       struct arch_uprobe *auprobe)
+{
+#ifndef ARCH_UPROBE_XXX
+       copy_to_page(area->page, vaddr, &auprobe->ixol, sizeof(&auprobe->ixol));
+       /*
+        * We probably need flush_icache_user_range() but it needs vma.
+        * If this doesn't work define ARCH_UPROBE_XXX.
+        */
+       flush_dcache_page(area->page);
+#else
+       struct mm_struct *mm = current->mm;
+       struct vm_area_struct *vma;
+
+       down_read(&mm->mmap_sem);

So I really think we should just have a fixed "flush_icache_page(page,vaddr)" function. Maybe add a "len" argument, to allow architectures that have to loop over cachelines to do just a minimal loop.

It's not enough, we need to have the 'mm' so we can know what cpu's this address space has executed upon, and therefore what cpus need the broadcast flush.

See my other reply, we can just make a __copy_to_user_page() that takes 'mm' and a boolean 'executable' which uprobes can unconditionally pass as true.

And quite frankly, using the "vma->vm_flags" sounds potentially *incorrect* to me, since it really isn't about the vma. If you change a page through a non-executable vma, you'd want to flush the icache entry for that page mapped in a totally different vma. So I really get the feeling that passing in "vma" is actively *wrong*. The vma interface really makes little to no sense.

Hmm?

The vm_flags check is about "could it have gotten into the I-cache via this VMA".

Quoted 3 lines

The vm_flags check is about "could it have gotten into the I-cache via this VMA".

.. and that's obviously complete bullshit and wrong. Which is my point.

Now, it's possible that doing things right is just too much work for architectures that don't even matter, but dammit, it's still wrong. If you change a page, and it's executably mapped into some other vma, the icache is possibly stale there. The whole _point_ of our cache flushing is to make caches coherent, and anything that uses "vma" to do so is *wrong*.

So your argument makes no sense. You're just re-stating that "it's wrong", but you're re-stating it in a way that makes it sounds like it could be right.

The "this page has been mapped executably" approach, in contrast, is *correct*. It has a chance in hell of actually making caches coherent.

Since we do most of the cache flushing stuff during normal operations at the PTE modification point, perhaps a piece of page state could be used to handle this. We already use such a thing for D-cache alias flushing.

From: Linus Torvalds <[email protected]>
Date: Fri, 11 Apr 2014 10:50:31 -0700

Quoted 4 lines

So I really think we should just have a fixed "flush_icache_page(page,vaddr)" function. Maybe add a "len" argument, to allow architectures that have to loop over cachelines to do just a minimal loop.

It's not enough, we need to have the 'mm' so we can know what cpu's this address space has executed upon, and therefore what cpus need the broadcast flush.

See my other reply, we can just make a __copy_to_user_page() that takes 'mm' and a boolean 'executable' which uprobes can unconditionally pass as true.

On 11 April 2014 11:02, David Miller <[email protected]> wrote:
But in uprobes case xol slot where instruction write happened will be used only by current CPU. The way I read uprobes code other core when it hit the same uprobe address will use different xol slot. Xol slot size is cache line so it will not be moved around.

So as long as we
know for sure that while tasks performs single step on uprobe xol area instruction it won't be migrated to another core we don't need to do broadcast to any other cores.

Quoted 19 lines · Oleg Nesterov

kernel/events/uprobes.c +19 -0

+static void arch_uprobe_copy_ixol(struct xol_area *area, unsigned long vaddr,
+                                       struct arch_uprobe *auprobe)
+{
+#ifndef ARCH_UPROBE_XXX
+       copy_to_page(area->page, vaddr, &auprobe->ixol, sizeof(&auprobe->ixol));
+       /*
+        * We probably need flush_icache_user_range() but it needs vma.
+        * If this doesn't work define ARCH_UPROBE_XXX.
+        */
+       flush_dcache_page(area->page);
+#else
+       struct mm_struct *mm = current->mm;
+       struct vm_area_struct *vma;
+
+       down_read(&mm->mmap_sem);
+       vma = find_exact_vma(mm, area->vaddr, area->vaddr + PAGE_SIZE);
+       if (vma) {
+               void *kaddr = kmap_atomic(area->page);
+               copy_to_user_page(vma, area->page,

kernel/events/uprobes.c static unsigned long xol_take_insn_slot(struct xol_area *area) · line 1301

Quoted 6 lines

kernel/events/uprobes.c +6 -0

+                                       vaddr, kaddr + (vaddr & ~PAGE_MASK),
+                                       &auprobe->ixol, sizeof(&auprobe->ixol));
+               kunmap_atomic(kaddr);
+       }
+       up_read(&mm->mmap_sem);
+#endif

Yeah, no, this is wrong.

So I really think we should just have a fixed
"flush_icache_page(page,vaddr)" function.
...
Then the uprobe case can just do

    copy_to_page()
    flush_dcache_page()
    flush_icache_page()

Quoted 21 lines · Linus Torvalds

Quoted 19 lines · Oleg Nesterov

kernel/events/uprobes.c +19 -0

+static void arch_uprobe_copy_ixol(struct xol_area *area, unsigned long vaddr,
+                                       struct arch_uprobe *auprobe)
+{
+#ifndef ARCH_UPROBE_XXX
+       copy_to_page(area->page, vaddr, &auprobe->ixol, sizeof(&auprobe->ixol));
+       /*
+        * We probably need flush_icache_user_range() but it needs vma.
+        * If this doesn't work define ARCH_UPROBE_XXX.
+        */
+       flush_dcache_page(area->page);
+#else
+       struct mm_struct *mm = current->mm;
+       struct vm_area_struct *vma;
+
+       down_read(&mm->mmap_sem);
+       vma = find_exact_vma(mm, area->vaddr, area->vaddr + PAGE_SIZE);
+       if (vma) {
+               void *kaddr = kmap_atomic(area->page);
+               copy_to_user_page(vma, area->page,

Quoted 8 lines

kernel/events/uprobes.c static unsigned long xol_take_insn_slot(struct xol_area *area) · line 1301

Quoted 6 lines

kernel/events/uprobes.c +6 -0

+                                       vaddr, kaddr + (vaddr & ~PAGE_MASK),
+                                       &auprobe->ixol, sizeof(&auprobe->ixol));
+               kunmap_atomic(kaddr);
+       }
+       up_read(&mm->mmap_sem);
+#endif

Yeah, no, this is wrong.

Yesss, agreed.

Quoted 5 lines

So I really think we should just have a fixed
"flush_icache_page(page,vaddr)" function.
...
Then the uprobe case can just do

Quoted 3 lines

    copy_to_page()
    flush_dcache_page()
    flush_icache_page()

And I obviously like this idea because (iiuc) it more or less matches flush_icache_page_xxx() I tried to suggest.

But we need a short term solution for arm. And unless I misunderstood Russell (this is quite possible), arm needs to disable preemption around copy + flush.

Russel, so what do you think we can do for arm right now? Does the patch above (and subsequent discussion) answer the "why reinvent" question ?

Oleg.

Quoted 38 lines · Oleg Nesterov

Quoted 21 lines · Linus Torvalds

Quoted 19 lines · Oleg Nesterov

kernel/events/uprobes.c +19 -0

+static void arch_uprobe_copy_ixol(struct xol_area *area, unsigned long vaddr,
+                                       struct arch_uprobe *auprobe)
+{
+#ifndef ARCH_UPROBE_XXX
+       copy_to_page(area->page, vaddr, &auprobe->ixol, sizeof(&auprobe->ixol));
+       /*
+        * We probably need flush_icache_user_range() but it needs vma.
+        * If this doesn't work define ARCH_UPROBE_XXX.
+        */
+       flush_dcache_page(area->page);
+#else
+       struct mm_struct *mm = current->mm;
+       struct vm_area_struct *vma;
+
+       down_read(&mm->mmap_sem);
+       vma = find_exact_vma(mm, area->vaddr, area->vaddr + PAGE_SIZE);
+       if (vma) {
+               void *kaddr = kmap_atomic(area->page);
+               copy_to_user_page(vma, area->page,

Quoted 8 lines

kernel/events/uprobes.c static unsigned long xol_take_insn_slot(struct xol_area *area) · line 1301

Quoted 6 lines

kernel/events/uprobes.c +6 -0

+                                       vaddr, kaddr + (vaddr & ~PAGE_MASK),
+                                       &auprobe->ixol, sizeof(&auprobe->ixol));
+               kunmap_atomic(kaddr);
+       }
+       up_read(&mm->mmap_sem);
+#endif

Yeah, no, this is wrong.

Yesss, agreed.

Quoted 5 lines

So I really think we should just have a fixed
"flush_icache_page(page,vaddr)" function.
...
Then the uprobe case can just do

Quoted 7 lines

Quoted 3 lines

    copy_to_page()
    flush_dcache_page()
    flush_icache_page()

And I obviously like this idea because (iiuc) it more or less matches flush_icache_page_xxx() I tried to suggest.

Would not page granularity to be too expensive? Note you need to do that on each probe hit and you flushing whole data and instruction page every time. IMHO it will work correctly when you flush just few dcache/icache lines that correspond to xol slot that got modified. Note copy_to_user_page takes len that describes size of area that has to be flushed. Given that we are flushing xol area page at this case; and nothing except one xol slot is any interest for current task, and if CPU can flush one dcache and icache page as quickly as it can flush range, my remark may not matter.

I personally would prefer if we could have function like copy_to_user_page but without requirement to pass vma to it.

Thanks,
Victor

Quoted 9 lines

But we need a short term solution for arm. And unless I misunderstood Russell (this is quite possible), arm needs to disable preemption around copy + flush.

Russel, so what do you think we can do for arm right now? Does the patch above (and subsequent discussion) answer the "why reinvent" question ?

Oleg.

Oleg's fix - Oleg's fix proposed on [1]. Basically it uses copy_to_user_page and it does dynamic look-up of xol area vma every tracing

my arm specific fix - this one was proposed on as [2].

x) fix that I did for ARM that shares ARM code with copy_to_user_page but does not need vma performs best.

When I looked at current uprobes single step out line code and compared it with code that was in the past (utrace times) I noticed main essential difference how xol slots are handled: Currently for each hit uprobes code allocate xol slot and needs dcache/icache flush. But in the past xol slot was attached/cached to uprobe entry

Quoted 7 lines · Oleg Nesterov

Quoted 6 lines · Linus Torvalds

So I really think we should just have a fixed
"flush_icache_page(page,vaddr)" function.
...
Then the uprobe case can just do

Quoted 7 lines

Quoted 3 lines

    copy_to_page()
    flush_dcache_page()
    flush_icache_page()

And I obviously like this idea because (iiuc) it more or less matches flush_icache_page_xxx() I tried to suggest.

Would not page granularity to be too expensive? Note you need to do that on each probe hit and you flushing whole data and instruction page every time. IMHO it will work correctly when you flush just few dcache/icache lines that correspond to xol slot that got modified. Note copy_to_user_page takes len that describes size of area that has to be flushed. Given that we are flushing xol area page at this case; and nothing except one xol slot is any interest for current task, and if CPU can flush one dcache and icache page as quickly as it can flush range, my remark may not matter.

I personally would prefer if we could have function like copy_to_user_page but without requirement to pass vma to it.

Quoted 17 lines · Victor Kamensky

Quoted 7 lines · Oleg Nesterov

Quoted 6 lines · Linus Torvalds

So I really think we should just have a fixed
"flush_icache_page(page,vaddr)" function.
...
Then the uprobe case can just do

Quoted 7 lines

Quoted 3 lines

    copy_to_page()
    flush_dcache_page()
    flush_icache_page()

And I obviously like this idea because (iiuc) it more or less matches flush_icache_page_xxx() I tried to suggest.

Quoted 8 lines

Would not page granularity to be too expensive? Note you need to do that on each probe hit and you flushing whole data and instruction page every time. IMHO it will work correctly when you flush just few dcache/icache lines that correspond to xol slot that got modified. Note copy_to_user_page takes len that describes size of area that has to be flushed. Given that we are flushing xol area page at this case; and nothing except one xol slot is any interest for current task, and if CPU can flush one dcache and icache page as quickly as it can flush range, my remark may not matter.

We can add "vaddr, len" to the argument list.

Quoted 2 lines

I personally would prefer if we could have function like copy_to_user_page but without requirement to pass vma to it.

I won't argue, but you need to convince maintainers.

And to remind, we need something simple/nonintrusive for arm right now. Again, I won't argue if we turn copy_to_page() + flush_dcache_page() into __weak arch_uprobe_copy_ixol(), and add the necessary hacks into arm's implementatiion. This is up to you and Russel.

But. Please do not add copy_to_user_page() into copy_to_page() (as your patch did). This is certainly not what uprobe_write_opcode() wants, we do not want or need "flush" in this case. The same for __create_xol_area().

Note also that currently copy_to_user_page() is always called under mmap_sem. I do not know if arm actually needs ->mmap_sem, but if you propose it as a generic solution we should probably take this lock.

Also. Even if we have copy_to_user_page_no_vma() or change copy_to_user_page() to accept vma => NULL, I am not sure this will work fine on arm when the probed application unmaps xol_area and mmaps something else at the same vaddr. I mean, in this case we do not care if the application crashes, but please verify that something really bad can't happen.

Let me repeat just in case, I know nothing about arm/. I can't even understand how, say, flush_pfn_alias() works, and how it should work if 2 threads call it at the same time with the same vaddr (or CACHE_COLOUR(vaddr)).

Oleg.

But. Please do not add copy_to_user_page() into copy_to_page() (as your patch did). This is certainly not what uprobe_write_opcode() wants, we do not want or need "flush" in this case. The same for __create_xol_area().

Quoted 5 lines

But. Please do not add copy_to_user_page() into copy_to_page() (as your patch did). This is certainly not what uprobe_write_opcode() wants, we do not want or need "flush" in this case. The same for __create_xol_area().

It looked me like a call to a new __copy_to_user_page(current->mm, ...) in xol_get_insn_slot() would be in line with David Miller's suggestion and would cure the problem on ARM (and hopefuly be more philosophically correct for all architectures):

From: David Long <[email protected]>
Date: Tue, 15 Apr 2014 13:19:27 -0400

Quoted 10 lines · Oleg Nesterov

Quoted 5 lines

But. Please do not add copy_to_user_page() into copy_to_page() (as your patch did). This is certainly not what uprobe_write_opcode() wants, we do not want or need "flush" in this case. The same for __create_xol_area().

It looked me like a call to a new __copy_to_user_page(current->mm, ...) in xol_get_insn_slot() would be in line with David Miller's suggestion and would cure the problem on ARM (and hopefuly be more philosophically correct for all architectures):

It occurs to me that because of the specific environment in which this executes we can make the interface take advantage of the invariants at this call site:

1) We are copying into userspace and thus current->mm

2) We are only storing an instruction or two

Furthermore, we can do the stores using something akin to put_user(), directly store them into userspace.

This avoids completely any D-cache aliasing issues. The kernel stores to the same address, and therefore the same cache lines, as userspace would.

We do not want to write to, say, page cache if the probed application mmaps a file at the same vaddr.

From: Oleg Nesterov <[email protected]>
Date: Tue, 15 Apr 2014 19:49:00 +0200

Quoted 2 lines

We do not want to write to, say, page cache if the probed application mmaps a file at the same vaddr.

If user has write access to that page... not our problem.

Quoted 7 lines

From: Oleg Nesterov <[email protected]>
Date: Tue, 15 Apr 2014 19:49:00 +0200

Quoted 2 lines

We do not want to write to, say, page cache if the probed application mmaps a file at the same vaddr.

Quoted 1 line

If user has write access to that page... not our problem.

Well, I am not sure.

Yes, this won't allow you to write the exploit. But if this actually happens because an application is buggy, this can lead to really hard-to-debug problems.

From: Oleg Nesterov <[email protected]>
Date: Tue, 15 Apr 2014 20:07:00 +0200

Quoted 10 lines · David Miller

Quoted 8 lines

From: Oleg Nesterov <[email protected]>
Date: Tue, 15 Apr 2014 19:49:00 +0200

Quoted 2 lines

We do not want to write to, say, page cache if the probed application mmaps a file at the same vaddr.

If user has write access to that page... not our problem.

Quoted 5 lines

Well, I am not sure.

Yes, this won't allow you to write the exploit. But if this actually happens because an application is buggy, this can lead to really hard-to-debug problems.

And this is different from a stray userland scribble onto such a page how?

This is all so much simpler if you don't insist on doing all of this hand-holding for the user, that's not the kernel's job.

Quoted 5 lines · Oleg Nesterov

But. Please do not add copy_to_user_page() into copy_to_page() (as your patch did). This is certainly not what uprobe_write_opcode() wants, we do not want or need "flush" in this case. The same for __create_xol_area().

It looked me like a call to a new __copy_to_user_page(current->mm, ...) in xol_get_insn_slot() would be in line with David Miller's suggestion and would cure the problem on ARM (and hopefuly be more philosophically correct for all architectures):

kernel/events/uprobes.c +0 -2

@@ -1297,13 +1298,11 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe)
                return 0;
 
        /* Initialize the slot */
-       copy_to_page(area->page, xol_vaddr,
-                       &uprobe->arch.ixol, sizeof(uprobe->arch.ixol));

Finally, let me repeat, you should verify that this __copy_to_user_page(page, uaddr, kaddr) will not something bad if uaddr is not mmapped, or its mapping do not match area->page.

From: Oleg Nesterov <[email protected]>
Date: Tue, 15 Apr 2014 19:43:30 +0200

Quoted 3 lines

Finally, let me repeat, you should verify that this __copy_to_user_page(page, uaddr, kaddr) will not something bad if uaddr is not mmapped, or its mapping do not match area->page.

Just directly access userspace with the usual exception mechanism we use for copy_to_user(), put_user(), et al. and if it faults you'll get -EFAULT and handle it.

This also avoids the D-cache aliasing issues entirely as I explained in my other reply.

Quoted 12 lines

From: Oleg Nesterov <[email protected]>
Date: Tue, 15 Apr 2014 19:43:30 +0200

Quoted 3 lines

Finally, let me repeat, you should verify that this __copy_to_user_page(page, uaddr, kaddr) will not something bad if uaddr is not mmapped, or its mapping do not match area->page.

Just directly access userspace with the usual exception mechanism we use for copy_to_user(), put_user(), et al. and if it faults you'll get -EFAULT and handle it.

Quoted 2 lines

This also avoids the D-cache aliasing issues entirely as I explained in my other reply.

Yes, yes, this is obvious.

But I have no idea what else we should do to take care of icache.

Russell, if for example userland stores into the instruction stream to patch an instruction, what needs to be done on ARM?

However, unlike your "most braindead sparc" CPU, the i-cache doesn't snoop d-cache stores at all.

However, this is something that we already deal with since self-modifying code has to work, so (from userspace) we have a syscall that is used to sort that out. Internally in the kernel, this translates to:

ret = flush_cache_user_range(start, end)

This deals with whatever the CPU requires to be able to correctly execute code which has been previously written in the range - and only actions on the currently mapped userspace.

Russell, if for example userland stores into the instruction stream to patch an instruction, what needs to be done on ARM?

From: David Miller <[email protected]>
Date: Tue, 15 Apr 2014 14:30:54 -0400 (EDT)

Quoted 2 lines

Russell, if for example userland stores into the instruction stream to patch an instruction, what needs to be done on ARM?

Looking around I suspect something like:

	mcrne	p15, 0, INSN_ADDR, c7, c5, 1

after the instruction stores will do it.

Quoted 11 lines

From: David Miller <[email protected]>
Date: Tue, 15 Apr 2014 14:30:54 -0400 (EDT)

Quoted 2 lines

Russell, if for example userland stores into the instruction stream to patch an instruction, what needs to be done on ARM?

Looking around I suspect something like:

	mcrne	p15, 0, INSN_ADDR, c7, c5, 1

after the instruction stores will do it.

It does still need to be pushed out of the D-cache first though. So, for ARMv7 for example:

	str	NEW_INSN, [INSN_ADDR]		@ store new instruction
	mcr	p15, 0, INSN_ADDR, c7, c11, 1	@ clean d line
	mcr	p15, 0, INSN_ADDR, c7, c5, 1	@ flush i line

would do it. We of course need the user access marking on that (so that any fault doesn't oops the kernel) - not only for the store, but also the following two instructions which could fault (and oops unless they're marked with a fixup) if someone were to munmap() this page in another thread. All those fixups can just do the "lets return -EFAULT" from the operation.

Quoted 11 lines · David Long

Quoted 5 lines · Oleg Nesterov

But. Please do not add copy_to_user_page() into copy_to_page() (as your patch did). This is certainly not what uprobe_write_opcode() wants, we do not want or need "flush" in this case. The same for __create_xol_area().

It looked me like a call to a new __copy_to_user_page(current->mm, ...) in xol_get_insn_slot() would be in line with David Miller's suggestion and would cure the problem on ARM (and hopefuly be more philosophically correct for all architectures):

David, let me repeat. I am not going to argue even if I obviously like the Linus's suggestion more.

I only argued with the suggestions to follow the __access_remote_vm() logic.

Quoted 3 lines

kernel/events/uprobes.c +0 -0

@@ -1297,13 +1298,11 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe)
                 return 0;
 

kernel/events/uprobes.c static unsigned long xol_get_insn_slot(struct uprobe *uprobe) · line 1305

Quoted 13 lines

kernel/events/uprobes.c +5 -7

         /* Initialize the slot */
-       copy_to_page(area->page, xol_vaddr,
-                       &uprobe->arch.ixol, sizeof(uprobe->arch.ixol));
-       /*
-        * We probably need flush_icache_user_range() but it needs vma.
-        * This should work on supported architectures too.
-        */
-       flush_dcache_page(area->page);
+       kaddr = kmap_atomic(area->page);
+       __copy_to_user_page(current->mm, area->page, xol_vaddr,
+                       kaddr + (xol_vaddr & ~PAGE_MASK),
+                       &uprobe->arch.ixol, sizeof(uprobe->arch.ixol), true);
+       kunmap_atomic(kaddr);

This all is fine, but you need to introduce __copy_to_user_page() first.

And you need to do this for every arch. And you need to verify that it does not need mmap_sem. It seems that at least alpha needs this lock. And as Russel pointed out (again, again, I could easily misunderstood him) you need preempt_disable() around memcpy + flush, so down_read(mmap_sem) should probably go into __copy_to_user_page(). And I am not sure this helper needs "struct mm_struct *mm" argument, but this is minor.

Finally, let me repeat, you should verify that this __copy_to_user_page(page, uaddr, kaddr) will not something bad if uaddr is not mmapped, or its mapping do not match area->page.

Good luck ;)

Oleg.

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).

From: David Long <[email protected]>
Date: Tue, 15 Apr 2014 15:39:18 -0400

Quoted 4 lines

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.

From e325a1a1bdddc7bd95301f0031d868bc69ddcddb Mon Sep 17 00:00:00 2001

Quoted 12 lines

From: David Long <[email protected]>
Date: Tue, 15 Apr 2014 15:39:18 -0400

Quoted 4 lines

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.

Quoted 2 lines

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.

(quoted diff) +5 -0

+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);

From: Victor Kamensky <[email protected]>
Date: Tue, 15 Apr 2014 18:42:39 -0700

Quoted 12 lines

(quoted diff) +12 -0

+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.
+     */

Review

Quoted 3 lines

(quoted diff) +3 -0

+    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.

But don't you think that writable and executable uprobes xol page is show stopper for this approach?

| Given that we've already solved that problem, wouldn't it be a good idea
| if the tracing code would stop trying to reinvent broken solutions to
| problems we have already solved?

I fail to see what your problem is with keeping the vma around, and using that infrastructure. If it needs optimisation for uprobes, then let's optimise it. Let's not go inventing a whole new interface solving the same problem.

And I am just curious, why arm's copy_to_user_page() disables premption before memcpy?

Quoted 2 lines · Oleg Nesterov

And I am just curious, why arm's copy_to_user_page() disables premption before memcpy?

Without looking, I suspect its because the VIVT caches, they need to get shot down on every context switch.

Quoted 6 lines · Peter Zijlstra

Quoted 2 lines · Oleg Nesterov

And I am just curious, why arm's copy_to_user_page() disables premption before memcpy?

Without looking, I suspect its because the VIVT caches, they need to get shot down on every context switch.

So... let's think about that for a moment... if we have a preemption event, then that's a context switch, which means...

No, this is obviously not the reason, because such an event on a fully VIVT system would result in the caches being flushed, meaning that we wouldn't need to do anything if we could be predicably preempted at that point.

First of all: I do not pretend I really understand the problems with icache/etc coherency and how flush_icache_range() actually works on alpha. Help.

For those who were not cc'ed. A probed task has a special "xol" vma, it is used to execute the probed insn out of line.

Initial implementation was x86 only, so we simply copied the probed insn into this vma and everything was fine, flush_icache_range() is nop on x86.

Then we added flush_dcache_page() for powerpc,

        // this is just kmap() + memcpy()
        copy_to_page(area->page, xol_vaddr,
                        &uprobe->arch.ixol, sizeof(uprobe->arch.ixol));

        /*
         * We probably need flush_icache_user_range() but it needs vma.
         * This should work on supported architectures too.
         */
        flush_dcache_page(area->page);

but this doesn't work on arm. So we need another fix.

Quoted 12 lines · David Miller

From: David Long <[email protected]>
Date: Thu, 10 Apr 2014 23:45:31 -0400

Quoted 8 lines

Replace memcpy and dcache flush in generic uprobes with a call to copy_to_user_page(), which will do a proper flushing of kernel and user cache. Also modify the inmplementation of copy_to_user_page to assume a NULL vma pointer means the user icache corresponding to this right is stale and needs to be flushed. Note that this patch does not fix copy_to_user page for the sh, alpha, sparc, or mips architectures (which do not currently support uprobes).

Quoted 5 lines

Quoted 1 line

Signed-off-by David A. Long <[email protected]>

You really need to pass the proper VMA down to the call site rather than pass NULL, that's extremely ugly and totally unnecesary.

I agree that we should not change copy_to_user_page(), but I am not sure the code above should use copy_to_user_page().

Because the code above really differs in my opinion from the only user of copy_to_user_page(), __access_remote_vm().

1. First of all, we do not know vma.

OK, we can down_read(mmap_sem) and do find_vma() of course. This is a bit unfortunate, especially because the architectures we currently support do not need this.

/*
* We keep the vma's vm_start rather than a pointer to the vma
* itself. The probed process or a naughty kernel module could make
* the vma go away, and we must handle that reasonably gracefully. */
unsigned long vaddr; /* Page(s) of instruction slots */

But,

2. The problem is, it would be very nice to remove this vma, or at least hide it somehow from find_vma/etc. This is the special mapping we do not want to expose to user-space.

In fact I even have the patches which remove this vma, but they do not work with compat tasks unfortunately.

3. Unlike __access_remote_vm() we always use current->mm, and the memory range changed by the code above can only be used by "current" thread.

So (perhaps) flush_icache_user_range() can even treat this case as "mm->mm_users) <= 1" and avoid ipi_flush_icache_page (just in case, of course I do not know if this is actually possible).

So can't we do something else? Lets forget about arch/arm for the moment, suppose that we want to support uprobes on alpha. Can't we do _something_ like below?

And perhap we can do something like this on arm? it can do kmap(page) / page_address(page) itself.

Oleg.

arch/alpha/kernel/smp.c +23 -8

@@ -751,15 +751,9 @@ ipi_flush_icache_page(void *x)
                flush_tlb_other(mm);
 }
 
-void
-flush_icache_user_range(struct vm_area_struct *vma, struct page *page,
-                       unsigned long addr, int len)
-{
-       struct mm_struct *mm = vma->vm_mm;
-
-       if ((vma->vm_flags & VM_EXEC) == 0)
-               return;
 
+void __flush_icache_page_xxx(struct mm_struct *mm, struct page *page) // addr, len ?
+{
        preempt_disable();
 
        if (mm == current->active_mm) {

@@ -783,3 +777,24 @@ flush_icache_user_range(struct vm_area_s
 
        preempt_enable();
 }
+
+void flush_icache_page_xxx(struct mm_struct *mm, struct page *page)
+{
+       struct mm_struct *mm = current->mm;
+
+       down_read(&mm->mmap_sem);
+       __flush_icache_page_xxx(mm, page);
+       up_read(&mm->mmap_sem);
+}
+
+void
+flush_icache_user_range(struct vm_area_struct *vma, struct page *page,
+                       unsigned long addr, int len)
+{
+       struct mm_struct *mm = vma->vm_mm;
+
+       if ((vma->vm_flags & VM_EXEC) == 0)
+               return;
+
+       __flush_icache_page_xxx(mm, page);
+}

Quoted 6 lines · Oleg Nesterov

1. First of all, we do not know vma.

OK, we can down_read(mmap_sem) and do find_vma() of course. This is a bit unfortunate, especially because the architectures we currently support do not need this.

Question, maybe silly one but I don't know the answer, why can't we just do look up for vma once and cache results in place like xol_area (along with xol_area.vaddr) and use it all the time. IOW under what circumstances vma for xol area can disappear change so we need constant lookup for it? Comment in xol_area

Quoted 6 lines

/*
* We keep the vma's vm_start rather than a pointer to the vma
* itself. The probed process or a naughty kernel module could make
* the vma go away, and we must handle that reasonably gracefully. */
unsigned long vaddr; /* Page(s) of instruction slots */

alludes to some of those conditions, but I don't quite follow. Should not we go after "probed process" ability to unmap xol area. xol area is like vdso,

mmap call should ignore
those..

Quoted 6 lines

2. The problem is, it would be very nice to remove this vma, or at least hide it somehow from find_vma/etc. This is the special mapping we do not want to expose to user-space.

In fact I even have the patches which remove this vma, but they do not work with compat tasks unfortunately.

I don't think it is right route. Xol area as well as vdso, signal page, etc should be visible as regular VMAs. There are other aspects of the system where they needed. Like core file collection - I would like to have xol area present in my core file if traced process crashed.

/porc/<pid>/maps - I would like to see my memory layout through this interface and I would like to see xol area there because I can see xol area addresses by some other means.

Appeal of copy_to_user_page approach is that I don't need to know how to handle sync up of icache and dcache on that architecture,

it is
already done by someone else when they programmed basic ptrace breakpoint write behavior.

First of all: I do not pretend I really understand the problems with icache/etc coherency and how flush_icache_range() actually works on alpha. Help.