BUG? race between kswapd and ptrace (access_process_vm )

BUG? race between kswapd and ptrace (access_process_vm )

[Manfred Spraul]

Is kswapd now running without lock_kernel()?

Then there is a race between swapout and ptrace:
access_process_vm() accesses the page table entries, only protected with
the mmap_sem semaphore and lock_kernel().

Isn't

    spin_lock(&mm->page_table_lock);

missing in access_one_page() [in linux/kernel/ptrace.c]?

--
    Manfred

Re: BUG? race between kswapd and ptrace (access_process_vm )

[Rik van Riel]

On Wed, 7 Mar 2001, Manfred Spraul wrote:

> Is kswapd now running without lock_kernel()?

Indeed ...

> Then there is a race between swapout and ptrace:
> access_process_vm() accesses the page table entries, only protected with
> the mmap_sem semaphore and lock_kernel().
>
> Isn't
>
>     spin_lock(&mm->page_table_lock);
>
> missing in access_one_page() [in linux/kernel/ptrace.c]?

You're probably right here ...

regards,

Rik
--
Linux MM bugzilla: http://linux-mm.org/bugzilla.shtml

Virtual memory is like a game you can't win;
However, without VM there's truly nothing to lose...

		http://www.surriel.com/
http://www.conectiva.com/	http://distro.conectiva.com/

Re: BUG? race between kswapd and ptrace (access_process_vm )

[Manfred Spraul]

Rik van Riel wrote:
> 
> On Wed, 7 Mar 2001, Manfred Spraul wrote:
> 
> > Is kswapd now running without lock_kernel()?
> 
> Indeed ...
> 
> > Then there is a race between swapout and ptrace:
> > access_process_vm() accesses the page table entries, only protected with
> > the mmap_sem semaphore and lock_kernel().
> >
> > Isn't
> >
> >     spin_lock(&mm->page_table_lock);
> >
> > missing in access_one_page() [in linux/kernel/ptrace.c]?
> 
> You're probably right here ...
>

Fixing the bug is more difficult than I thought:

Initially I assumed it would be a two-liner (lock, unlock) but kmap()
can sleep.

Can I reuse a kmap_atomic() type or should I add a new type?

I could add local_irq_save() and (ab)use KMAP_BOUNCE_READ, but I'm not
sure if that's the Right Thing(tm)

--
	Manfred

Re: BUG? race between kswapd and ptrace (access_process_vm )

[Stephen C. Tweedie]

[-- Attachment #1: Type: text/plain, Size: 460 bytes --]

Hi,

On Thu, Mar 08, 2001 at 09:12:52PM +0100, Manfred Spraul wrote:
> > 
> Fixing the bug is more difficult than I thought:
> 
> Initially I assumed it would be a two-liner (lock, unlock) but kmap()
> can sleep.
> 
> Can I reuse a kmap_atomic() type or should I add a new type?

I've just tried with the patch below and it seems fine.  You don't
need to hold the spinlock over the kmap() call: you only need to hold
a reference to the page.

Cheers,
 Stephen

[-- Attachment #2: ptrace-fix.diff --]
[-- Type: text/plain, Size: 1162 bytes --]

--- linux-2.4.2-ac18/kernel/ptrace.c.~1~	Thu Nov  9 03:01:34 2000
+++ linux-2.4.2-ac18/kernel/ptrace.c	Mon Mar 12 11:32:30 2001
@@ -28,6 +28,7 @@
 	struct page *page;
 
 repeat:
+	spin_lock(&mm->page_table_lock);
 	pgdir = pgd_offset(vma->vm_mm, addr);
 	if (pgd_none(*pgdir))
 		goto fault_in_page;
@@ -47,9 +48,13 @@
 
 	/* ZERO_PAGE is special: reads from it are ok even though it's marked reserved */
 	if (page != ZERO_PAGE(addr) || write) {
-		if ((!VALID_PAGE(page)) || PageReserved(page))
+		if ((!VALID_PAGE(page)) || PageReserved(page)) {
+			spin_unlock(&mm->page_table_lock);
 			return 0;
+		}
 	}
+	get_page(page);
+	spin_unlock(&mm->page_table_lock);
 	flush_cache_page(vma, addr);
 
 	if (write) {
@@ -64,19 +69,23 @@
 		flush_page_to_ram(page);
 		kunmap(page);
 	}
+	put_page(page);
 	return len;
 
 fault_in_page:
+	spin_unlock(&mm->page_table_lock);
 	/* -1: out of memory. 0 - unmapped page */
 	if (handle_mm_fault(mm, vma, addr, write) > 0)
 		goto repeat;
 	return 0;
 
 bad_pgd:
+	spin_unlock(&mm->page_table_lock);
 	pgd_ERROR(*pgdir);
 	return 0;
 
 bad_pmd:
+	spin_unlock(&mm->page_table_lock);
 	pmd_ERROR(*pgmiddle);
 	return 0;
 }