[PATCH 1/3] fix page-becoming-writable in do_wp_page

[PATCH 1/3] fix page-becoming-writable in do_wp_page

[Hugh Dickins]

do_wp_page_mk_pte_writable was rather broken: page_mkwrite is for shared
pages, whereas the code it was trying to share was for private pages, as
the recent addition of a PageAnon test there has made more explicit.

Sort that out and reabsorb it into do_wp_page: hch and others found that
separation less than helpful.  And page_cache_get on the old_page before
page_table_lock is dropped - nothing else stabilizes the page in there.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
---

 mm/memory.c |  113 +++++++++++++++++++++++++++---------------------------------
 1 files changed, 51 insertions(+), 62 deletions(-)

--- 2.6.13-rc2-mm2/mm/memory.c	2005-07-07 12:33:21.000000000 +0100
+++ linux/mm/memory.c	2005-07-11 20:01:28.000000000 +0100
@@ -1199,58 +1199,6 @@ static inline void break_cow(struct vm_a
 }
 
 /*
- * Make a PTE writeable for do_wp_page() on a shared-writable page
- */
-static inline int do_wp_page_mk_pte_writable(struct mm_struct *mm,
-					     struct vm_area_struct *vma,
-					     unsigned long address,
-					     pmd_t *pmd,
-					     pte_t *page_table,
-					     struct page *old_page,
-					     pte_t pte)
-{
-	pte_t entry;
-
-	/* See if the VMA's owner wants to know that the page is about to
-	 * become writable */
-	if (vma->vm_ops && vma->vm_ops->page_mkwrite) {
-		/* Notify the page owner without the lock held so they can
-		 * sleep if they want to */
-		pte_unmap(page_table);
-		spin_unlock(&mm->page_table_lock);
-
-		if (vma->vm_ops->page_mkwrite(vma, old_page) < 0)
-			goto bus_error;
-
-		spin_lock(&mm->page_table_lock);
-
-		/* Since we dropped the lock we need to revalidate the PTE as
-		 * someone else may have changed it. If they did, we just
-		 * return, as we can count on the MMU to tell us if they didn't
-		 * also make it writable
-		 */
-		page_table = pte_offset_map(pmd, address);
-		if (!pte_same(*page_table, pte))
-			goto minor_fault;
-	}
-
-	flush_cache_page(vma, address, page_to_pfn(old_page));
-	entry = maybe_mkwrite(pte_mkyoung(pte_mkdirty(pte)),
-			      vma);
-	ptep_set_access_flags(vma, address, page_table, entry, 1);
-	update_mmu_cache(vma, address, entry);
-	lazy_mmu_prot_update(entry);
-	pte_unmap(page_table);
-
- minor_fault:
-	spin_unlock(&mm->page_table_lock);
-	return VM_FAULT_MINOR;
-
- bus_error:
-	return VM_FAULT_SIGBUS;
-}
-
-/*
  * This routine handles present pages, when users try to write
  * to a shared page. It is done by copying the page to a new address
  * and decrementing the shared-page counter for the old page.
@@ -1275,6 +1223,8 @@ static int do_wp_page(struct mm_struct *
 {
 	struct page *old_page, *new_page;
 	unsigned long pfn = pte_pfn(pte);
+	pte_t entry;
+	int reuse;
 
 	if (unlikely(!pfn_valid(pfn))) {
 		/*
@@ -1290,21 +1240,53 @@ static int do_wp_page(struct mm_struct *
 	}
 	old_page = pfn_to_page(pfn);
 
-	if (PageAnon(old_page) && !TestSetPageLocked(old_page)) {
-		int reuse = can_share_swap_page(old_page);
-		unlock_page(old_page);
-		if (reuse) {
-			/* We can just make the PTE writable */
-			return do_wp_page_mk_pte_writable(mm, vma, address, pmd,
-							  page_table, old_page,
-							  pte);
+	if (unlikely(vma->vm_flags & VM_SHARED)) {
+		if (vma->vm_ops && vma->vm_ops->page_mkwrite) {
+			/*
+			 * Notify the page owner without the lock held,
+			 * so they can sleep if they want to.
+			 */
+			pte_unmap(page_table);
+			if (!PageReserved(old_page))
+				page_cache_get(old_page);
+			spin_unlock(&mm->page_table_lock);
+
+			if (vma->vm_ops->page_mkwrite(vma, old_page) < 0)
+				goto unwritable_page;
+
+			spin_lock(&mm->page_table_lock);
+			page_cache_release(old_page);
+
+			/*
+			 * Since we dropped the lock we need to revalidate
+			 * the PTE as someone else may have changed it.  If
+			 * they did, we just return, as we can count on the
+			 * MMU to tell us if they didn't also make it writable.
+			 */
+			page_table = pte_offset_map(pmd, address);
+			if (!pte_same(*page_table, pte))
+				goto success;
 		}
+		reuse = 1;
+	} else if (PageAnon(old_page) && !TestSetPageLocked(old_page)) {
+		reuse = can_share_swap_page(old_page);
+		unlock_page(old_page);
+	} else
+		reuse = 0;
+
+	if (reuse) {
+		flush_cache_page(vma, address, pfn);
+		entry = maybe_mkwrite(pte_mkyoung(pte_mkdirty(pte)), vma);
+		ptep_set_access_flags(vma, address, page_table, entry, 1);
+		update_mmu_cache(vma, address, entry);
+		lazy_mmu_prot_update(entry);
+		goto success;
 	}
-	pte_unmap(page_table);
 
 	/*
 	 * Ok, we need to copy. Oh, well..
 	 */
+	pte_unmap(page_table);
 	if (!PageReserved(old_page))
 		page_cache_get(old_page);
 	spin_unlock(&mm->page_table_lock);
@@ -1321,6 +1303,7 @@ static int do_wp_page(struct mm_struct *
 			goto no_new_page;
 		copy_user_highpage(new_page, old_page, address);
 	}
+
 	/*
 	 * Re-check the pte - we dropped the lock
 	 */
@@ -1341,15 +1324,21 @@ static int do_wp_page(struct mm_struct *
 		/* Free the old page.. */
 		new_page = old_page;
 	}
-	pte_unmap(page_table);
 	page_cache_release(new_page);
 	page_cache_release(old_page);
+
+success:
+	pte_unmap(page_table);
 	spin_unlock(&mm->page_table_lock);
 	return VM_FAULT_MINOR;
 
 no_new_page:
 	page_cache_release(old_page);
 	return VM_FAULT_OOM;
+
+unwritable_page:
+	page_cache_release(old_page);
+	return VM_FAULT_SIGBUS;
 }
 
 /*

[PATCH 2/3] fix page-becoming-writable vm_page_prot

[Hugh Dickins]

The do_wp_page page_mkwrite breakage went unnoticed because do_no_page
was as usual giving write permission to the pte in handling a read fault
on a shared writable mapping, thus sneaking around page_mkwrite.

It could likewise be evaded by do_file_page->populate->install_page.  And
if those were to write protect the pte, mprotect back and forth could be
used to reinstate write permission without going through page_mkwrite.

No explicit change to those: deal with it by using the vm_page_prot
of a private mapping on any shared mapping which has a page_mkwrite.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
---

 mm/mmap.c     |    9 +++++++--
 mm/mprotect.c |    8 ++++++--
 2 files changed, 13 insertions(+), 4 deletions(-)

--- 2.6.13-rc2-mm2/mm/mmap.c	2005-07-07 12:33:21.000000000 +0100
+++ linux/mm/mmap.c	2005-07-11 20:01:28.000000000 +0100
@@ -1051,7 +1051,8 @@ munmap_back:
 	vma->vm_start = addr;
 	vma->vm_end = addr + len;
 	vma->vm_flags = vm_flags;
-	vma->vm_page_prot = protection_map[vm_flags & 0x0f];
+	vma->vm_page_prot = protection_map[vm_flags &
+				(VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)];
 	vma->vm_pgoff = pgoff;
 
 	if (file) {
@@ -1074,6 +1075,9 @@ munmap_back:
 		if (error)
 			goto free_vma;
 	}
+	if (vma->vm_ops && vma->vm_ops->page_mkwrite)
+		vma->vm_page_prot = protection_map[vm_flags &
+					(VM_READ|VM_WRITE|VM_EXEC)];
 
 	/* We set VM_ACCOUNT in a shared mapping's vm_flags, to inform
 	 * shmem_zero_setup (perhaps called through /dev/zero's ->mmap)
@@ -1910,7 +1914,8 @@ unsigned long do_brk(unsigned long addr,
 	vma->vm_end = addr + len;
 	vma->vm_pgoff = pgoff;
 	vma->vm_flags = flags;
-	vma->vm_page_prot = protection_map[flags & 0x0f];
+	vma->vm_page_prot = protection_map[flags &
+				(VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)];
 	vma_link(mm, vma, prev, rb_link, rb_parent);
 out:
 	mm->total_vm += len >> PAGE_SHIFT;
--- 2.6.13-rc2-mm2/mm/mprotect.c	2005-06-17 20:48:29.000000000 +0100
+++ linux/mm/mprotect.c	2005-07-11 20:01:28.000000000 +0100
@@ -107,6 +107,7 @@ mprotect_fixup(struct vm_area_struct *vm
 	unsigned long oldflags = vma->vm_flags;
 	long nrpages = (end - start) >> PAGE_SHIFT;
 	unsigned long charged = 0;
+	unsigned int mask;
 	pgprot_t newprot;
 	pgoff_t pgoff;
 	int error;
@@ -133,8 +134,6 @@ mprotect_fixup(struct vm_area_struct *vm
 		}
 	}
 
-	newprot = protection_map[newflags & 0xf];
-
 	/*
 	 * First try to merge with previous and/or next vma.
 	 */
@@ -161,6 +160,11 @@ mprotect_fixup(struct vm_area_struct *vm
 	}
 
 success:
+	mask = VM_READ|VM_WRITE|VM_EXEC|VM_SHARED;
+	if (vma->vm_ops && vma->vm_ops->page_mkwrite)
+		mask &= ~VM_SHARED;
+	newprot = protection_map[newflags & mask];
+
 	/*
 	 * vm_flags and vm_page_prot are protected by the mmap_sem
 	 * held in write mode.

[PATCH 3/3] fix page-becoming-writable in do_file_page

[Hugh Dickins]

The get_user_pages force write case (from ptrace) expects that a single
call to handle_mm_fault is enough to give it a page it can safely write
to.  This implies that when handling a write access to a page_mkwrite
area, do_file_page must now itself call do_wp_page to call page_mkwrite
and (probably) make the pte writable: that cannot safely be left to a
subsequent fault.

Clarify today's flow of control in do_file_page: it is only called for a
pte_file entry, which only appears in a non-linear vma, which is always
shared and must have a populate: so the do_no_page path is never taken.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
---

 mm/memory.c |   35 +++++++++++++++++++++++------------
 1 files changed, 23 insertions(+), 12 deletions(-)

--- 2.6.13-rc2-mm2/mm/memory.c	2005-07-07 12:33:21.000000000 +0100
+++ linux/mm/memory.c	2005-07-11 20:01:28.000000000 +0100
@@ -1968,27 +1968,38 @@ static int do_file_page(struct mm_struct
 	unsigned long pgoff;
 	int err;
 
-	BUG_ON(!vma->vm_ops || !vma->vm_ops->nopage);
-	/*
-	 * Fall back to the linear mapping if the fs does not support
-	 * ->populate:
-	 */
-	if (!vma->vm_ops || !vma->vm_ops->populate || 
-			(write_access && !(vma->vm_flags & VM_SHARED))) {
-		pte_clear(mm, address, pte);
-		return do_no_page(mm, vma, address, write_access, pte, pmd);
-	}
+	BUG_ON(!vma->vm_ops || !vma->vm_ops->populate);
+	BUG_ON(!(vma->vm_flags & VM_SHARED));
 
 	pgoff = pte_to_pgoff(*pte);
-
+again:
 	pte_unmap(pte);
 	spin_unlock(&mm->page_table_lock);
 
-	err = vma->vm_ops->populate(vma, address & PAGE_MASK, PAGE_SIZE, vma->vm_page_prot, pgoff, 0);
+	err = vma->vm_ops->populate(vma, address & PAGE_MASK, PAGE_SIZE,
+					vma->vm_page_prot, pgoff, 0);
 	if (err == -ENOMEM)
 		return VM_FAULT_OOM;
 	if (err)
 		return VM_FAULT_SIGBUS;
+
+	/*
+	 * For the get_user_pages force write case, we must make sure that
+	 * page_mkwrite is called by this invocation of handle_mm_fault.
+	 */
+	if (write_access && vma->vm_ops->page_mkwrite) {
+		pte_t entry;
+		int ret;
+
+		spin_lock(&mm->page_table_lock);
+		pte = pte_offset_map(pmd, address);
+		entry = *pte;
+		if (!pte_present(entry))
+			goto again;
+		ret = do_wp_page(mm, vma, address, pte, pmd, entry);
+		if (ret != VM_FAULT_MINOR)
+			return ret;
+	}
 	return VM_FAULT_MAJOR;
 }
 

[PATCH 1/3] fix page-becoming-writable in do_wp_page

[Hugh Dickins]

do_wp_page_mk_pte_writable was rather broken: page_mkwrite is for shared
pages, whereas the code it was trying to share was for private pages, as
the recent addition of a PageAnon test there has made more explicit.

Sort that out and reabsorb it into do_wp_page: hch and others found that
separation less than helpful.  And page_cache_get on the old_page before
page_table_lock is dropped - nothing else stabilizes the page in there.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
---

[I sent this out an hour and a half ago, but it still hasn't appeared,
whereas 2/3 and 3/3 did: let's try again, and sorry if it's repeated.]

 mm/memory.c |  113 +++++++++++++++++++++++++++---------------------------------
 1 files changed, 51 insertions(+), 62 deletions(-)

--- 2.6.13-rc2-mm2/mm/memory.c	2005-07-07 12:33:21.000000000 +0100
+++ linux/mm/memory.c	2005-07-11 20:01:28.000000000 +0100
@@ -1199,58 +1199,6 @@ static inline void break_cow(struct vm_a
 }
 
 /*
- * Make a PTE writeable for do_wp_page() on a shared-writable page
- */
-static inline int do_wp_page_mk_pte_writable(struct mm_struct *mm,
-					     struct vm_area_struct *vma,
-					     unsigned long address,
-					     pmd_t *pmd,
-					     pte_t *page_table,
-					     struct page *old_page,
-					     pte_t pte)
-{
-	pte_t entry;
-
-	/* See if the VMA's owner wants to know that the page is about to
-	 * become writable */
-	if (vma->vm_ops && vma->vm_ops->page_mkwrite) {
-		/* Notify the page owner without the lock held so they can
-		 * sleep if they want to */
-		pte_unmap(page_table);
-		spin_unlock(&mm->page_table_lock);
-
-		if (vma->vm_ops->page_mkwrite(vma, old_page) < 0)
-			goto bus_error;
-
-		spin_lock(&mm->page_table_lock);
-
-		/* Since we dropped the lock we need to revalidate the PTE as
-		 * someone else may have changed it. If they did, we just
-		 * return, as we can count on the MMU to tell us if they didn't
-		 * also make it writable
-		 */
-		page_table = pte_offset_map(pmd, address);
-		if (!pte_same(*page_table, pte))
-			goto minor_fault;
-	}
-
-	flush_cache_page(vma, address, page_to_pfn(old_page));
-	entry = maybe_mkwrite(pte_mkyoung(pte_mkdirty(pte)),
-			      vma);
-	ptep_set_access_flags(vma, address, page_table, entry, 1);
-	update_mmu_cache(vma, address, entry);
-	lazy_mmu_prot_update(entry);
-	pte_unmap(page_table);
-
- minor_fault:
-	spin_unlock(&mm->page_table_lock);
-	return VM_FAULT_MINOR;
-
- bus_error:
-	return VM_FAULT_SIGBUS;
-}
-
-/*
  * This routine handles present pages, when users try to write
  * to a shared page. It is done by copying the page to a new address
  * and decrementing the shared-page counter for the old page.
@@ -1275,6 +1223,8 @@ static int do_wp_page(struct mm_struct *
 {
 	struct page *old_page, *new_page;
 	unsigned long pfn = pte_pfn(pte);
+	pte_t entry;
+	int reuse;
 
 	if (unlikely(!pfn_valid(pfn))) {
 		/*
@@ -1290,21 +1240,53 @@ static int do_wp_page(struct mm_struct *
 	}
 	old_page = pfn_to_page(pfn);
 
-	if (PageAnon(old_page) && !TestSetPageLocked(old_page)) {
-		int reuse = can_share_swap_page(old_page);
-		unlock_page(old_page);
-		if (reuse) {
-			/* We can just make the PTE writable */
-			return do_wp_page_mk_pte_writable(mm, vma, address, pmd,
-							  page_table, old_page,
-							  pte);
+	if (unlikely(vma->vm_flags & VM_SHARED)) {
+		if (vma->vm_ops && vma->vm_ops->page_mkwrite) {
+			/*
+			 * Notify the page owner without the lock held,
+			 * so they can sleep if they want to.
+			 */
+			pte_unmap(page_table);
+			if (!PageReserved(old_page))
+				page_cache_get(old_page);
+			spin_unlock(&mm->page_table_lock);
+
+			if (vma->vm_ops->page_mkwrite(vma, old_page) < 0)
+				goto unwritable_page;
+
+			spin_lock(&mm->page_table_lock);
+			page_cache_release(old_page);
+
+			/*
+			 * Since we dropped the lock we need to revalidate
+			 * the PTE as someone else may have changed it.  If
+			 * they did, we just return, as we can count on the
+			 * MMU to tell us if they didn't also make it writable.
+			 */
+			page_table = pte_offset_map(pmd, address);
+			if (!pte_same(*page_table, pte))
+				goto success;
 		}
+		reuse = 1;
+	} else if (PageAnon(old_page) && !TestSetPageLocked(old_page)) {
+		reuse = can_share_swap_page(old_page);
+		unlock_page(old_page);
+	} else
+		reuse = 0;
+
+	if (reuse) {
+		flush_cache_page(vma, address, pfn);
+		entry = maybe_mkwrite(pte_mkyoung(pte_mkdirty(pte)), vma);
+		ptep_set_access_flags(vma, address, page_table, entry, 1);
+		update_mmu_cache(vma, address, entry);
+		lazy_mmu_prot_update(entry);
+		goto success;
 	}
-	pte_unmap(page_table);
 
 	/*
 	 * Ok, we need to copy. Oh, well..
 	 */
+	pte_unmap(page_table);
 	if (!PageReserved(old_page))
 		page_cache_get(old_page);
 	spin_unlock(&mm->page_table_lock);
@@ -1321,6 +1303,7 @@ static int do_wp_page(struct mm_struct *
 			goto no_new_page;
 		copy_user_highpage(new_page, old_page, address);
 	}
+
 	/*
 	 * Re-check the pte - we dropped the lock
 	 */
@@ -1341,15 +1324,21 @@ static int do_wp_page(struct mm_struct *
 		/* Free the old page.. */
 		new_page = old_page;
 	}
-	pte_unmap(page_table);
 	page_cache_release(new_page);
 	page_cache_release(old_page);
+
+success:
+	pte_unmap(page_table);
 	spin_unlock(&mm->page_table_lock);
 	return VM_FAULT_MINOR;
 
 no_new_page:
 	page_cache_release(old_page);
 	return VM_FAULT_OOM;
+
+unwritable_page:
+	page_cache_release(old_page);
+	return VM_FAULT_SIGBUS;
 }
 
 /*