diff mbox

mtd: kernel BUG at arch/x86/mm/pat.c:279!

Message ID 1348935073.2036.147.camel@shinybook.infradead.org
State New, archived
Headers show

Commit Message

David Woodhouse Sept. 29, 2012, 4:11 p.m. UTC 
On Fri, 2012-09-28 at 20:04 +0100, David Woodhouse wrote:
> > I was really hoping it would go through the regular channels and come
> > back to me that way, since I can't really test it, and it's bigger
> > than the trivial obvious one-liners that I'm happy to commit.
> 
> I can't test it on real hardware here either, but I can pull it through
> my tree. 

Hmm... I don't have access to real hardware with mmapable flash right
now (I haven't finished setting up the machine room since moving house
last month). But you're seeing this in a case where you *don't* have
mmapable flash either. If the value of map->phys is NO_XIP (-1UL), that
should be taken to mean that you can't directly mmap it at all.

That check seems to have been missing from David's commit 402d3265 in
which he introduced the mtd_mmap() operation, and wasn't fixed in commit
dd02b67d5 where Anatolij fixed things to actually *work* in the MMU code
path. This should fix it:

 		if ((vma->vm_end - vma->vm_start + off) > len)

Now, we should ideally *also* handle the case where there genuinely *is*
a mappable ROM device at the very top of physical memory, and not suffer
from overflow. But that's a more esoteric case, and the patch above
seems to be the one that we want to get merged into -stable.

Comments

David Woodhouse Sept. 29, 2012, 4:34 p.m. UTC | #1 
On Sat, 2012-09-29 at 17:11 +0100, David Woodhouse wrote:
> 
> That check seems to have been missing from David's commit 402d3265 in
> which he introduced the mtd_mmap() operation, and wasn't fixed in commit
> dd02b67d5 where Anatolij fixed things to actually *work* in the MMU code
> path. This should fix it:

> +               if (map->phys == NO_XIP)
> +                       return -EINVAL; 

Hm, but there's another problem. That 'map' variable is pulled from
mtd->priv but there's a clue in the name 'priv'.... it isn't guaranteed
to *be* a device that goes through the map abstraction.

Anatolij? Your patch dd02b67d5 might have worked on your test case, but
it fails disgracefully in any of the cases where it *isn't* expected to
work.

I think it needs to use mtd_unmapped_area() on the device in question
just like the !CONFIG_MMU code path does, and avoid grubbing around in
things that it shouldn't be looking at directly.

David, you made mtd_unmapped_area() return the *virtual* address... but
there's no reason that couldn't have been the physical address, right?
You were only using it in the !CONFIG_MMU case anyway, where they're
equal.

In the meantime, I think the quick fix is just to disable mtdchar_mmap
in the CONFIG_MMU case. It was broken from the moment David introduced
it, and Anatolij's fix was insufficient. I'll do that.
diff mbox

Patch

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index f2f482b..27c2f57 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -1137,8 +1137,10 @@  static int mtdchar_mmap(struct file *file, struct
vm_area_struct *vma)
 	u32 len;
 
 	if (mtd->type == MTD_RAM || mtd->type == MTD_ROM) {
-		off = vma->vm_pgoff << PAGE_SHIFT;
 		start = map->phys;
+		if (map->phys == NO_XIP)
+			return -EINVAL;
+		off = vma->vm_pgoff << PAGE_SHIFT;
 		len = PAGE_ALIGN((start & ~PAGE_MASK) + map->size);
 		start &= PAGE_MASK;