Call me crazy..

Call me crazy..

[Linus Torvalds]

..but there seems to be a huge gaping hole in copy_page_range().

It's called during fork(), and as far as I can tell it doesn't get the
page table lock at all when it copies the page table from the parent to
the child.

Now, just for fun, explain to me why some other process couldn't race with
copy_page_range() on another CPU, and decimate the parents page tables,
resulting in the child getting a page table entry that isn't valid any
more?

Now, that race looks fairly small (we do increase the page count pretty
quickly after having looked up the page in the parent), but even so it
does look to me like the thing needs a 

	spin_lock(&src->page_table_lock);
	..
	spin_unlock(&src->page_table_lock);

around the innermost loop (we don't need it in the destination, because
the destination won't even be visible to the page-outs yet. Never mind the
fact that the destination will be empty, and after we've filled it in it
_would_ be ok to page it out because we no longer care).

Does anybody see why this wouldn't be required?

Can anybody find any _other_ cases of something like this?

		Linus

--
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.eu.org/Linux-MM/

Re: Call me crazy..

[David S. Miller]

   Does anybody see why this wouldn't be required?

One day long ago fork() and vmscan both ran under the
big lock.

Those days are no more, and this now needs locking.

Later,
David S. Miller
davem@redhat.com

--
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.eu.org/Linux-MM/

Re: Call me crazy..

[Stephen C. Tweedie]

Hi,

On Sun, Jan 07, 2001 at 10:51:04PM -0800, Linus Torvalds wrote:
> ..but there seems to be a huge gaping hole in copy_page_range().
> 
> It's called during fork(), and as far as I can tell it doesn't get the
> page table lock at all when it copies the page table from the parent to
> the child.
> 
> Now, just for fun, explain to me why some other process couldn't race with
> copy_page_range() on another CPU, and decimate the parents page tables,
> resulting in the child getting a page table entry that isn't valid any
> more?

It looks like it is needed.  It's even worse on PAE36, where we are
doing things like

				if (!pte_present(pte)) {
					swap_duplicate(pte_to_swp_entry(pte));
					goto cont_copy_pte_range;
				}

without the lock: other CPUs may be doing non-atomic operations such
as ptep_get_and_clear() which leave a !pte_present() pte with invalid
contents for a brief period.

--Stephen
--
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.eu.org/Linux-MM/