Re: [PATCH] hold cow while breaking

Re: [PATCH] hold cow while breaking

[Linus Torvalds]

On Sun, 29 Jul 2001, Hugh Dickins wrote:
>
> do_wp_page() COW breaking is now very slightly unsafe.  Please don't
> ask me to provide a test case! but the pte_same() check after regetting
> page_table_lock is not quite enough to guarantee that the old_page was
> not reaped, reused for something else, copy_cow_paged while containing
> that other data, freed and then reused for precisely its original pte.

Oh, but it is.

We do hold the MM semaphore over the whole sequence, so there's no way the
page table entry can be replaced by anything else than a non-present one
(ie vmscan can swap it out, but nothing can swap it in because of the
lock).

So yes, we may copy data that is "garbage", but re-testing the page table
will make sure that if it was garbage we will never use it.

		Linus

Re: [PATCH] hold cow while breaking

[Hugh Dickins]

On Sun, 29 Jul 2001, Linus Torvalds wrote:
> On Sun, 29 Jul 2001, Hugh Dickins wrote:
> >
> > do_wp_page() COW breaking is now very slightly unsafe.  Please don't
> > ask me to provide a test case! but the pte_same() check after regetting
> > page_table_lock is not quite enough to guarantee that the old_page was
> > not reaped, reused for something else, copy_cow_paged while containing
> > that other data, freed and then reused for precisely its original pte.
> 
> Oh, but it is.
> 
> We do hold the MM semaphore over the whole sequence, so there's no way the
> page table entry can be replaced by anything else than a non-present one
> (ie vmscan can swap it out, but nothing can swap it in because of the
> lock).

Sorry for being dense, but I still don't get it.  I thought the
down_read on mmap_sem is permitting concurrent faults by other users
of the address space (but excluding structural changes to the address
space)?  and we haven't locked the page itself, and we've temporarily
dropped the page_table_lock.  I just don't see what lock prevents the
page from being refaulted in.

Hugh

Re: [PATCH] hold cow while breaking

[Linus Torvalds]

On Sun, 29 Jul 2001, Hugh Dickins wrote:
>
> Sorry for being dense, but I still don't get it.  I thought the
> down_read on mmap_sem is permitting concurrent faults by other users
> of the address space (but excluding structural changes to the address
> space)?  and we haven't locked the page itself, and we've temporarily
> dropped the page_table_lock.  I just don't see what lock prevents the
> page from being refaulted in.

Ehh, you're right. But you're still wrong, I think.

Because we hold the mm semaphore, nobody can change the mapping on us.

Which means that even if we first page somthing out and page something
else in to the same page, that "something else" has to be the same thing.
See?

		Linus

Re: [PATCH] hold cow while breaking

[Hugh Dickins]

On Sun, 29 Jul 2001, Linus Torvalds wrote:
> On Sun, 29 Jul 2001, Hugh Dickins wrote:
> >
> > Sorry for being dense, but I still don't get it.  I thought the
> > down_read on mmap_sem is permitting concurrent faults by other users
> > of the address space (but excluding structural changes to the address
> > space)?  and we haven't locked the page itself, and we've temporarily
> > dropped the page_table_lock.  I just don't see what lock prevents the
> > page from being refaulted in.
> 
> Ehh, you're right. But you're still wrong, I think.
> 
> Because we hold the mm semaphore, nobody can change the mapping on us.
> 
> Which means that even if we first page somthing out and page something
> else in to the same page, that "something else" has to be the same thing.
> See?

Sure, I agree with you on that, but it doesn't let us off the hook.
This isn't necessarily the first reuse of that page: after paging it
out and freeing it, it may have got allocated to some other purpose
and freed again, and then reassigned to the original use.  And while
it held the data from that intermediate use, we may have done the
copy_cow_page from old_page to new_page, and now we come along and
substitute this corrupt new_page for the good old_page.

Hugh

--- linux-2.4.8-pre2/mm/memory.c	Sun Jul 29 08:10:42 2001
+++ linux/mm/memory.c	Sun Jul 29 13:12:20 2001
@@ -862,8 +862,8 @@
 /*
  * We hold the mm semaphore for reading and vma->vm_mm->page_table_lock
  */
-static inline void break_cow(struct vm_area_struct * vma, struct page *	old_page, struct page * new_page, unsigned long address, 
-		pte_t *page_table)
+static inline void break_cow(struct vm_area_struct * vma, struct page * new_page,
+			     unsigned long address, pte_t *page_table)
 {
 	flush_page_to_ram(new_page);
 	flush_cache_page(vma, address);
@@ -935,12 +935,15 @@
 	/*
 	 * Ok, we need to copy. Oh, well..
 	 */
+	if (!PageReserved(old_page))
+		page_cache_get(old_page);
 	spin_unlock(&mm->page_table_lock);
 
 	new_page = alloc_page(GFP_HIGHUSER);
 	if (!new_page)
 		goto no_mem;
 	copy_cow_page(old_page,new_page,address);
+	page_cache_release(old_page);
 
 	/*
 	 * Re-check the pte - we dropped the lock
@@ -949,7 +952,7 @@
 	if (pte_same(*page_table, pte)) {
 		if (PageReserved(old_page))
 			++mm->rss;
-		break_cow(vma, old_page, new_page, address, page_table);
+		break_cow(vma, new_page, address, page_table);
 
 		/* Free the old page.. */
 		new_page = old_page;

Re: [PATCH] hold cow while breaking

[Linus Torvalds]

In article <Pine.LNX.4.21.0107292212330.1139-100000@localhost.localdomain> you write:
>
>Sure, I agree with you on that, but it doesn't let us off the hook.
>This isn't necessarily the first reuse of that page: after paging it
>out and freeing it, it may have got allocated to some other purpose
>and freed again, and then reassigned to the original use.

Ok, you've convinced me. Thanks for showing me the error of my ways.
Fixed.

		Linus