queue-4.4/coredump-fix-race-condition-between-mmget_not_zero-get_task_mm-and-core-dumping.patch

   1 From 04f5866e41fb70690e28397487d8bd8eea7d712a Mon Sep 17 00:00:00 2001
   2 From: Andrea Arcangeli <aarcange@redhat.com>
   3 Date: Thu, 18 Apr 2019 17:50:52 -0700
   4 Subject: coredump: fix race condition between mmget_not_zero()/get_task_mm() and core dumping
   5
   6 From: Andrea Arcangeli <aarcange@redhat.com>
   7
   8 commit 04f5866e41fb70690e28397487d8bd8eea7d712a upstream.
   9
  10 The core dumping code has always run without holding the mmap_sem for
  11 writing, despite that is the only way to ensure that the entire vma
  12 layout will not change from under it.  Only using some signal
  13 serialization on the processes belonging to the mm is not nearly enough.
  14 This was pointed out earlier.  For example in Hugh's post from Jul 2017:
  15
  16   https://lkml.kernel.org/r/alpine.LSU.2.11.1707191716030.2055@eggly.anvils
  17
  18   "Not strictly relevant here, but a related note: I was very surprised
  19    to discover, only quite recently, how handle_mm_fault() may be called
  20    without down_read(mmap_sem) - when core dumping. That seems a
  21    misguided optimization to me, which would also be nice to correct"
  22
  23 In particular because the growsdown and growsup can move the
  24 vm_start/vm_end the various loops the core dump does around the vma will
  25 not be consistent if page faults can happen concurrently.
  26
  27 Pretty much all users calling mmget_not_zero()/get_task_mm() and then
  28 taking the mmap_sem had the potential to introduce unexpected side
  29 effects in the core dumping code.
  30
  31 Adding mmap_sem for writing around the ->core_dump invocation is a
  32 viable long term fix, but it requires removing all copy user and page
  33 faults and to replace them with get_dump_page() for all binary formats
  34 which is not suitable as a short term fix.
  35
  36 For the time being this solution manually covers the places that can
  37 confuse the core dump either by altering the vma layout or the vma flags
  38 while it runs.  Once ->core_dump runs under mmap_sem for writing the
  39 function mmget_still_valid() can be dropped.
  40
  41 Allowing mmap_sem protected sections to run in parallel with the
  42 coredump provides some minor parallelism advantage to the swapoff code
  43 (which seems to be safe enough by never mangling any vma field and can
  44 keep doing swapins in parallel to the core dumping) and to some other
  45 corner case.
  46
  47 In order to facilitate the backporting I added "Fixes: 86039bd3b4e6"
  48 however the side effect of this same race condition in /proc/pid/mem
  49 should be reproducible since before 2.6.12-rc2 so I couldn't add any
  50 other "Fixes:" because there's no hash beyond the git genesis commit.
  51
  52 Because find_extend_vma() is the only location outside of the process
  53 context that could modify the "mm" structures under mmap_sem for
  54 reading, by adding the mmget_still_valid() check to it, all other cases
  55 that take the mmap_sem for reading don't need the new check after
  56 mmget_not_zero()/get_task_mm().  The expand_stack() in page fault
  57 context also doesn't need the new check, because all tasks under core
  58 dumping are frozen.
  59
  60 Link: http://lkml.kernel.org/r/20190325224949.11068-1-aarcange@redhat.com
  61 Fixes: 86039bd3b4e6 ("userfaultfd: add new syscall to provide memory externalization")
  62 Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
  63 Reported-by: Jann Horn <jannh@google.com>
  64 Suggested-by: Oleg Nesterov <oleg@redhat.com>
  65 Acked-by: Peter Xu <peterx@redhat.com>
  66 Reviewed-by: Mike Rapoport <rppt@linux.ibm.com>
  67 Reviewed-by: Oleg Nesterov <oleg@redhat.com>
  68 Reviewed-by: Jann Horn <jannh@google.com>
  69 Acked-by: Jason Gunthorpe <jgg@mellanox.com>
  70 Acked-by: Michal Hocko <mhocko@suse.com>
  71 Cc: <stable@vger.kernel.org>
  72 Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
  73 Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
  74 Cc: Joel Fernandes (Google) <joel@joelfernandes.org>
  75 [mhocko@suse.com: stable 4.4 backport
  76  - drop infiniband part because of missing 5f9794dc94f59
  77  - drop userfaultfd_event_wait_completion hunk because of
  78    missing 9cd75c3cd4c3d]
  79  - handle binder_update_page_range because of missing 720c241924046
  80  - handle mlx5_ib_disassociate_ucontext - akaher@vmware.com
  81 ]
  82 Signed-off-by: Michal Hocko <mhocko@suse.com>
  83 Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
  84
  85 ---
  86  drivers/android/binder.c          |    6 ++++++
  87  drivers/infiniband/hw/mlx4/main.c |    3 +++
  88  fs/proc/task_mmu.c                |   18 ++++++++++++++++++
  89  fs/userfaultfd.c                  |    7 +++++++
  90  include/linux/mm.h                |   21 +++++++++++++++++++++
  91  mm/mmap.c                         |    7 ++++++-
  92  6 files changed, 61 insertions(+), 1 deletion(-)
  93
  94 --- a/drivers/android/binder.c
  95 +++ b/drivers/android/binder.c
  96 @@ -570,6 +570,12 @@ static int binder_update_page_range(stru
  97
  98         if (mm) {
  99                 down_write(&mm->mmap_sem);
 100 +               if (!mmget_still_valid(mm)) {
 101 +                       if (allocate == 0)
 102 +                               goto free_range;
 103 +                       goto err_no_vma;
 104 +               }
 105 +
 106                 vma = proc->vma;
 107                 if (vma && mm != proc->vma_vm_mm) {
 108                         pr_err("%d: vma mm and task mm mismatch\n",
 109 --- a/drivers/infiniband/hw/mlx4/main.c
 110 +++ b/drivers/infiniband/hw/mlx4/main.c
 111 @@ -1042,6 +1042,8 @@ static void mlx4_ib_disassociate_ucontex
 112          * mlx4_ib_vma_close().
 113          */
 114         down_write(&owning_mm->mmap_sem);
 115 +       if (!mmget_still_valid(owning_mm))
 116 +               goto skip_mm;
 117         for (i = 0; i < HW_BAR_COUNT; i++) {
 118                 vma = context->hw_bar_info[i].vma;
 119                 if (!vma)
 120 @@ -1061,6 +1063,7 @@ static void mlx4_ib_disassociate_ucontex
 121                 context->hw_bar_info[i].vma->vm_ops = NULL;
 122         }
 123
 124 +skip_mm:
 125         up_write(&owning_mm->mmap_sem);
 126         mmput(owning_mm);
 127         put_task_struct(owning_process);
 128 --- a/fs/proc/task_mmu.c
 129 +++ b/fs/proc/task_mmu.c
 130 @@ -947,6 +947,24 @@ static ssize_t clear_refs_write(struct f
 131                                         continue;
 132                                 up_read(&mm->mmap_sem);
 133                                 down_write(&mm->mmap_sem);
 134 +                               /*
 135 +                                * Avoid to modify vma->vm_flags
 136 +                                * without locked ops while the
 137 +                                * coredump reads the vm_flags.
 138 +                                */
 139 +                               if (!mmget_still_valid(mm)) {
 140 +                                       /*
 141 +                                        * Silently return "count"
 142 +                                        * like if get_task_mm()
 143 +                                        * failed. FIXME: should this
 144 +                                        * function have returned
 145 +                                        * -ESRCH if get_task_mm()
 146 +                                        * failed like if
 147 +                                        * get_proc_task() fails?
 148 +                                        */
 149 +                                       up_write(&mm->mmap_sem);
 150 +                                       goto out_mm;
 151 +                               }
 152                                 for (vma = mm->mmap; vma; vma = vma->vm_next) {
 153                                         vma->vm_flags &= ~VM_SOFTDIRTY;
 154                                         vma_set_page_prot(vma);
 155 --- a/fs/userfaultfd.c
 156 +++ b/fs/userfaultfd.c
 157 @@ -446,6 +446,8 @@ static int userfaultfd_release(struct in
 158          * taking the mmap_sem for writing.
 159          */
 160         down_write(&mm->mmap_sem);
 161 +       if (!mmget_still_valid(mm))
 162 +               goto skip_mm;
 163         prev = NULL;
 164         for (vma = mm->mmap; vma; vma = vma->vm_next) {
 165                 cond_resched();
 166 @@ -468,6 +470,7 @@ static int userfaultfd_release(struct in
 167                 vma->vm_flags = new_flags;
 168                 vma->vm_userfaultfd_ctx = NULL_VM_UFFD_CTX;
 169         }
 170 +skip_mm:
 171         up_write(&mm->mmap_sem);
 172         mmput(mm);
 173  wakeup:
 174 @@ -769,6 +772,8 @@ static int userfaultfd_register(struct u
 175                 goto out;
 176
 177         down_write(&mm->mmap_sem);
 178 +       if (!mmget_still_valid(mm))
 179 +               goto out_unlock;
 180         vma = find_vma_prev(mm, start, &prev);
 181         if (!vma)
 182                 goto out_unlock;
 183 @@ -914,6 +919,8 @@ static int userfaultfd_unregister(struct
 184                 goto out;
 185
 186         down_write(&mm->mmap_sem);
 187 +       if (!mmget_still_valid(mm))
 188 +               goto out_unlock;
 189         vma = find_vma_prev(mm, start, &prev);
 190         if (!vma)
 191                 goto out_unlock;
 192 --- a/include/linux/mm.h
 193 +++ b/include/linux/mm.h
 194 @@ -1098,6 +1098,27 @@ void zap_page_range(struct vm_area_struc
 195  void unmap_vmas(struct mmu_gather *tlb, struct vm_area_struct *start_vma,
 196                 unsigned long start, unsigned long end);
 197
 198 +/*
 199 + * This has to be called after a get_task_mm()/mmget_not_zero()
 200 + * followed by taking the mmap_sem for writing before modifying the
 201 + * vmas or anything the coredump pretends not to change from under it.
 202 + *
 203 + * NOTE: find_extend_vma() called from GUP context is the only place
 204 + * that can modify the "mm" (notably the vm_start/end) under mmap_sem
 205 + * for reading and outside the context of the process, so it is also
 206 + * the only case that holds the mmap_sem for reading that must call
 207 + * this function. Generally if the mmap_sem is hold for reading
 208 + * there's no need of this check after get_task_mm()/mmget_not_zero().
 209 + *
 210 + * This function can be obsoleted and the check can be removed, after
 211 + * the coredump code will hold the mmap_sem for writing before
 212 + * invoking the ->core_dump methods.
 213 + */
 214 +static inline bool mmget_still_valid(struct mm_struct *mm)
 215 +{
 216 +       return likely(!mm->core_state);
 217 +}
 218 +
 219  /**
 220   * mm_walk - callbacks for walk_page_range
 221   * @pmd_entry: if set, called for each non-empty PMD (3rd-level) entry
 222 --- a/mm/mmap.c
 223 +++ b/mm/mmap.c
 224 @@ -42,6 +42,7 @@
 225  #include <linux/memory.h>
 226  #include <linux/printk.h>
 227  #include <linux/userfaultfd_k.h>
 228 +#include <linux/mm.h>
 229
 230  #include <asm/uaccess.h>
 231  #include <asm/cacheflush.h>
 232 @@ -2398,7 +2399,8 @@ find_extend_vma(struct mm_struct *mm, un
 233         vma = find_vma_prev(mm, addr, &prev);
 234         if (vma && (vma->vm_start <= addr))
 235                 return vma;
 236 -       if (!prev || expand_stack(prev, addr))
 237 +       /* don't alter vm_end if the coredump is running */
 238 +       if (!prev || !mmget_still_valid(mm) || expand_stack(prev, addr))
 239                 return NULL;
 240         if (prev->vm_flags & VM_LOCKED)
 241                 populate_vma_page_range(prev, addr, prev->vm_end, NULL);
 242 @@ -2424,6 +2426,9 @@ find_extend_vma(struct mm_struct *mm, un
 243                 return vma;
 244         if (!(vma->vm_flags & VM_GROWSDOWN))
 245                 return NULL;
 246 +       /* don't alter vm_start if the coredump is running */
 247 +       if (!mmget_still_valid(mm))
 248 +               return NULL;
 249         start = vma->vm_start;
 250         if (expand_stack(vma, addr))
 251                 return NULL;