mlockall and mmap of IO devices don't mix

mlockall and mmap of IO devices don't mix

[Joe Korty]

2.6.0-test6: the use of mlockall(2) in a process that has mmap(2)ed
the registers of an IO device will hang that process uninterruptibly.
The task runs in an infinite loop in get_user_pages(), invoking
follow_page() forever.

Using binary search I discovered that the problem was introduced
in 2.5.14, specifically in ChangeSetKey

    zippel@linux-m68k.org|ChangeSet|20020503210330|37095

The following patch backs out those lines of the above changeset
which are causing the problem:

--- a/mm/memory.c	2003-10-02 15:32:51.000000000 -0400
+++ b/mm/memory.c	2003-10-02 17:02:48.000000000 -0400
@@ -485,9 +485,7 @@
 	if (pte_present(pte)) {
 		if (!write ||
 		    (pte_write(pte) && pte_dirty(pte))) {
-			pfn = pte_pfn(pte);
-			if (pfn_valid(pfn))
-				return pfn_to_page(pfn);
+			return pte_page(pte);
 		}
 	}
 
The equivalent backout patch for 2.6.0-test6 is:

--- linux-2.6.0-test6/mm/memory.c.orig	2003-09-27 20:50:19.000000000 -0400
+++ linux-2.6.0-test6/mm/memory.c	2003-10-03 16:17:25.000000000 -0400
@@ -618,7 +618,6 @@
 	pgd_t *pgd;
 	pmd_t *pmd;
 	pte_t *ptep, pte;
-	unsigned long pfn;
 	struct vm_area_struct *vma;
 
 	vma = hugepage_vma(mm, address);
@@ -645,13 +644,11 @@
 	pte_unmap(ptep);
 	if (pte_present(pte)) {
 		if (!write || (pte_write(pte) && pte_dirty(pte))) {
-			pfn = pte_pfn(pte);
-			if (pfn_valid(pfn)) {
-				struct page *page = pfn_to_page(pfn);
-
+			struct page *page = pte_page(pte);
+			if (pfn_valid(pte_pfn(pte))) {
 				mark_page_accessed(page);
-				return page;
 			}
+			return page;
 		}
 	}
 
I do not believe that the above constitutes a correct fix.  The
problem is that follow_pages() is fundamentally not able to handle a
mapping which does not have a 'struct page' backing it up, and a
mapping to IO memory by definition has no 'struct page' structure to
back it up.

IMO the original 2.5.14 patch which 'broke' the kernel is correct.
It made follow_page() return zero for a 'struct page' address, which
is entirely reasonable when a mapping has no 'struct page'.  Prior to
2.5.14 follow_page() would return some meaningless nonzero value
which, nevertheless, allowed its caller to continue and establish an
apparently correct mapping.

Joe

PS: my test program.  Nice and short.

/* test mlockall() in conjunction with mmap(2) of an IO device.
 * success -- program exits.
 * failure -- program hangs up, unkillable.
 */
#include <stdio.h>
#include <stdlib.h>
#include <fcntl.h>
#include <sys/mman.h>

int main(int argc, char **argv)
{
	int stat, fd;
	unsigned char *p;
	unsigned long off;

	if (argc > 1) {
		off = strtoul(argv[1], NULL, 16);
	} else {
		fprintf(stderr, "arg is video card addr in /dev/mem");
		exit(1);
	}

	stat = mlockall(MCL_CURRENT | MCL_FUTURE);
	if (stat == -1) {
		perror("mlockall");
		exit(1);
	}

	fd = open ("/dev/mem", O_RDWR, 0666);
	if (fd == -1) {
		perror("open");
		exit(1);
	}

	p = (unsigned char *)mmap(NULL, 0x4096,
		PROT_READ | PROT_WRITE, MAP_SHARED, fd, (off_t)off);
	if (!p) {
		perror("mmap");
		exit(1);
	}
	printf("[%08lx] %02x%02x%02x%02x\n",
		off, p[0], p[1], p[2], p[3]);
	return 0;
}

Re: mlockall and mmap of IO devices don't mix

[Andrew Morton]

Joe Korty <joe.korty@ccur.com> wrote:
>
> 2.6.0-test6: the use of mlockall(2) in a process that has mmap(2)ed
> the registers of an IO device will hang that process uninterruptibly.
> The task runs in an infinite loop in get_user_pages(), invoking
> follow_page() forever.

whoops.

I think the right fix is in get_user_pages(): if the region is VM_IO then
don't call follow_page() at all.

Something like this?

 25-akpm/mm/memory.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff -puN mm/memory.c~get_user_pages-handle-VM_IO mm/memory.c
--- 25/mm/memory.c~get_user_pages-handle-VM_IO	Fri Oct  3 15:22:18 2003
+++ 25-akpm/mm/memory.c	Fri Oct  3 15:22:18 2003
@@ -683,6 +683,7 @@ int get_user_pages(struct task_struct *t
 		struct page **pages, struct vm_area_struct **vmas)
 {
 	int i;
+	int vm_io;
 	unsigned int flags;
 
 	/* 
@@ -739,8 +740,8 @@ int get_user_pages(struct task_struct *t
 		}
 #endif
 
-		if (!vma || (pages && (vma->vm_flags & VM_IO))
-				|| !(flags & vma->vm_flags))
+		vm_io = vma->vm_flags & VM_IO;
+		if (!vma || (pages && vm_io) || !(flags & vma->vm_flags))
 			return i ? : -EFAULT;
 
 		if (is_vm_hugetlb_page(vma)) {
@@ -750,8 +751,14 @@ int get_user_pages(struct task_struct *t
 		}
 		spin_lock(&mm->page_table_lock);
 		do {
-			struct page *map;
-			while (!(map = follow_page(mm, start, write))) {
+			struct page *map = NULL;
+
+			/*
+			 * We don't follow pagetables for VM_IO regions - they
+			 * have no pageframes.  And the caller passed NULL
+			 * for `pages' anyway.
+			 */
+			while (!vm_io && !(map = follow_page(mm,start,write))) {
 				spin_unlock(&mm->page_table_lock);
 				switch (handle_mm_fault(mm,vma,start,write)) {
 				case VM_FAULT_MINOR:

_

Re: mlockall and mmap of IO devices don't mix

[Joe Korty]

On Fri, Oct 03, 2003 at 03:23:49PM -0700, Andrew Morton wrote:
> Joe Korty <joe.korty@ccur.com> wrote:
> >
> > 2.6.0-test6: the use of mlockall(2) in a process that has mmap(2)ed
> > the registers of an IO device will hang that process uninterruptibly.
> > The task runs in an infinite loop in get_user_pages(), invoking
> > follow_page() forever.
> 
> whoops.
> 
> I think the right fix is in get_user_pages(): if the region is VM_IO then
> don't call follow_page() at all.
> 
> Something like this?


Sigh.  No go; it *looks* good but my app still locks up....

Regards,
Joe

Re: mlockall and mmap of IO devices don't mix

[Andrew Morton]

Joe Korty <joe.korty@ccur.com> wrote:
>
> > Something like this?
> 
> 
> Sigh.  No go; it *looks* good but my app still locks up....

Oh crap, you're mapping /dev/mem rather than going through a device driver.
/dev/mem isn't setting VM_IO.

Does this little experiment make it go?


diff -puN drivers/char/mem.c~a drivers/char/mem.c
--- 25/drivers/char/mem.c~a	Fri Oct  3 16:04:04 2003
+++ 25-akpm/drivers/char/mem.c	Fri Oct  3 16:04:15 2003
@@ -176,7 +176,7 @@ static int mmap_mem(struct file * file, 
 	/*
 	 * Don't dump addresses that are not real memory to a core file.
 	 */
-	if (uncached)
+//	if (uncached)
 		vma->vm_flags |= VM_IO;
 
 	if (remap_page_range(vma, vma->vm_start, offset, vma->vm_end-vma->vm_start,

_

Course I could test it myself...


I wonder what to do.  Perhaps /dev/mam should set VM_IO if any of the
mapped pages are not valid mem_map-style pageframes.  Or maybe it should
just set VM_IO all the time.

Re: mlockall and mmap of IO devices don't mix

[Andrew Morton]

Joe Korty <joe.korty@ccur.com> wrote:
>
> Sigh.  No go; it *looks* good but my app still locks up....

Or we could use that VM_RESERVED thing?

 25-akpm/mm/memory.c |   15 +++++++++++----
 drivers/char/mem.c  |    0 
 2 files changed, 11 insertions(+), 4 deletions(-)

diff -puN mm/memory.c~get_user_pages-handle-VM_IO mm/memory.c
--- 25/mm/memory.c~get_user_pages-handle-VM_IO	Fri Oct  3 16:12:50 2003
+++ 25-akpm/mm/memory.c	Fri Oct  3 16:15:07 2003
@@ -683,6 +683,7 @@ int get_user_pages(struct task_struct *t
 		struct page **pages, struct vm_area_struct **vmas)
 {
 	int i;
+	int special;
 	unsigned int flags;
 
 	/* 
@@ -739,8 +740,8 @@ int get_user_pages(struct task_struct *t
 		}
 #endif
 
-		if (!vma || (pages && (vma->vm_flags & VM_IO))
-				|| !(flags & vma->vm_flags))
+		special = vma->vm_flags & (VM_IO | VM_RESERVED);
+		if (!vma || (pages && vm_io) || !(flags & vma->vm_flags))
 			return i ? : -EFAULT;
 
 		if (is_vm_hugetlb_page(vma)) {
@@ -750,8 +751,14 @@ int get_user_pages(struct task_struct *t
 		}
 		spin_lock(&mm->page_table_lock);
 		do {
-			struct page *map;
-			while (!(map = follow_page(mm, start, write))) {
+			struct page *map = NULL;
+
+			/*
+			 * We don't follow pagetables for VM_IO regions or
+			 * mappings of /dev/mem - they may have no pageframes.
+			 * And the caller passed NULL for `pages' anyway.
+			 */
+			while (!special && !(map=follow_page(mm,start,write)) {
 				spin_unlock(&mm->page_table_lock);
 				switch (handle_mm_fault(mm,vma,start,write)) {
 				case VM_FAULT_MINOR:
diff -puN drivers/char/mem.c~get_user_pages-handle-VM_IO drivers/char/mem.c

_

Re: mlockall and mmap of IO devices don't mix

[Joe Korty]

> Oh crap, you're mapping /dev/mem rather than going through a device driver.
> /dev/mem isn't setting VM_IO.

You're right; I just tried your original patch with a real device driver
and it worked.


> Does this little experiment make it go?
> -	if (uncached)
> +//	if (uncached)

Yes it does....

(am about to look at and try your third patch out).

Comments of the first patch:

	vm_io = vma->vm_flags & VM_IO;

uses 'vma' before it has been established that vma is a
non-NULL pointer.  Perhaps we should go back to just using
(vma_flags &VM_IO) everywhere it is needed?

Also, conside changing
	
	while (!vm_io && !(map = follow_page(mm,start,write))) {
to
	if (!vm_io) {
	    while (!(map = ...))) {
		....
            }
            if (pages) {
		....
            }
        }

as it more clearly codes up the intended effect, rather than relying
on  side effects ('pages' happens to be NULL whenever VM_IO is set).

Joe

Re: mlockall and mmap of IO devices don't mix

[Joe Korty]

On Fri, Oct 03, 2003 at 04:15:40PM -0700, Andrew Morton wrote:
>> Sigh.  No go; it *looks* good but my app still locks up....
> 
> Or we could use that VM_RESERVED thing?

Hi Andrew,
 Your third patch worked perfectly for all the tested cases:
    o /dev/mem at offset fd000000 (my video card mem addr)
    o /dev/mem at offset 0
    o with an mmapable device driver.

I did have to make two changes to get it to compile:

--- mm/memory.c.am3	2003-10-03 19:44:17.000000000 -0400
+++ mm/memory.c	2003-10-03 19:43:47.000000000 -0400
@@ -738,7 +738,7 @@
 #endif
 
 		special = vma->vm_flags & (VM_IO | VM_RESERVED);
-		if (!vma || (pages && vm_io) || !(flags & vma->vm_flags))
+		if (!vma || (pages && special) || !(flags & vma->vm_flags))
 			return i ? : -EFAULT;
 
 		if (is_vm_hugetlb_page(vma)) {
@@ -755,7 +755,7 @@
 			 * mappings of /dev/mem - they may have no pageframes.
 			 * And the caller passed NULL for `pages' anyway.
 			 */
-			while (!special && !(map=follow_page(mm,start,write)) {
+			while (!special && !(map=follow_page(mm,start,write))) {
 				spin_unlock(&mm->page_table_lock);
 				switch (handle_mm_fault(mm,vma,start,write)) {
 				case VM_FAULT_MINOR:

In the first change, 'special' != '(vma->vma_flags & VM_IO)' which
was what was originally being tested.  Could that cause a problem?

Also, could the use of VM_RESERVED cause in some cases memory with
pageframes to skip adjustment/use of those pageframes?

Regards, and thanks,
Joe

Re: mlockall and mmap of IO devices don't mix

[Andrew Morton]

Joe Korty <joe.korty@ccur.com> wrote:
>
>  Your third patch worked perfectly for all the tested cases:
>     o /dev/mem at offset fd000000 (my video card mem addr)
>     o /dev/mem at offset 0
>     o with an mmapable device driver.

OK, thanks.

> I did have to make two changes to get it to compile:


> --- mm/memory.c.am3	2003-10-03 19:44:17.000000000 -0400
> +++ mm/memory.c	2003-10-03 19:43:47.000000000 -0400
> @@ -738,7 +738,7 @@
>  #endif
>  
>  		special = vma->vm_flags & (VM_IO | VM_RESERVED);
> -		if (!vma || (pages && vm_io) || !(flags & vma->vm_flags))
> +		if (!vma || (pages && special) || !(flags & vma->vm_flags))
>  			return i ? : -EFAULT;
>  
>  		if (is_vm_hugetlb_page(vma)) {
> @@ -755,7 +755,7 @@
>  			 * mappings of /dev/mem - they may have no pageframes.
>  			 * And the caller passed NULL for `pages' anyway.
>  			 */
> -			while (!special && !(map=follow_page(mm,start,write)) {
> +			while (!special && !(map=follow_page(mm,start,write))) {
>  				spin_unlock(&mm->page_table_lock);
>  				switch (handle_mm_fault(mm,vma,start,write)) {
>  				case VM_FAULT_MINOR:
> 
> In the first change, 'special' != '(vma->vma_flags & VM_IO)' which
> was what was originally being tested.  Could that cause a problem?

Yes, this is saying that you cannot extract pageframes from a mapping of
/dev/mem.  It means that you can no longer, for example, do a direct-IO
write of a chunk of /dev/mem onto disk.  I seem to recall that this is
disallowed for other reasons anwyay.

> Also, could the use of VM_RESERVED cause in some cases memory with
> pageframes to skip adjustment/use of those pageframes?

Could be so, if someone is trying to (say) do a direct-IO write of some
sound card's DMA buffer to disk.  Which is a pretty reasonable thing to do.

Maybe it's best to not overload VM_RESERVED in this manner, but to always
mark /dev/mem as VM_IO. 

Re: mlockall and mmap of IO devices don't mix

[David S. Miller]

On Fri, 3 Oct 2003 17:27:27 -0700
Andrew Morton <akpm@osdl.org> wrote:

> Maybe it's best to not overload VM_RESERVED in this manner, but to always
> mark /dev/mem as VM_IO. 

Just in cast is isn't clear, it should be defined that anything
that sets VM_IO on an mmap() area should prefill the page tables
as a side effect of such a mmap() request.

Then things like mlockall() have simple semantics on VM_IO area, they
end up being a nop.

Re: mlockall and mmap of IO devices don't mix

[Ingo Oeser]

On Saturday 04 October 2003 07:47, David S. Miller wrote:
> On Fri, 3 Oct 2003 17:27:27 -0700
>
> Andrew Morton <akpm@osdl.org> wrote:
> > Maybe it's best to not overload VM_RESERVED in this manner, but to always
> > mark /dev/mem as VM_IO.
>
> Just in cast is isn't clear, it should be defined that anything
> that sets VM_IO on an mmap() area should prefill the page tables
> as a side effect of such a mmap() request.
>
> Then things like mlockall() have simple semantics on VM_IO area, they
> end up being a nop.

It should be already, but the check in get_user_pages() looks wrong and
will only disallow VM_IO if you provide space for an page array.
It should be unconditional and we are done, because follow_pages will
never be called with such vmas.

Putting this check for "forbidden VMAs" into follow_pages is also a good
decision, if follow_pages() is called by strange callers not knowing the
limitations. kernel/futex.c is such an user, which don't check these in
the fast path (does it like wait on hardware triggered futexes there?).

It might be good to add VM_RESERVED check to get_user_pages(), too.
These pages are available anyway, so never need to be considered from
its users.

Regards

Ingo Oeser

Re: mlockall and mmap of IO devices don't mix

[Mark Hounschell]

Joe Korty wrote:
> 
> 2.6.0-test6: the use of mlockall(2) in a process that has mmap(2)ed
> the registers of an IO device will hang that process uninterruptibly.
> The task runs in an infinite loop in get_user_pages(), invoking
> follow_page() forever.
> 
> Using binary search I discovered that the problem was introduced
> in 2.5.14, specifically in ChangeSetKey
> 
>     zippel@linux-m68k.org|ChangeSet|20020503210330|37095
> 

I know this is an old thread but can anyone tell me if this problem is
resolved in the current 2.6.6 kernel? 

Thanks
Mark

Re: mlockall and mmap of IO devices don't mix

[Andrew Morton]

Mark Hounschell <markh@compro.net> wrote:
>
> Joe Korty wrote:
> > 
> > 2.6.0-test6: the use of mlockall(2) in a process that has mmap(2)ed
> > the registers of an IO device will hang that process uninterruptibly.
> > The task runs in an infinite loop in get_user_pages(), invoking
> > follow_page() forever.
> > 
> > Using binary search I discovered that the problem was introduced
> > in 2.5.14, specifically in ChangeSetKey
> > 
> >     zippel@linux-m68k.org|ChangeSet|20020503210330|37095
> > 
> 
> I know this is an old thread but can anyone tell me if this problem is
> resolved in the current 2.6.6 kernel? 
> 

There's an utterly ancient patch in -mm which might fix this.

http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.6/2.6.6-mm4/broken-out/get_user_pages-handle-VM_IO.patch

Re: mlockall and mmap of IO devices don't mix

[Mark Hounschell]

Andrew Morton wrote:
> 
> Mark Hounschell <markh@compro.net> wrote:
> >
> > Joe Korty wrote:
> > >
> > > 2.6.0-test6: the use of mlockall(2) in a process that has mmap(2)ed
> > > the registers of an IO device will hang that process uninterruptibly.
> > > The task runs in an infinite loop in get_user_pages(), invoking
> > > follow_page() forever.
> > >
> > > Using binary search I discovered that the problem was introduced
> > > in 2.5.14, specifically in ChangeSetKey
> > >
> > >     zippel@linux-m68k.org|ChangeSet|20020503210330|37095
> > >
> >
> > I know this is an old thread but can anyone tell me if this problem is
> > resolved in the current 2.6.6 kernel?
> >
> 
> There's an utterly ancient patch in -mm which might fix this.
> 
> http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.6/2.6.6-mm4/broken-out/get_user_pages-handle-VM_IO.patch

Thanks for that. I'll try it. 

Mark

Re: mlockall and mmap of IO devices don't mix

[Mark Hounschell]

Mark Hounschell wrote:
> 
> Andrew Morton wrote:
> >
> > Mark Hounschell <markh@compro.net> wrote:
> > >
> > > Joe Korty wrote:
> > > >
> > > > 2.6.0-test6: the use of mlockall(2) in a process that has mmap(2)ed
> > > > the registers of an IO device will hang that process uninterruptibly.
> > > > The task runs in an infinite loop in get_user_pages(), invoking
> > > > follow_page() forever.
> > > >
> > > > Using binary search I discovered that the problem was introduced
> > > > in 2.5.14, specifically in ChangeSetKey
> > > >
> > > >     zippel@linux-m68k.org|ChangeSet|20020503210330|37095
> > > >
> > >
> > > I know this is an old thread but can anyone tell me if this problem is
> > > resolved in the current 2.6.6 kernel?
> > >
> >
> > There's an utterly ancient patch in -mm which might fix this.
> >
> > http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.6/2.6.6-mm4/broken-out/get_user_pages-handle-VM_IO.patch
> 
> Thanks for that. I'll try it.
> 
> Mark

Thank you. That definatly fixed my problem. Is fixing this main line
WIP? 

Regards
Mark

Re: mlockall and mmap of IO devices don't mix

[Joe Korty]

On Fri, May 21, 2004 at 07:13:26PM -0700, Andrew Morton wrote:
> Mark Hounschell <markh@compro.net> wrote:
> >
> > Joe Korty wrote:
> > > 
> > > 2.6.0-test6: the use of mlockall(2) in a process that has mmap(2)ed
> > > the registers of an IO device will hang that process uninterruptibly.
> > > The task runs in an infinite loop in get_user_pages(), invoking
> > > follow_page() forever.
> > > 
> > > Using binary search I discovered that the problem was introduced
> > > in 2.5.14, specifically in ChangeSetKey
> > > 
> > >     zippel@linux-m68k.org|ChangeSet|20020503210330|37095
> > > 
> > 
> > I know this is an old thread but can anyone tell me if this problem is
> > resolved in the current 2.6.6 kernel? 
> > 
> 
> There's an utterly ancient patch in -mm which might fix this.
> 
> http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.6/2.6.6-mm4/broken-out/get_user_pages-handle-VM_IO.patch

[ 2nd send -- corporate email system in the throes of being scrambled / updated ]

Andrew,
I have been using this patch for ages.  Any chance of it being forwared to
the official tree?

Regards,
Joe

Re: mlockall and mmap of IO devices don't mix

[Andrew Morton]

Joe Korty <joe.korty@ccur.com> wrote:
>
> On Fri, May 21, 2004 at 07:13:26PM -0700, Andrew Morton wrote:
>  > Mark Hounschell <markh@compro.net> wrote:
>  > >
>  > > Joe Korty wrote:
>  > > > 
>  > > > 2.6.0-test6: the use of mlockall(2) in a process that has mmap(2)ed
>  > > > the registers of an IO device will hang that process uninterruptibly.
>  > > > The task runs in an infinite loop in get_user_pages(), invoking
>  > > > follow_page() forever.
>  > > > 
>  > > > Using binary search I discovered that the problem was introduced
>  > > > in 2.5.14, specifically in ChangeSetKey
>  > > > 
>  > > >     zippel@linux-m68k.org|ChangeSet|20020503210330|37095
>  > > > 
>  > > 
>  > > I know this is an old thread but can anyone tell me if this problem is
>  > > resolved in the current 2.6.6 kernel? 
>  > > 
>  > 
>  > There's an utterly ancient patch in -mm which might fix this.
>  > 
>  > http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.6/2.6.6-mm4/broken-out/get_user_pages-handle-VM_IO.patch
> 
>  [ 2nd send -- corporate email system in the throes of being scrambled / updated ]
> 
>  Andrew,
>  I have been using this patch for ages.  Any chance of it being forwared to
>  the official tree?

That patch had its first birthday last week.  I wrote it in response to
some long-forgotten problem, failed to changelog it at the time then forgot
why I wrote it.  I kept it in the hope that I'd remember why I wrote it.  I
subsequently wrote a best-effort changelog but am unconvinced by it.  Ho
hum.

Let me genuflect a bit.  I guess we can be reasonably confident it won't
break anything.

Re: mlockall and mmap of IO devices don't mix

[Joe Korty]

On Tue, May 25, 2004 at 03:47:15PM -0400, Andrew Morton wrote:

>>>>> 2.6.0-test6: the use of mlockall(2) in a process that has mmap(2)ed
>>>>> the registers of an IO device will hang that process uninterruptibly.
>>>>> The task runs in an infinite loop in get_user_pages(), invoking
>>>>> follow_page() forever.

>>>> I know this is an old thread but can anyone tell me if this problem is
>>>> resolved in the current 2.6.6 kernel? 

>>> There's an utterly ancient patch in -mm which might fix this.

> That patch had its first birthday last week.  I wrote it in response to
> some long-forgotten problem, failed to changelog it at the time then forgot
> why I wrote it.  I kept it in the hope that I'd remember why I wrote it.  I
> subsequently wrote a best-effort changelog but am unconvinced by it.  Ho
> hum.
> 
> Let me genuflect a bit.  I guess we can be reasonably confident it won't
> break anything.

How about this for a ChangeLog (also created from memory and from some
of your inlined comments):

    Do not follow pagetables for VM_IO regions, they might
    not have pageframes.

    Discovered when an mlockall'ed program tried to mmap
    some device's registers (using /dev/mem); the program
    hangs on the mmap, looping forever in get_user_pages(),
    trying to do a follow_page() that never succeeds.

-- 
Joe
"Money can buy bandwidth, but latency is forever" -- John Mashey

Re: mlockall and mmap of IO devices don't mix

[Mark Hounschell]

Andrew Morton wrote:
> 
> Joe Korty <joe.korty@ccur.com> wrote:
> >
> > On Fri, May 21, 2004 at 07:13:26PM -0700, Andrew Morton wrote:
> >  > Mark Hounschell <markh@compro.net> wrote:
> >  > >
> >  > > Joe Korty wrote:
> >  > > >
> >  > > > 2.6.0-test6: the use of mlockall(2) in a process that has mmap(2)ed
> >  > > > the registers of an IO device will hang that process uninterruptibly.
> >  > > > The task runs in an infinite loop in get_user_pages(), invoking
> >  > > > follow_page() forever.
> >  > > >
> >  > > > Using binary search I discovered that the problem was introduced
> >  > > > in 2.5.14, specifically in ChangeSetKey
> >  > > >
> >  > > >     zippel@linux-m68k.org|ChangeSet|20020503210330|37095
> >  > > >
> >  > >
> >  > > I know this is an old thread but can anyone tell me if this problem is
> >  > > resolved in the current 2.6.6 kernel?
> >  > >
> >  >
> >  > There's an utterly ancient patch in -mm which might fix this.
> >  >
> >  > http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.6/2.6.6-mm4/broken-out/get_user_pages-handle-VM_IO.patch
> >
> >  [ 2nd send -- corporate email system in the throes of being scrambled / updated ]
> >
> >  Andrew,
> >  I have been using this patch for ages.  Any chance of it being forwared to
> >  the official tree?
> 
> That patch had its first birthday last week.  I wrote it in response to
> some long-forgotten problem, failed to changelog it at the time then forgot
> why I wrote it.  I kept it in the hope that I'd remember why I wrote it.  I
> subsequently wrote a best-effort changelog but am unconvinced by it.  Ho
> hum.
> 
> Let me genuflect a bit.  I guess we can be reasonably confident it won't
> break anything.

Strange thing about this patch is "it doesn't seem to work on all
machines". Originally it worked for me on a dual AMD1900 box. Now I'm
trying to use it on a Dual P4 box and a dual Opteron in both 32 and 64
bit modes with no luck at all???? I guess I need to go back to my
original AMD and reverify it's functionality.

The user level code I'm using:


    status = mlockall(MCL_CURRENT | MCL_FUTURE);        
    if (status < 0)
        perror("mlock all failure");

    dhan = rtom_usec_map(&rtom_microseconds, 0); mmaps rtom usec timer

?????


Regards
Mark

Re: mlockall and mmap of IO devices don't mix

[Martin J. Bligh]

>> > pfn_valid is useless, it doesn't handle all IO holes on x86 for examples.
>> 
>> Sounds like pfn_valid() is buggy on x86.  It's supposed to definitively
>> indicate whether the PFN is a valid page of ram (and has a valid struct
>> page entry.)  If it doesn't do that, the architecture implementation is
>> wrong.
> 
> Looks like it. But it also has to be fast (see include/asm-i386/mmzone.h) 
> and doesn't even hide the holes in NUMA machines. 

There are no holes between nodes for any i386 NUMA machines at the moment,
and we don't free back the struct pages for internal holes yet. So we have
pfn_valid set up for i386 such that there's a valid struct page if pfn_valid
is true.
 
> We had a page_is_ram() for this somewhere. I don't know, why this is
> gone. It would be useful in other places as well.
> 
> If the page_is_ram() test could be done using the vma only now, this
> would be even better and should be called vma_is_ram() to generalize
> these corner cases (today and in the future) and make more
> clear what these kind of tests want to do.

page_is_ram is defined in arch/i386/mm/init.c:

static inline int page_is_ram(unsigned long pagenr)
{
        int i;

        for (i = 0; i < e820.nr_map; i++) {
                unsigned long addr, end;

                if (e820.map[i].type != E820_RAM)       /* not usable memory */
                        continue;
                /*
                 *      !!!FIXME!!! Some BIOSen report areas as RAM that
                 *      are not. Notably the 640->1Mb area. We need a sanity
                 *      check here.
                 */
                addr = (e820.map[i].addr+PAGE_SIZE-1) >> PAGE_SHIFT;
                end = (e820.map[i].addr+e820.map[i].size) >> PAGE_SHIFT;
                if  ((pagenr >= addr) && (pagenr < end))
                        return 1;
        }
        return 0;
}

However, we probably want a runtime one that checks some aspect of
the struct page itself to see whether it's a valid memory page or not.

I believe it's useful to have the faster and slower tests still.
Most things using pfn_valid seem to be doing it before a pfn_to_page
translation to check it's safe, and we still seem to have that correct.
pfn_has_struct_page or something might be a better name, but still.

M.

Re: mlockall and mmap of IO devices don't mix

[Ingo Oeser]

Hi there,

CC'ed linux-mm and Andrew Morton for expertise.

On Saturday 04 October 2003 12:13, Russell King wrote:
> It has to be correct.  We do the following in a hell of a lot of places:
>
> 	pfn = pte_pfn(pte);
> 	if (pfn_valid(pfn)) {
> 		struct page *page = pfn_to_page(pfn);
> 		/* do something with page */
> 	}
>
> basically this type of thing happens in any of the PTE manipulation
> functions (eg, copy_page_range, zap_pte_range, etc.)

These functions are called always with pages, where we know, that this
is RAM, AFICS. Since sometimes other things are encoded in the PTE, whe
check this via pfn_valid().

If I'm wrong about this the gurus from LINUX-MM should complain loudly.

> If pfn_valid is returning false positives, and you happen to mmap() an
> area which gives false positives from a user space application, your
> kernel will either oops or will corrupt RAM when that application exits.
>
> I believe the comment in mmzone.h therefore is an opinion, and indicates
> a concern for performance rather than correctness and stability.

I hope you are wrong about this, but I'm not totally sure. So I included
the linux-mm mailing list and Andrew Morton for expert advice.

Regards

Ingo Oeser

Re: mlockall and mmap of IO devices don't mix

[Russell King]

On Sat, Oct 04, 2003 at 12:02:08PM +0200, Ingo Oeser wrote:
> On Saturday 04 October 2003 11:22, you wrote:
> > On Sat, Oct 04, 2003 at 11:17:03AM +0200, Andi Kleen wrote:
> > > > This check is only done, if it is a valid pfn (pfn_valid()) of a
> > > > present pte.
> > >
> > > pfn_valid is useless, it doesn't handle all IO holes on x86 for examples.
> >
> > Sounds like pfn_valid() is buggy on x86.  It's supposed to definitively
> > indicate whether the PFN is a valid page of ram (and has a valid struct
> > page entry.)  If it doesn't do that, the architecture implementation is
> > wrong.
> 
> Looks like it. But it also has to be fast (see include/asm-i386/mmzone.h) 
> and doesn't even hide the holes in NUMA machines. 

It has to be correct.  We do the following in a hell of a lot of places:

	pfn = pte_pfn(pte);
	if (pfn_valid(pfn)) {
		struct page *page = pfn_to_page(pfn);
		/* do something with page */
	}

basically this type of thing happens in any of the PTE manipulation
functions (eg, copy_page_range, zap_pte_range, etc.)

If pfn_valid is returning false positives, and you happen to mmap() an
area which gives false positives from a user space application, your
kernel will either oops or will corrupt RAM when that application exits.

I believe the comment in mmzone.h therefore is an opinion, and indicates
a concern for performance rather than correctness and stability.

-- 
Russell King (rmk@arm.linux.org.uk)	http://www.arm.linux.org.uk/personal/
      Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
      maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                      2.6 Serial core

Re: mlockall and mmap of IO devices don't mix

[Ingo Oeser]

Hi there,

On Saturday 04 October 2003 11:22, you wrote:
> On Sat, Oct 04, 2003 at 11:17:03AM +0200, Andi Kleen wrote:
> > > This check is only done, if it is a valid pfn (pfn_valid()) of a
> > > present pte.
> >
> > pfn_valid is useless, it doesn't handle all IO holes on x86 for examples.
>
> Sounds like pfn_valid() is buggy on x86.  It's supposed to definitively
> indicate whether the PFN is a valid page of ram (and has a valid struct
> page entry.)  If it doesn't do that, the architecture implementation is
> wrong.

Looks like it. But it also has to be fast (see include/asm-i386/mmzone.h) 
and doesn't even hide the holes in NUMA machines. 

We had a page_is_ram() for this somewhere. I don't know, why this is
gone. It would be useful in other places as well.

If the page_is_ram() test could be done using the vma only now, this
would be even better and should be called vma_is_ram() to generalize
these corner cases (today and in the future) and make more
clear what these kind of tests want to do.

Regards

Ingo Oeser

Re: mlockall and mmap of IO devices don't mix

[Russell King]

On Sat, Oct 04, 2003 at 11:17:03AM +0200, Andi Kleen wrote:
> > This check is only done, if it is a valid pfn (pfn_valid()) of a present
> > pte.
> 
> pfn_valid is useless, it doesn't handle all IO holes on x86 for examples.

Sounds like pfn_valid() is buggy on x86.  It's supposed to definitively
indicate whether the PFN is a valid page of ram (and has a valid struct
page entry.)  If it doesn't do that, the architecture implementation is
wrong.

-- 
Russell King (rmk@arm.linux.org.uk)	http://www.arm.linux.org.uk/personal/
      Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
      maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                      2.6 Serial core

Re: mlockall and mmap of IO devices don't mix

[Andi Kleen]

> This check is only done, if it is a valid pfn (pfn_valid()) of a present
> pte.

pfn_valid is useless, it doesn't handle all IO holes on x86 for examples.

-Andi

Re: mlockall and mmap of IO devices don't mix

[Ingo Oeser]

Hi,

On Saturday 04 October 2003 09:02, Andi Kleen wrote:
> Joe Korty <joe.korty@ccur.com> writes:
> > I do not believe that the above constitutes a correct fix.  The
> > problem is that follow_pages() is fundamentally not able to handle a
> > mapping which does not have a 'struct page' backing it up, and a
> > mapping to IO memory by definition has no 'struct page' structure to
> > back it up.
>
> The 2.4 vm scanner handles this by always checking VALID_PAGE().
>
> Maybe follow_pages() should do that too?

It does already. 

Just see the most indented check there. It tries to find a struct page from the
page frame number (pfn_to_page) after obtaining the  pfn from the pte.

This check is only done, if it is a valid pfn (pfn_valid()) of a present
pte.

Since make_pages_present uses get_user_pages(), which in turn calls
follow_pages() with the above checks, this is properly checked.

The only path where is is NOT checked, is hugetlb stuff. So the error must be
there.

If somebody does the changes, please remove the useless get_page_map(), since
it just does the above checks done already again after a reverse lookup and I
could not find ANY case, where the check in get_page_map() would fail. 

So maybe a BUG_ON() the checks in get_page_map() and a LTP run combinded with
bttv usage afterwards would be good.

Regards

Ingo Oeser

Re: mlockall and mmap of IO devices don't mix

[Andi Kleen]

> It is not a trivial thing to do now we no longer have a single mem_map[].

2.4/DISCONTIGMEM also didn't have a single mem_map and it worked there
fine.

I implemented it for x86-64. Arguably it was a bit ugly, but not too bad.

-Andi

Re: mlockall and mmap of IO devices don't mix

[Andrew Morton]

Andi Kleen <ak@muc.de> wrote:
>
> Joe Korty <joe.korty@ccur.com> writes:
> 
> >  
> > I do not believe that the above constitutes a correct fix.  The
> > problem is that follow_pages() is fundamentally not able to handle a
> > mapping which does not have a 'struct page' backing it up, and a
> > mapping to IO memory by definition has no 'struct page' structure to
> > back it up.
> 
> The 2.4 vm scanner handles this by always checking VALID_PAGE().
> 

VALID_PAGE got nuked.

It still exists in vestigial form in some architectures, but x86 does not
implement it and core kernel does not use it.

It is not a trivial thing to do now we no longer have a single mem_map[].

Re: mlockall and mmap of IO devices don't mix

[Andi Kleen]

Joe Korty <joe.korty@ccur.com> writes:

>  
> I do not believe that the above constitutes a correct fix.  The
> problem is that follow_pages() is fundamentally not able to handle a
> mapping which does not have a 'struct page' backing it up, and a
> mapping to IO memory by definition has no 'struct page' structure to
> back it up.

The 2.4 vm scanner handles this by always checking VALID_PAGE().

Maybe follow_pages() should do that too?

-Andi