page_mkwrite caller is racy?

After do_wp_page calls page_mkwrite on its target (old_page), it then drops the reference to the page before locking the ptl and verifying that the pte points to old_page.

Unfortunately, old_page may have been truncated and freed, or reclaimed, then re-allocated and used again for the same pagecache position and faulted in read-only into the same pte by another thread. Then you will have a situation where page_mkwrite succeeds but the page we use is actually a readonly one.

Moving page_cache_release(old_page) to below the next statement will fix that problem.

But it is sad that this thing got merged without any callers to even know how it is intended to work.

Must it be able to sleep?

Quoted 8 lines · Nick Piggin

After do_wp_page calls page_mkwrite on its target (old_page), it then drops the reference to the page before locking the ptl and verifying that the pte points to old_page.

Unfortunately, old_page may have been truncated and freed, or reclaimed, then re-allocated and used again for the same pagecache position and faulted in read-only into the same pte by another thread. Then you will have a situation where page_mkwrite succeeds but the page we use is actually a readonly one.

You're right. Well observed. It was I who originally added that page_cache_release/page_cache_get, and the page_cache_get certainly followed getting the page_table_lock when I first added them.

Looks like amidst all the intervening versions, with the patch going into and getting dropped from -mm from time to time, those positions became reversed without us noticing (almost certainly when the lock and the pte_offset_map got merged into the pte_offset_map_lock).

Quoted 3 lines

Moving page_cache_release(old_page) to below the next statement will fix that problem.

Yes. I'm reluctant to steal your credit, but also reluctant to go back and forth too much over this: please insert your Signed-off-by _before_ mine in the patch below (substituting your own comment if you prefer) and send it Andrew.

Not a priority for 2.6.20 or -stable: aside from the unlikelihood, we don't seem to have any page_mkwrite users yet, as you point out.

Quoted 2 lines

But it is sad that this thing got merged without any callers to even know how it is intended to work.

I'm rather to blame for that: I pushed Peter to rearranging his work on top of what David had, since they were dabbling in related issues, and we'd already solved a number of them in relation to page_mkwrite; so then when dirty tracking was wanted in, page_mkwrite came with it.

At the time I believed that AntonA was on the point of using it in NTFS, but apparently not yet.

Quoted 1 line

Must it be able to sleep?

Not as David was using it: that was something I felt strongly it should be allowd to do. For example, in order to allocate backing store for the mmap'ed page to be written (that need has been talked about off and on for years).

Hugh

After do_wp_page has tested page_mkwrite, it must release old_page after acquiring page table lock, not before: at some stage that ordering got reversed, leaving a (very unlikely) window in which old_page might be truncated, freed, and reused in the same position.

Signed-off-by Hugh Dickins <[email protected]>

---

 mm/memory.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

mm/memory.c +1 -3

@@ -1531,8 +1531,6 @@ static int do_wp_page(struct mm_struct *
 			if (vma->vm_ops->page_mkwrite(vma, old_page) < 0)
 				goto unwritable_page;
 
-			page_cache_release(old_page);
-
 			/*
 			 * Since we dropped the lock we need to revalidate
 			 * the PTE as someone else may have changed it.  If

@@ -1541,6 +1539,7 @@ static int do_wp_page(struct mm_struct *
 			 */
 			page_table = pte_offset_map_lock(mm, pmd, address,
 							 &ptl);
+			page_cache_release(old_page);
 			if (!pte_same(*page_table, orig_pte))
 				goto unlock;
 		}
-

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 2 lines · Nick Piggin

Moving page_cache_release(old_page) to below the next statement will fix that problem.

Yes. I'm reluctant to steal your credit, but also reluctant to go back and forth too much over this: please insert your Signed-off-by _before_ mine in the patch below (substituting your own comment if you prefer) and send it Andrew.

Not a priority for 2.6.20 or -stable: aside from the unlikelihood, we don't seem to have any page_mkwrite users yet, as you point out.

Quoted 2 lines

But it is sad that this thing got merged without any callers to even know how it is intended to work.

I'm rather to blame for that: I pushed Peter to rearranging his work on top of what David had, since they were dabbling in related issues, and we'd already solved a number of them in relation to page_mkwrite; so then when dirty tracking was wanted in, page_mkwrite came with it.

Quoted 1 line

Must it be able to sleep?

Not as David was using it: that was something I felt strongly it should be allowd to do. For example, in order to allocate backing store for the mmap'ed page to be written (that need has been talked about off and on for years).

After do_wp_page has tested page_mkwrite, it must release old_page after acquiring page table lock, not before: at some stage that ordering got reversed, leaving a (very unlikely) window in which old_page might be truncated, freed, and reused in the same position.

Signed-off-by Hugh Dickins <[email protected]>

---

 mm/memory.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

mm/memory.c +1 -2

@@ -1531,8 +1531,6 @@ static int do_wp_page(struct mm_struct *
 			if (vma->vm_ops->page_mkwrite(vma, old_page) < 0)
 				goto unwritable_page;
 
-			page_cache_release(old_page);
-
 			/*
 			 * Since we dropped the lock we need to revalidate
 			 * the PTE as someone else may have changed it.  If

@@ -1541,6 +1539,7 @@ static int do_wp_page(struct mm_struct *
 			 */
 			page_table = pte_offset_map_lock(mm, pmd, address,
 							 &ptl);
+			page_cache_release(old_page);
 			if (!pte_same(*page_table, orig_pte))
 				goto unlock;
 		}

This is another discussion, but do we want the page locked here? Or are the filesystems happy to exclude truncate themselves?

Quoted 2 lines · Nick Piggin

This is another discussion, but do we want the page locked here? Or are the filesystems happy to exclude truncate themselves?

No page lock please. Generally, Ocfs2 wants to order cluster locks outside of page locks. Also, the sparse b-tree support I'm working on right now will need to be able to allocate in ->page_mkwrite() which would become very nasty if we came in with the page lock - aside from the additional cluster locks taken, ocfs2 will want to zero some adjacent pages (because we support atomic allocation up to 1 meg).

Quoted 7 lines · Mark Fasheh

No page lock please. Generally, Ocfs2 wants to order cluster locks outside of page locks. Also, the sparse b-tree support I'm working on right now will need to be able to allocate in ->page_mkwrite() which would become very nasty if we came in with the page lock - aside from the additional cluster locks taken, ocfs2 will want to zero some adjacent pages (because we support atomic allocation up to 1 meg).

Ditto for NTFS. I will need to lock pages on both sides of the page for large volume cluster sizes thus I will have to drop the page lock if it is already taken so it might as well not be... Although I do not feel strongly about it. If the page is locked I will just drop the lock and then take it again. If possible to not have the page locked that would make my code a little easier/more efficient I expect...

Quoted 1 line

Must it be able to sleep?

Not as David was using it

But it is sad that this thing got merged without any callers to even know how it is intended to work. Must it be able to sleep?