[PATCH 0/4] mm: vma_merge/vma_adjust minor updates against -mm

[PATCH 0/4] mm: vma_merge/vma_adjust minor updates against -mm

[Andrea Arcangeli]

Hello,

Here 4 more minor patches that are incremental against -mm.

In short:

1/4) updates vm_page_prot atomically as it's updated outside the
     rmap_locks. It's always better to use READ_ONCE/WRITE_ONCE if the
     access is concurrent from multiple CPUs, even if there wouldn't
     be risk of reading intermediate values... like in this case there
     is too.

     Not critical, theoretical issue only so far, it might be possible
     that gcc emits right asm code already, but this is safer and
     forces gcc to emit the right asm code.

2/4) comment correction

     Not critical, noop change.

3/4) adapts CONFIG_DEBUG_VM_RB=y to cope with the case of
     next->vm_start reduced. Earlier that could never happen and
     vma->vm_end was always increased instead. validate_mm() is always
     called before returning from vma_adjust() and the argumented
     rbtree is always consistent while exercising a flood of case8.

     Not critical if CONFIG_DEBUG_VM_RB=n as there's no functional
     change in such case. Critical if you set CONFIG_DEBUG_VM_RB=y, in
     which case without the patch false positives are emitted.

4/4) cleanup a line that is superfluous and in turns it can confuse
     the reader if the reader assumes it's not superfluous.

     Not critical, noop change.

Andrea Arcangeli (4):
  mm: vm_page_prot: update with WRITE_ONCE/READ_ONCE
  mm: vma_adjust: minor comment correction
  mm: vma_merge: correct false positive from
    __vma_unlink->validate_mm_rb
  mm: vma_adjust: remove superfluous confusing update in remove_next ==
    1 case

 mm/huge_memory.c |  2 +-
 mm/migrate.c     |  2 +-
 mm/mmap.c        | 94 +++++++++++++++++++++++++++++++++++++++++---------------
 3 files changed, 72 insertions(+), 26 deletions(-)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 1/4] mm: vm_page_prot: update with WRITE_ONCE/READ_ONCE

[Andrea Arcangeli]

vma->vm_page_prot is read lockless from the rmap_walk, it may be
updated concurrently and this prevents the risk of reading
intermediate values.

Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
---
 mm/huge_memory.c | 2 +-
 mm/migrate.c     | 2 +-
 mm/mmap.c        | 9 ++++++---
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index a6abd76..ccdc703 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1566,7 +1566,7 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
 			if (soft_dirty)
 				entry = pte_swp_mksoft_dirty(entry);
 		} else {
-			entry = mk_pte(page + i, vma->vm_page_prot);
+			entry = mk_pte(page + i, READ_ONCE(vma->vm_page_prot));
 			entry = maybe_mkwrite(entry, vma);
 			if (!write)
 				entry = pte_wrprotect(entry);
diff --git a/mm/migrate.c b/mm/migrate.c
index f7ee04a..99250ae 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -234,7 +234,7 @@ static int remove_migration_pte(struct page *new, struct vm_area_struct *vma,
 		goto unlock;
 
 	get_page(new);
-	pte = pte_mkold(mk_pte(new, vma->vm_page_prot));
+	pte = pte_mkold(mk_pte(new, READ_ONCE(vma->vm_page_prot)));
 	if (pte_swp_soft_dirty(*ptep))
 		pte = pte_mksoft_dirty(pte);
 
diff --git a/mm/mmap.c b/mm/mmap.c
index d8b5fb3..4f512ca 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -111,13 +111,16 @@ static pgprot_t vm_pgprot_modify(pgprot_t oldprot, unsigned long vm_flags)
 void vma_set_page_prot(struct vm_area_struct *vma)
 {
 	unsigned long vm_flags = vma->vm_flags;
+	pgprot_t vm_page_prot;
 
-	vma->vm_page_prot = vm_pgprot_modify(vma->vm_page_prot, vm_flags);
+	vm_page_prot = vm_pgprot_modify(vma->vm_page_prot, vm_flags);
 	if (vma_wants_writenotify(vma)) {
 		vm_flags &= ~VM_SHARED;
-		vma->vm_page_prot = vm_pgprot_modify(vma->vm_page_prot,
-						     vm_flags);
+		vm_page_prot = vm_pgprot_modify(vma->vm_page_prot,
+						vm_flags);
 	}
+	/* remove_protection_ptes reads vma->vm_page_prot without mmap_sem */
+	WRITE_ONCE(vma->vm_page_prot, vm_page_prot);
 }
 
 /*

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/4] mm: vm_page_prot: update with WRITE_ONCE/READ_ONCE

[Hillf Danton]

Hey Andrea
> 
> @@ -111,13 +111,16 @@ static pgprot_t vm_pgprot_modify(pgprot_t oldprot, unsigned long vm_flags)
>  void vma_set_page_prot(struct vm_area_struct *vma)
>  {
>  	unsigned long vm_flags = vma->vm_flags;
> +	pgprot_t vm_page_prot;
> 
> -	vma->vm_page_prot = vm_pgprot_modify(vma->vm_page_prot, vm_flags);
> +	vm_page_prot = vm_pgprot_modify(vma->vm_page_prot, vm_flags);
>  	if (vma_wants_writenotify(vma)) {

Since vma->vm_page_prot is currently used in vma_wants_writenotify(), is 
it possible that semantic change is introduced here with local variable? 

thanks
Hillf
>  		vm_flags &= ~VM_SHARED;
> -		vma->vm_page_prot = vm_pgprot_modify(vma->vm_page_prot,
> -						     vm_flags);
> +		vm_page_prot = vm_pgprot_modify(vma->vm_page_prot,
> +						vm_flags);
>  	}
> +	/* remove_protection_ptes reads vma->vm_page_prot without mmap_sem */
> +	WRITE_ONCE(vma->vm_page_prot, vm_page_prot);
>  }
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/4] mm: vm_page_prot: update with WRITE_ONCE/READ_ONCE

[Andrea Arcangeli]

Hello Hillf,

On Thu, Sep 22, 2016 at 03:17:52PM +0800, Hillf Danton wrote:
> Hey Andrea
> > 
> > @@ -111,13 +111,16 @@ static pgprot_t vm_pgprot_modify(pgprot_t oldprot, unsigned long vm_flags)
> >  void vma_set_page_prot(struct vm_area_struct *vma)
> >  {
> >  	unsigned long vm_flags = vma->vm_flags;
> > +	pgprot_t vm_page_prot;
> > 
> > -	vma->vm_page_prot = vm_pgprot_modify(vma->vm_page_prot, vm_flags);
> > +	vm_page_prot = vm_pgprot_modify(vma->vm_page_prot, vm_flags);
> >  	if (vma_wants_writenotify(vma)) {
> 
> Since vma->vm_page_prot is currently used in vma_wants_writenotify(), is 
> it possible that semantic change is introduced here with local variable? 

>From a short review I think you're right.

Writing an intermediate value with WRITE_ONCE before clearing
VM_SHARED wouldn't be correct either if the "vma" was returned by
vma_merge, so to fix this, the intermediate vm_page_prot needs to be
passed as parameter to vma_wants_writenotify(vma, vm_page_prot).

For now it's safer to drop this patch 1/4. The atomic setting of
vm_page_prot in mprotect is an orthogonal problem to the vma_merge
case8 issues in the other patches. The side effect would be the same
("next" vma ptes going out of sync with the write bit set, because
vm_page_prot was the intermediate value created with VM_SHARED still
set in vm_flags) but it's not a bug in vma_merge/vma_adjust here.

I can correct and resend this one later.

While at it, I've to say the handling of VM_SOFTDIRTY across vma_merge
also seems dubious when it's not mmap_region calling vma_merge but
that would be yet another third orthogonal problem, so especially that
one should be handled separately as it'd be specific to soft dirty
only, the atomicity issue above is somewhat more generic.

On a side note, the fix for vma_merge in -mm changes nothing in regard
of the above or soft dirty, they're orthogonal issues.

Thanks,
Andrea

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 1/4] mm: vm_page_prot: update with WRITE_ONCE/READ_ONCE

[Andrea Arcangeli]

vma->vm_page_prot is read lockless from the rmap_walk, it may be
updated concurrently and this prevents the risk of reading
intermediate values.

v2: avoid semantic change noticed by Hillf Danton in
    vma_wants_writenotify().

Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
---
 include/linux/mm.h |  2 +-
 mm/huge_memory.c   |  2 +-
 mm/migrate.c       |  2 +-
 mm/mmap.c          | 16 +++++++++-------
 mm/mprotect.c      |  2 +-
 5 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 2334052..67b48fb 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1529,7 +1529,7 @@ static inline int pte_devmap(pte_t pte)
 }
 #endif
 
-int vma_wants_writenotify(struct vm_area_struct *vma);
+int vma_wants_writenotify(struct vm_area_struct *vma, pgprot_t vm_page_prot);
 
 extern pte_t *__get_locked_pte(struct mm_struct *mm, unsigned long addr,
 			       spinlock_t **ptl);
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index a6abd76..ccdc703 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1566,7 +1566,7 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
 			if (soft_dirty)
 				entry = pte_swp_mksoft_dirty(entry);
 		} else {
-			entry = mk_pte(page + i, vma->vm_page_prot);
+			entry = mk_pte(page + i, READ_ONCE(vma->vm_page_prot));
 			entry = maybe_mkwrite(entry, vma);
 			if (!write)
 				entry = pte_wrprotect(entry);
diff --git a/mm/migrate.c b/mm/migrate.c
index f7ee04a..99250ae 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -234,7 +234,7 @@ static int remove_migration_pte(struct page *new, struct vm_area_struct *vma,
 		goto unlock;
 
 	get_page(new);
-	pte = pte_mkold(mk_pte(new, vma->vm_page_prot));
+	pte = pte_mkold(mk_pte(new, READ_ONCE(vma->vm_page_prot)));
 	if (pte_swp_soft_dirty(*ptep))
 		pte = pte_mksoft_dirty(pte);
 
diff --git a/mm/mmap.c b/mm/mmap.c
index d8b5fb3..12f32dd 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -111,13 +111,15 @@ static pgprot_t vm_pgprot_modify(pgprot_t oldprot, unsigned long vm_flags)
 void vma_set_page_prot(struct vm_area_struct *vma)
 {
 	unsigned long vm_flags = vma->vm_flags;
+	pgprot_t vm_page_prot;
 
-	vma->vm_page_prot = vm_pgprot_modify(vma->vm_page_prot, vm_flags);
-	if (vma_wants_writenotify(vma)) {
+	vm_page_prot = vm_pgprot_modify(vma->vm_page_prot, vm_flags);
+	if (vma_wants_writenotify(vma, vm_page_prot)) {
 		vm_flags &= ~VM_SHARED;
-		vma->vm_page_prot = vm_pgprot_modify(vma->vm_page_prot,
-						     vm_flags);
+		vm_page_prot = vm_pgprot_modify(vm_page_prot, vm_flags);
 	}
+	/* remove_protection_ptes reads vma->vm_page_prot without mmap_sem */
+	WRITE_ONCE(vma->vm_page_prot, vm_page_prot);
 }
 
 /*
@@ -1484,7 +1486,7 @@ SYSCALL_DEFINE1(old_mmap, struct mmap_arg_struct __user *, arg)
  * to the private version (using protection_map[] without the
  * VM_SHARED bit).
  */
-int vma_wants_writenotify(struct vm_area_struct *vma)
+int vma_wants_writenotify(struct vm_area_struct *vma, pgprot_t vm_page_prot)
 {
 	vm_flags_t vm_flags = vma->vm_flags;
 	const struct vm_operations_struct *vm_ops = vma->vm_ops;
@@ -1499,8 +1501,8 @@ int vma_wants_writenotify(struct vm_area_struct *vma)
 
 	/* The open routine did something to the protections that pgprot_modify
 	 * won't preserve? */
-	if (pgprot_val(vma->vm_page_prot) !=
-	    pgprot_val(vm_pgprot_modify(vma->vm_page_prot, vm_flags)))
+	if (pgprot_val(vm_page_prot) !=
+	    pgprot_val(vm_pgprot_modify(vm_page_prot, vm_flags)))
 		return 0;
 
 	/* Do we need to track softdirty? */
diff --git a/mm/mprotect.c b/mm/mprotect.c
index e55e2c9..ec91dfd 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -328,7 +328,7 @@ success:
 	 * held in write mode.
 	 */
 	vma->vm_flags = newflags;
-	dirty_accountable = vma_wants_writenotify(vma);
+	dirty_accountable = vma_wants_writenotify(vma, vma->vm_page_prot);
 	vma_set_page_prot(vma);
 
 	change_protection(vma, start, end, vma->vm_page_prot,

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2/4] mm: vma_adjust: minor comment correction

[Andrea Arcangeli]

The cases are three not two.

Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
---
 mm/mmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index 4f512ca..1b88754 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -663,7 +663,7 @@ int __vma_adjust(struct vm_area_struct *vma, unsigned long start,
 			/*
 			 * vma expands, overlapping all the next, and
 			 * perhaps the one after too (mprotect case 6).
-			 * The only two other cases that gets here are
+			 * The only other cases that gets here are
 			 * case 1, case 7 and case 8.
 			 */
 			if (next == expand) {

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 3/4] mm: vma_merge: correct false positive from __vma_unlink->validate_mm_rb

[Andrea Arcangeli]

The old code was always doing:

   vma->vm_end = next->vm_end
   vma_rb_erase(next) // in __vma_unlink
   vma->vm_next = next->vm_next // in __vma_unlink
   next = vma->vm_next
   vma_gap_update(next)

The new code still does the above for remove_next == 1 and 2, but for
remove_next == 3 it has been changed and it does:

   next->vm_start = vma->vm_start
   vma_rb_erase(vma) // in __vma_unlink
   vma_gap_update(next)

In the latter case, while unlinking "vma", validate_mm_rb() is told to
ignore "vma" that is being removed, but next->vm_start was reduced
instead. So for the new case, to avoid the false positive from
validate_mm_rb, it should be "next" that is ignored when "vma" is
being unlinked.

"vma" and "next" in the above comment, considered pre-swap().

Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
---
 mm/mmap.c | 59 +++++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 41 insertions(+), 18 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index 1b88754..57b1eaf 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -398,15 +398,9 @@ static inline void vma_rb_insert(struct vm_area_struct *vma,
 	rb_insert_augmented(&vma->vm_rb, root, &vma_gap_callbacks);
 }
 
-static void vma_rb_erase(struct vm_area_struct *vma, struct rb_root *root)
+static void __vma_rb_erase(struct vm_area_struct *vma, struct rb_root *root)
 {
 	/*
-	 * All rb_subtree_gap values must be consistent prior to erase,
-	 * with the possible exception of the vma being erased.
-	 */
-	validate_mm_rb(root, vma);
-
-	/*
 	 * Note rb_erase_augmented is a fairly large inline function,
 	 * so make sure we instantiate it only once with our desired
 	 * augmented rbtree callbacks.
@@ -414,6 +408,32 @@ static void vma_rb_erase(struct vm_area_struct *vma, struct rb_root *root)
 	rb_erase_augmented(&vma->vm_rb, root, &vma_gap_callbacks);
 }
 
+static __always_inline void vma_rb_erase_ignore(struct vm_area_struct *vma,
+						struct rb_root *root,
+						struct vm_area_struct *ignore)
+{
+	/*
+	 * All rb_subtree_gap values must be consistent prior to erase,
+	 * with the possible exception of the "next" vma being erased if
+	 * next->vm_start was reduced.
+	 */
+	validate_mm_rb(root, ignore);
+
+	__vma_rb_erase(vma, root);
+}
+
+static __always_inline void vma_rb_erase(struct vm_area_struct *vma,
+					 struct rb_root *root)
+{
+	/*
+	 * All rb_subtree_gap values must be consistent prior to erase,
+	 * with the possible exception of the vma being erased.
+	 */
+	validate_mm_rb(root, vma);
+
+	__vma_rb_erase(vma, root);
+}
+
 /*
  * vma has some anon_vma assigned, and is already inserted on that
  * anon_vma's interval trees.
@@ -600,11 +620,12 @@ static void __insert_vm_struct(struct mm_struct *mm, struct vm_area_struct *vma)
 static __always_inline void __vma_unlink_common(struct mm_struct *mm,
 						struct vm_area_struct *vma,
 						struct vm_area_struct *prev,
-						bool has_prev)
+						bool has_prev,
+						struct vm_area_struct *ignore)
 {
 	struct vm_area_struct *next;
 
-	vma_rb_erase(vma, &mm->mm_rb);
+	vma_rb_erase_ignore(vma, &mm->mm_rb, ignore);
 	next = vma->vm_next;
 	if (has_prev)
 		prev->vm_next = next;
@@ -626,13 +647,7 @@ static inline void __vma_unlink_prev(struct mm_struct *mm,
 				     struct vm_area_struct *vma,
 				     struct vm_area_struct *prev)
 {
-	__vma_unlink_common(mm, vma, prev, true);
-}
-
-static inline void __vma_unlink(struct mm_struct *mm,
-				struct vm_area_struct *vma)
-{
-	__vma_unlink_common(mm, vma, NULL, false);
+	__vma_unlink_common(mm, vma, prev, true, vma);
 }
 
 /*
@@ -811,8 +826,16 @@ again:
 		if (remove_next != 3)
 			__vma_unlink_prev(mm, next, vma);
 		else
-			/* vma is not before next if they've been swapped */
-			__vma_unlink(mm, next);
+			/*
+			 * vma is not before next if they've been
+			 * swapped.
+			 *
+			 * pre-swap() next->vm_start was reduced so
+			 * tell validate_mm_rb to ignore pre-swap()
+			 * "next" (which is stored in post-swap()
+			 * "vma").
+			 */
+			__vma_unlink_common(mm, next, NULL, false, vma);
 		if (file)
 			__remove_shared_vm_struct(next, file, mapping);
 	} else if (insert) {

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 4/4] mm: vma_adjust: remove superfluous confusing update in remove_next == 1 case

[Andrea Arcangeli]

mm->highest_vm_end doesn't need any update.

After finally removing the oddness from vma_merge case 8 that was causing:

1) constant risk of trouble whenever anybody would check vma fields
   from rmap_walks, like it happened when page migration was
   introduced and it read the vma->vm_page_prot from a rmap_walk

2) the callers of vma_merge to re-initialize any value different from
   the current vma, instead of vma_merge() more reliably returning a
   vma that already matches all fields passed as parameter

... it is also worth to take the opportunity of cleaning up
superfluous code in vma_adjust(), that if not removed adds up to the
hard readability of the function.

Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
---
 mm/mmap.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index 57b1eaf..bf2dc6b 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -915,8 +915,28 @@ again:
 		}
 		else if (next)
 			vma_gap_update(next);
-		else
-			mm->highest_vm_end = end;
+		else {
+			/*
+			 * If remove_next == 2 we obviously can't
+			 * reach this path.
+			 *
+			 * If remove_next == 3 we can't reach this
+			 * path because pre-swap() next is always not
+			 * NULL. pre-swap() "next" is not being
+			 * removed and its next->vm_end is not altered
+			 * (and furthermore "end" already matches
+			 * next->vm_end in remove_next == 3).
+			 *
+			 * We reach this only in the remove_next == 1
+			 * case if the "next" vma that was removed was
+			 * the highest vma of the mm. However in such
+			 * case next->vm_end == "end" and the extended
+			 * "vma" has vma->vm_end == next->vm_end so
+			 * mm->highest_vm_end doesn't need any update
+			 * in remove_next == 1 case.
+			 */
+			VM_WARN_ON(mm->highest_vm_end != end);
+		}
 	}
 	if (insert && file)
 		uprobe_mmap(insert);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>