Skip to contentSkip to conversation

mlockall and mmap of IO devices don't mix

PATCH Joe Korty 5 participants 18 messages Oct 3, 2003 – Jul 16, 2004

Subsystems (3)
lore
Share & tools
lore

Joe Korty

author
#0 lore raw

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

    [email protected]|ChangeSet|20020503210330|37095

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

mm/memory.c +1 -3
@@ -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:

mm/memory.c renamed from mm/memory.c.orig +3 -6
@@ -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;
}
#1 lore raw
Quoted 5 lines · 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.

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(-)
mm/memory.c renamed from mm/memory.c~get_user_pages-handle-VM_IO +11 -4
diff -puN mm/memory.c~get_user_pages-handle-VM_IO mm/memory.c
@@ -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:

_

author
#2 lore raw
Quoted 7 lines · Andrew Morton
Quoted 5 lines · 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.

Quoted 6 lines

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

#3 lore raw
Quoted 5 lines · Joe Korty
Quoted 2 lines

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?

drivers/char/mem.c renamed from drivers/char/mem.c~a +1 -1
diff -puN drivers/char/mem.c~a drivers/char/mem.c
@@ -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.

author
#4 lore raw
Quoted 2 lines

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.

drivers/char/mem.c static int mmap_mem(struct file * file, · line 179

Quoted 3 lines

Does this little experiment make it go?

drivers/char/mem.c renamed from drivers/char/mem.c~a +1 -1
-	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

#5 lore raw
In reply to Joe Korty
Quoted 2 lines · Joe Korty

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(-)
mm/memory.c renamed from mm/memory.c~get_user_pages-handle-VM_IO +11 -4
diff -puN mm/memory.c~get_user_pages-handle-VM_IO mm/memory.c
@@ -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

_

author
#6 lore raw
Quoted 3 lines · Andrew Morton
Quoted 1 line

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:

memory.c renamed from memory.c.am3 +2 -2
@@ -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

#7 lore raw
Quoted 5 lines · Joe Korty

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.

Quoted 24 lines

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

memory.c renamed from memory.c.am3 +2 -2
@@ -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:
Quoted 2 lines

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.

Quoted 2 lines

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.

#8 lore raw
Quoted 2 lines · On Fri, 3 Oct 2003 17:27:27 -0700 Andrew Morton <[email protected]> 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.

#9 lore raw
Quoted 6 lines · David S. Miller

On Fri, 3 Oct 2003 17:27:27 -0700

Quoted 2 lines · Andrew Morton

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

Quoted 6 lines

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

#10 lore raw
In reply to Joe Korty
Quoted 11 lines · 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

    [email protected]|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

#11 lore raw
Quoted 8 lines · Mark Hounschell
Quoted 6 lines · 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.

Quoted 9 lines
Quoted 5 lines

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

    [email protected]|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

#12 lore raw
Quoted 16 lines · Andrew Morton
Quoted 14 lines · Mark Hounschell
Quoted 11 lines · 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

    [email protected]|ChangeSet|20020503210330|37095
Quoted 7 lines
Quoted 3 lines

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

#13 lore raw
Quoted 22 lines · Mark Hounschell
Quoted 20 lines · Andrew Morton
Quoted 17 lines · Mark Hounschell
Quoted 11 lines · 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

    [email protected]|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?

Quoted 7 lines
Quoted 3 lines

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

author
#14 lore raw
In reply to Andrew Morton
Quoted 15 lines · Andrew Morton
Quoted 14 lines · Mark Hounschell
Quoted 11 lines · 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

    [email protected]|ChangeSet|20020503210330|37095
Quoted 7 lines
Quoted 3 lines

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

#15 lore raw
Quoted 23 lines · Joe Korty
Quoted 21 lines · Andrew Morton
Quoted 8 lines · Mark Hounschell
Quoted 6 lines · 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.

Quoted 9 lines
Quoted 5 lines

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

    [email protected]|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.

Quoted 7 lines
Quoted 1 line

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.

author
#16 lore raw
Quoted 10 lines · Andrew Morton
Quoted 9 lines
Quoted 9 lines
Quoted 7 lines
Quoted 4 lines

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.

Quoted 8 lines

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.

Signature · 2 lines
Joe
"Money can buy bandwidth, but latency is forever" -- John Mashey
#17 lore raw
In reply to Andrew Morton
Quoted 33 lines · Andrew Morton
Quoted 23 lines · Joe Korty
Quoted 21 lines · Andrew Morton
Quoted 8 lines · Mark Hounschell
Quoted 6 lines · 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.

Quoted 9 lines
Quoted 5 lines

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

    [email protected]|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.

Quoted 7 lines
Quoted 1 line

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?

Quoted 8 lines

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