uprobes: Kill uprobes_state->count

uprobes_state->count is only needed to avoid the slow path in
uprobe_pre_sstep_notifier(). It is also checked in uprobe_munmap()
but ironically its only goal to decrement this counter. However,
it is very broken. Just some examples:

- uprobe_mmap() can race with uprobe_unregister() and wrongly
  increment the counter if it hits the non-uprobe "int3". Note
  that install_breakpoint() checks ->consumers first and returns
  -EEXIST if it is NULL.

  "atomic_sub() if error" in uprobe_mmap() looks obviously wrong
  too.

- uprobe_munmap() can race with uprobe_register() and wrongly
  decrement the counter by the same reason.

- Suppose an appication tries to increase the mmapped area via
  sys_mremap(). vma_adjust() does uprobe_munmap(whole_vma) first,
  this can nullify the counter temporarily and race with another
  thread which can hit the bp, the application will be killed by
  SIGTRAP.

- Suppose an application mmaps 2 consecutive areas in the same file
  and one (or both) of these areas has uprobes. In the likely case
  mmap_region()->vma_merge() suceeds. Like above, this leads to
  uprobe_munmap/uprobe_mmap from vma_merge()->vma_adjust() but then
  mmap_region() does another uprobe_mmap(resulting_vma) and doubles
  the counter.

This patch only removes this counter and fixes the compile errors,
then we will try to cleanup the changed code and add something else
instead.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index efe4b3308c74cfc9f2140e07f876ae77e2e9182b..03ae547c1c3137cb3b05d9dd675354203b0c6184 100644 (file)
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -99,8 +99,8 @@ struct xol_area {
 
 struct uprobes_state {
        struct xol_area         *xol_area;
-       atomic_t                count;
 };
+
 extern int __weak set_swbp(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
 extern int __weak set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm,  unsigned long vaddr, bool verify);
 extern bool __weak is_swbp_insn(uprobe_opcode_t *insn);

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 0cefde276641783f66f470f8d4dac04d87cfcf81..6f1664d217dc7bd052593f9ddb394cce5ce84a9f 100644 (file)
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -678,18 +678,7 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
                uprobe->flags |= UPROBE_COPY_INSN;
        }
 
-       /*
-        * Ideally, should be updating the probe count after the breakpoint
-        * has been successfully inserted. However a thread could hit the
-        * breakpoint we just inserted even before the probe count is
-        * incremented. If this is the first breakpoint placed, breakpoint
-        * notifier might ignore uprobes and pass the trap to the thread.
-        * Hence increment before and decrement on failure.
-        */
-       atomic_inc(&mm->uprobes_state.count);
        ret = set_swbp(&uprobe->arch, mm, vaddr);
-       if (ret)
-               atomic_dec(&mm->uprobes_state.count);
 
        return ret;
 }
@@ -697,8 +686,7 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 static void
 remove_breakpoint(struct uprobe *uprobe, struct mm_struct *mm, unsigned long vaddr)
 {
-       if (!set_orig_insn(&uprobe->arch, mm, vaddr, true))
-               atomic_dec(&mm->uprobes_state.count);
+       set_orig_insn(&uprobe->arch, mm, vaddr, true);
 }
 
 /*
@@ -1051,13 +1039,6 @@ int uprobe_mmap(struct vm_area_struct *vma)
 
                                if (!is_swbp_at_addr(vma->vm_mm, vaddr))
                                        continue;
-
-                               /*
-                                * Unable to insert a breakpoint, but
-                                * breakpoint lies underneath. Increment the
-                                * probe count.
-                                */
-                               atomic_inc(&vma->vm_mm->uprobes_state.count);
                        }
 
                        if (!ret)
@@ -1068,9 +1049,6 @@ int uprobe_mmap(struct vm_area_struct *vma)
 
        mutex_unlock(uprobes_mmap_hash(inode));
 
-       if (ret)
-               atomic_sub(count, &vma->vm_mm->uprobes_state.count);
-
        return ret;
 }
 
@@ -1089,9 +1067,6 @@ void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned lon
        if (!atomic_read(&vma->vm_mm->mm_users)) /* called by mmput() ? */
                return;
 
-       if (!atomic_read(&vma->vm_mm->uprobes_state.count))
-               return;
-
        inode = vma->vm_file->f_mapping->host;
        if (!inode)
                return;
@@ -1100,13 +1075,6 @@ void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned lon
        build_probe_list(inode, vma, start, end, &tmp_list);
 
        list_for_each_entry_safe(uprobe, u, &tmp_list, pending_list) {
-               unsigned long vaddr = offset_to_vaddr(vma, uprobe->offset);
-               /*
-                * An unregister could have removed the probe before
-                * unmap. So check before we decrement the count.
-                */
-               if (is_swbp_at_addr(vma->vm_mm, vaddr) == 1)
-                       atomic_dec(&vma->vm_mm->uprobes_state.count);
                put_uprobe(uprobe);
        }
        mutex_unlock(uprobes_mmap_hash(inode));
@@ -1217,7 +1185,6 @@ void uprobe_clear_state(struct mm_struct *mm)
 void uprobe_reset_state(struct mm_struct *mm)
 {
        mm->uprobes_state.xol_area = NULL;
-       atomic_set(&mm->uprobes_state.count, 0);
 }
 
 /*
@@ -1585,8 +1552,7 @@ int uprobe_pre_sstep_notifier(struct pt_regs *regs)
 {
        struct uprobe_task *utask;
 
-       if (!current->mm || !atomic_read(&current->mm->uprobes_state.count))
-               /* task is currently not uprobed */
+       if (!current->mm)
                return 0;
 
        utask = current->utask;