Re: [PATCH v9 08/24] mm: Protect VMA modifications using VMA sequence count

[Laurent Dufour]

On 27/03/2018 23:57, David Rientjes wrote:
> On Tue, 13 Mar 2018, Laurent Dufour wrote:
> 
>> diff --git a/mm/mmap.c b/mm/mmap.c
>> index 5898255d0aeb..d6533cb85213 100644
>> --- a/mm/mmap.c
>> +++ b/mm/mmap.c
>> @@ -847,17 +847,18 @@ int __vma_adjust(struct vm_area_struct *vma, unsigned 
>> long start,
>>  }
>>  
>>  if (start != vma->vm_start) {
>> -vma->vm_start = start;
>> +WRITE_ONCE(vma->vm_start, start);
>>  start_changed = true;
>>  }
>>  if (end != vma->vm_end) {
>> -vma->vm_end = end;
>> +WRITE_ONCE(vma->vm_end, end);
>>  end_changed = true;
>>  }
>> -vma->vm_pgoff = pgoff;
>> +WRITE_ONCE(vma->vm_pgoff, pgoff);
>>  if (adjust_next) {
>> -next->vm_start += adjust_next << PAGE_SHIFT;
>> -next->vm_pgoff += adjust_next;
>> +WRITE_ONCE(next->vm_start,
>> +   next->vm_start + (adjust_next << PAGE_SHIFT));
>> +WRITE_ONCE(next->vm_pgoff, next->vm_pgoff + adjust_next);
>>  }
>>  
>>  if (root) {
>> @@ -1781,6 +1782,7 @@ unsigned long mmap_region(struct file *file, unsigned 
>> long addr,
>>  out:
>>  perf_event_mmap(vma);
>>  
>> +vm_write_begin(vma);
>>  vm_stat_account(mm, vm_flags, len >> PAGE_SHIFT);
>>  if (vm_flags & VM_LOCKED) {
>>  if (!((vm_flags & VM_SPECIAL) || is_vm_hugetlb_page(vma) ||
>> @@ -1803,6 +1805,7 @@ unsigned long mmap_region(struct file *file, unsigned 
>> long addr,
>>  vma->vm_flags |= VM_SOFTDIRTY;
>>  
>>  vma_set_page_prot(vma);
>> +vm_write_end(vma);
>>  
>>  return addr;
>>  
> 
> Shouldn't this also protect vma->vm_flags?

Nice catch !
I just found that too while reviewing the entire patch to answer your previous
email.

> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1796,7 +1796,8 @@ unsigned long mmap_region(struct file *file, unsigned 
> long addr,
>   vma == get_gate_vma(current->mm)))
>   mm->locked_vm += (len >> PAGE_SHIFT);
>   else
> - vma->vm_flags &= VM_LOCKED_CLEAR_MASK;
> + WRITE_ONCE(vma->vm_flags,
> +vma->vm_flags & VM_LOCKED_CLEAR_MASK);
>   }
> 
>   if (file)
> @@ -1809,7 +1810,7 @@ unsigned long mmap_region(struct file *file, unsigned 
> long addr,
>* then new mapped in-place (which must be aimed as
>* a completely new data area).
>*/
> - vma->vm_flags |= VM_SOFTDIRTY;
> + WRITE_ONCE(vma->vm_flags, vma->vm_flags | VM_SOFTDIRTY);
> 
>   vma_set_page_prot(vma);
>   vm_write_end(vma);
> 

Re: [PATCH v9 08/24] mm: Protect VMA modifications using VMA sequence count

[Laurent Dufour]

On 27/03/2018 23:45, David Rientjes wrote:
> On Tue, 13 Mar 2018, Laurent Dufour wrote:
> 
>> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
>> index 65ae54659833..a2d9c87b7b0b 100644
>> --- a/fs/proc/task_mmu.c
>> +++ b/fs/proc/task_mmu.c
>> @@ -1136,8 +1136,11 @@ static ssize_t clear_refs_write(struct file *file, 
>> const char __user *buf,
>>  goto out_mm;
>>  }
>>  for (vma = mm->mmap; vma; vma = vma->vm_next) {
>> -vma->vm_flags &= ~VM_SOFTDIRTY;
>> +vm_write_begin(vma);
>> +WRITE_ONCE(vma->vm_flags,
>> +   vma->vm_flags & 
>> ~VM_SOFTDIRTY);
>>  vma_set_page_prot(vma);
>> +vm_write_end(vma);
>>  }
>>  downgrade_write(>mmap_sem);
>>  break;
>> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
>> index cec550c8468f..b8212ba17695 100644
>> --- a/fs/userfaultfd.c
>> +++ b/fs/userfaultfd.c
>> @@ -659,8 +659,11 @@ int dup_userfaultfd(struct vm_area_struct *vma, struct 
>> list_head *fcs)
>>  
>>  octx = vma->vm_userfaultfd_ctx.ctx;
>>  if (!octx || !(octx->features & UFFD_FEATURE_EVENT_FORK)) {
>> +vm_write_begin(vma);
>>  vma->vm_userfaultfd_ctx = NULL_VM_UFFD_CTX;
>> -vma->vm_flags &= ~(VM_UFFD_WP | VM_UFFD_MISSING);
>> +WRITE_ONCE(vma->vm_flags,
>> +   vma->vm_flags & ~(VM_UFFD_WP | VM_UFFD_MISSING));
>> +vm_write_end(vma);
>>  return 0;
>>  }
>>  
> 
> In several locations in this patch vm_write_begin(vma) -> 
> vm_write_end(vma) is nesting things other than vma->vm_flags, 
> vma->vm_policy, etc.  I think it's better to do vm_write_end(vma) as soon 
> as the members that the seqcount protects are modified.  In other words, 
> this isn't offering protection for vma->vm_userfaultfd_ctx.  There are 
> several examples of this in the patch.

That's true in this particular case, and I could change that to not include the
change to vm_userfaultfd_ctx.
This being said, I don't think this will have a major impact, but I'll make a
close review on this patch to be sure there is too large protected part of code.

>> @@ -885,8 +888,10 @@ static int userfaultfd_release(struct inode *inode, 
>> struct file *file)
>>  vma = prev;
>>  else
>>  prev = vma;
>> -vma->vm_flags = new_flags;
>> +vm_write_begin(vma);
>> +WRITE_ONCE(vma->vm_flags, new_flags);
>>  vma->vm_userfaultfd_ctx = NULL_VM_UFFD_CTX;
>> +vm_write_end(vma);
>>  }
>>  up_write(>mmap_sem);
>>  mmput(mm);
>> @@ -1434,8 +1439,10 @@ static int userfaultfd_register(struct 
>> userfaultfd_ctx *ctx,
>>   * the next vma was merged into the current one and
>>   * the current one has not been updated yet.
>>   */
>> -vma->vm_flags = new_flags;
>> +vm_write_begin(vma);
>> +WRITE_ONCE(vma->vm_flags, new_flags);
>>  vma->vm_userfaultfd_ctx.ctx = ctx;
>> +vm_write_end(vma);
>>  
>>  skip:
>>  prev = vma;
>> @@ -1592,8 +1599,10 @@ static int userfaultfd_unregister(struct 
>> userfaultfd_ctx *ctx,
>>   * the next vma was merged into the current one and
>>   * the current one has not been updated yet.
>>   */
>> -vma->vm_flags = new_flags;
>> +vm_write_begin(vma);
>> +WRITE_ONCE(vma->vm_flags, new_flags);
>>  vma->vm_userfaultfd_ctx = NULL_VM_UFFD_CTX;
>> +vm_write_end(vma);
>>  
>>  skip:
>>  prev = vma;
>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>> index b7e2268dfc9a..32314e9e48dd 100644
>> --- a/mm/khugepaged.c
>> +++ b/mm/khugepaged.c
>> @@ -1006,6 +1006,7 @@ static void collapse_huge_page(struct mm_struct *mm,
>>  if (mm_find_pmd(mm, address) != pmd)
>>  goto out;
>>  
>> +vm_write_begin(vma);
>>  anon_vma_lock_write(vma->anon_vma);
>>  
>>  pte = pte_offset_map(pmd, address);
>> @@ -1041,6 +1042,7 @@ static void collapse_huge_page(struct mm_struct *mm,
>>  pmd_populate(mm, pmd, pmd_pgtable(_pmd));
>>  spin_unlock(pmd_ptl);
>>  anon_vma_unlock_write(vma->anon_vma);
>> +vm_write_end(vma);
>>  result = SCAN_FAIL;
>>  goto out;
>>  }
>> @@ -1075,6 +1077,7 @@ static void collapse_huge_page(struct mm_struct *mm,
>>  set_pmd_at(mm, address, pmd, _pmd);
>>  update_mmu_cache_pmd(vma, address, pmd);
>>  spin_unlock(pmd_ptl);
>> +vm_write_end(vma);
>>  
>>  *hpage = NULL;
>>  
>> diff --git a/mm/madvise.c 

Re: [PATCH v9 08/24] mm: Protect VMA modifications using VMA sequence count

[David Rientjes]

On Tue, 13 Mar 2018, Laurent Dufour wrote:

> diff --git a/mm/mmap.c b/mm/mmap.c
> index 5898255d0aeb..d6533cb85213 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -847,17 +847,18 @@ int __vma_adjust(struct vm_area_struct *vma, unsigned 
> long start,
>   }
>  
>   if (start != vma->vm_start) {
> - vma->vm_start = start;
> + WRITE_ONCE(vma->vm_start, start);
>   start_changed = true;
>   }
>   if (end != vma->vm_end) {
> - vma->vm_end = end;
> + WRITE_ONCE(vma->vm_end, end);
>   end_changed = true;
>   }
> - vma->vm_pgoff = pgoff;
> + WRITE_ONCE(vma->vm_pgoff, pgoff);
>   if (adjust_next) {
> - next->vm_start += adjust_next << PAGE_SHIFT;
> - next->vm_pgoff += adjust_next;
> + WRITE_ONCE(next->vm_start,
> +next->vm_start + (adjust_next << PAGE_SHIFT));
> + WRITE_ONCE(next->vm_pgoff, next->vm_pgoff + adjust_next);
>   }
>  
>   if (root) {
> @@ -1781,6 +1782,7 @@ unsigned long mmap_region(struct file *file, unsigned 
> long addr,
>  out:
>   perf_event_mmap(vma);
>  
> + vm_write_begin(vma);
>   vm_stat_account(mm, vm_flags, len >> PAGE_SHIFT);
>   if (vm_flags & VM_LOCKED) {
>   if (!((vm_flags & VM_SPECIAL) || is_vm_hugetlb_page(vma) ||
> @@ -1803,6 +1805,7 @@ unsigned long mmap_region(struct file *file, unsigned 
> long addr,
>   vma->vm_flags |= VM_SOFTDIRTY;
>  
>   vma_set_page_prot(vma);
> + vm_write_end(vma);
>  
>   return addr;
>  

Shouldn't this also protect vma->vm_flags?

diff --git a/mm/mmap.c b/mm/mmap.c
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1796,7 +1796,8 @@ unsigned long mmap_region(struct file *file, unsigned 
long addr,
vma == get_gate_vma(current->mm)))
mm->locked_vm += (len >> PAGE_SHIFT);
else
-   vma->vm_flags &= VM_LOCKED_CLEAR_MASK;
+   WRITE_ONCE(vma->vm_flags,
+  vma->vm_flags & VM_LOCKED_CLEAR_MASK);
}
 
if (file)
@@ -1809,7 +1810,7 @@ unsigned long mmap_region(struct file *file, unsigned 
long addr,
 * then new mapped in-place (which must be aimed as
 * a completely new data area).
 */
-   vma->vm_flags |= VM_SOFTDIRTY;
+   WRITE_ONCE(vma->vm_flags, vma->vm_flags | VM_SOFTDIRTY);
 
vma_set_page_prot(vma);
vm_write_end(vma);

Re: [PATCH v9 08/24] mm: Protect VMA modifications using VMA sequence count

[David Rientjes]

On Tue, 13 Mar 2018, Laurent Dufour wrote:

> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 65ae54659833..a2d9c87b7b0b 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -1136,8 +1136,11 @@ static ssize_t clear_refs_write(struct file *file, 
> const char __user *buf,
>   goto out_mm;
>   }
>   for (vma = mm->mmap; vma; vma = vma->vm_next) {
> - vma->vm_flags &= ~VM_SOFTDIRTY;
> + vm_write_begin(vma);
> + WRITE_ONCE(vma->vm_flags,
> +vma->vm_flags & 
> ~VM_SOFTDIRTY);
>   vma_set_page_prot(vma);
> + vm_write_end(vma);
>   }
>   downgrade_write(>mmap_sem);
>   break;
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index cec550c8468f..b8212ba17695 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -659,8 +659,11 @@ int dup_userfaultfd(struct vm_area_struct *vma, struct 
> list_head *fcs)
>  
>   octx = vma->vm_userfaultfd_ctx.ctx;
>   if (!octx || !(octx->features & UFFD_FEATURE_EVENT_FORK)) {
> + vm_write_begin(vma);
>   vma->vm_userfaultfd_ctx = NULL_VM_UFFD_CTX;
> - vma->vm_flags &= ~(VM_UFFD_WP | VM_UFFD_MISSING);
> + WRITE_ONCE(vma->vm_flags,
> +vma->vm_flags & ~(VM_UFFD_WP | VM_UFFD_MISSING));
> + vm_write_end(vma);
>   return 0;
>   }
>  

In several locations in this patch vm_write_begin(vma) -> 
vm_write_end(vma) is nesting things other than vma->vm_flags, 
vma->vm_policy, etc.  I think it's better to do vm_write_end(vma) as soon 
as the members that the seqcount protects are modified.  In other words, 
this isn't offering protection for vma->vm_userfaultfd_ctx.  There are 
several examples of this in the patch.

> @@ -885,8 +888,10 @@ static int userfaultfd_release(struct inode *inode, 
> struct file *file)
>   vma = prev;
>   else
>   prev = vma;
> - vma->vm_flags = new_flags;
> + vm_write_begin(vma);
> + WRITE_ONCE(vma->vm_flags, new_flags);
>   vma->vm_userfaultfd_ctx = NULL_VM_UFFD_CTX;
> + vm_write_end(vma);
>   }
>   up_write(>mmap_sem);
>   mmput(mm);
> @@ -1434,8 +1439,10 @@ static int userfaultfd_register(struct userfaultfd_ctx 
> *ctx,
>* the next vma was merged into the current one and
>* the current one has not been updated yet.
>*/
> - vma->vm_flags = new_flags;
> + vm_write_begin(vma);
> + WRITE_ONCE(vma->vm_flags, new_flags);
>   vma->vm_userfaultfd_ctx.ctx = ctx;
> + vm_write_end(vma);
>  
>   skip:
>   prev = vma;
> @@ -1592,8 +1599,10 @@ static int userfaultfd_unregister(struct 
> userfaultfd_ctx *ctx,
>* the next vma was merged into the current one and
>* the current one has not been updated yet.
>*/
> - vma->vm_flags = new_flags;
> + vm_write_begin(vma);
> + WRITE_ONCE(vma->vm_flags, new_flags);
>   vma->vm_userfaultfd_ctx = NULL_VM_UFFD_CTX;
> + vm_write_end(vma);
>  
>   skip:
>   prev = vma;
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index b7e2268dfc9a..32314e9e48dd 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -1006,6 +1006,7 @@ static void collapse_huge_page(struct mm_struct *mm,
>   if (mm_find_pmd(mm, address) != pmd)
>   goto out;
>  
> + vm_write_begin(vma);
>   anon_vma_lock_write(vma->anon_vma);
>  
>   pte = pte_offset_map(pmd, address);
> @@ -1041,6 +1042,7 @@ static void collapse_huge_page(struct mm_struct *mm,
>   pmd_populate(mm, pmd, pmd_pgtable(_pmd));
>   spin_unlock(pmd_ptl);
>   anon_vma_unlock_write(vma->anon_vma);
> + vm_write_end(vma);
>   result = SCAN_FAIL;
>   goto out;
>   }
> @@ -1075,6 +1077,7 @@ static void collapse_huge_page(struct mm_struct *mm,
>   set_pmd_at(mm, address, pmd, _pmd);
>   update_mmu_cache_pmd(vma, address, pmd);
>   spin_unlock(pmd_ptl);
> + vm_write_end(vma);
>  
>   *hpage = NULL;
>  
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 4d3c922ea1a1..e328f7ab5942 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -184,7 +184,9 @@ static long madvise_behavior(struct vm_area_struct *vma,
>   /*
>* vm_flags is protected by the mmap_sem held in write mode.
>*/
> - vma->vm_flags = new_flags;
> + vm_write_begin(vma);
> + WRITE_ONCE(vma->vm_flags, 

[PATCH v9 08/24] mm: Protect VMA modifications using VMA sequence count

[Laurent Dufour]

The VMA sequence count has been introduced to allow fast detection of
VMA modification when running a page fault handler without holding
the mmap_sem.

This patch provides protection against the VMA modification done in :
- madvise()
- mpol_rebind_policy()
- vma_replace_policy()
- change_prot_numa()
- mlock(), munlock()
- mprotect()
- mmap_region()
- collapse_huge_page()
- userfaultd registering services

In addition, VMA fields which will be read during the speculative fault
path needs to be written using WRITE_ONCE to prevent write to be split
and intermediate values to be pushed to other CPUs.

Signed-off-by: Laurent Dufour 
---
 fs/proc/task_mmu.c |  5 -
 fs/userfaultfd.c   | 17 +
 mm/khugepaged.c|  3 +++
 mm/madvise.c   |  6 +-
 mm/mempolicy.c | 51 ++-
 mm/mlock.c | 13 -
 mm/mmap.c  | 17 ++---
 mm/mprotect.c  |  4 +++-
 mm/swap_state.c|  8 ++--
 9 files changed, 86 insertions(+), 38 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 65ae54659833..a2d9c87b7b0b 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1136,8 +1136,11 @@ static ssize_t clear_refs_write(struct file *file, const 
char __user *buf,
goto out_mm;
}
for (vma = mm->mmap; vma; vma = vma->vm_next) {
-   vma->vm_flags &= ~VM_SOFTDIRTY;
+   vm_write_begin(vma);
+   WRITE_ONCE(vma->vm_flags,
+  vma->vm_flags & 
~VM_SOFTDIRTY);
vma_set_page_prot(vma);
+   vm_write_end(vma);
}
downgrade_write(>mmap_sem);
break;
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index cec550c8468f..b8212ba17695 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -659,8 +659,11 @@ int dup_userfaultfd(struct vm_area_struct *vma, struct 
list_head *fcs)
 
octx = vma->vm_userfaultfd_ctx.ctx;
if (!octx || !(octx->features & UFFD_FEATURE_EVENT_FORK)) {
+   vm_write_begin(vma);
vma->vm_userfaultfd_ctx = NULL_VM_UFFD_CTX;
-   vma->vm_flags &= ~(VM_UFFD_WP | VM_UFFD_MISSING);
+   WRITE_ONCE(vma->vm_flags,
+  vma->vm_flags & ~(VM_UFFD_WP | VM_UFFD_MISSING));
+   vm_write_end(vma);
return 0;
}
 
@@ -885,8 +888,10 @@ static int userfaultfd_release(struct inode *inode, struct 
file *file)
vma = prev;
else
prev = vma;
-   vma->vm_flags = new_flags;
+   vm_write_begin(vma);
+   WRITE_ONCE(vma->vm_flags, new_flags);
vma->vm_userfaultfd_ctx = NULL_VM_UFFD_CTX;
+   vm_write_end(vma);
}
up_write(>mmap_sem);
mmput(mm);
@@ -1434,8 +1439,10 @@ static int userfaultfd_register(struct userfaultfd_ctx 
*ctx,
 * the next vma was merged into the current one and
 * the current one has not been updated yet.
 */
-   vma->vm_flags = new_flags;
+   vm_write_begin(vma);
+   WRITE_ONCE(vma->vm_flags, new_flags);
vma->vm_userfaultfd_ctx.ctx = ctx;
+   vm_write_end(vma);
 
skip:
prev = vma;
@@ -1592,8 +1599,10 @@ static int userfaultfd_unregister(struct userfaultfd_ctx 
*ctx,
 * the next vma was merged into the current one and
 * the current one has not been updated yet.
 */
-   vma->vm_flags = new_flags;
+   vm_write_begin(vma);
+   WRITE_ONCE(vma->vm_flags, new_flags);
vma->vm_userfaultfd_ctx = NULL_VM_UFFD_CTX;
+   vm_write_end(vma);
 
skip:
prev = vma;
diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index b7e2268dfc9a..32314e9e48dd 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -1006,6 +1006,7 @@ static void collapse_huge_page(struct mm_struct *mm,
if (mm_find_pmd(mm, address) != pmd)
goto out;
 
+   vm_write_begin(vma);
anon_vma_lock_write(vma->anon_vma);
 
pte = pte_offset_map(pmd, address);
@@ -1041,6 +1042,7 @@ static void collapse_huge_page(struct mm_struct *mm,
pmd_populate(mm, pmd, pmd_pgtable(_pmd));
spin_unlock(pmd_ptl);
anon_vma_unlock_write(vma->anon_vma);
+   vm_write_end(vma);
result = SCAN_FAIL;
goto out;