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

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

[Sasha Levin]

Hi all,

I've stumbled on the following while fuzzing with trinity in a KVM tools guest using latest linux-next:

[ 3299.675163] ------------[ cut here ]------------
[ 3299.676027] kernel BUG at arch/x86/mm/pat.c:279!
[ 3299.676027] invalid opcode: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
[ 3299.678596] CPU 2 
[ 3299.678596] Pid: 21541, comm: trinity-child6 Tainted: G        W    3.5.0-rc4-next-20120628-sasha-00005-g9f23eb7 #479  
[ 3299.678596] RIP: 0010:[<ffffffff810a8b62>]  [<ffffffff810a8b62>] reserve_memtype+0x22/0x3d0
[ 3299.678596] RSP: 0018:ffff88000ad61bc8  EFLAGS: 00010286
[ 3299.678596] RAX: 0000000000000000 RBX: fffffffffffff000 RCX: ffff88000ad61c50
[ 3299.678596] RDX: 0000000000000010 RSI: 0000000000000000 RDI: fffffffffffff000
[ 3299.696632] RBP: ffff88000ad61c08 R08: 0000000000000010 R09: ffff88002617d5a8
[ 3299.696632] R10: ffff88003111edc8 R11: 0000000000000001 R12: ffff88000ad61c50
[ 3299.696632] R13: fffffffffffff000 R14: 0000000000000000 R15: ffff88000ad61d18
[ 3299.696632] FS:  00007f3ffc3aa700(0000) GS:ffff880029800000(0000) knlGS:0000000000000000
[ 3299.696632] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 3299.696632] CR2: 0000000000f73ffc CR3: 000000000ad6e000 CR4: 00000000000406e0
[ 3299.696632] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 3299.696632] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 3299.696632] Process trinity-child6 (pid: 21541, threadinfo ffff88000ad60000, task ffff88000a390000)
[ 3299.696632] Stack:
[ 3299.696632]  ffff88000ad61c18 ffffffff81161bc6 ffff88000ad61c18 fffffffffffff000
[ 3299.696632]  0000000000000010 0000000000000000 0000000000001000 ffff88000ad61d18
[ 3299.696632]  ffff88000ad61c88 ffffffff810a8fe2 ffff88000ad61c38 0000000000000086
[ 3299.696632] Call Trace:
[ 3299.696632]  [<ffffffff81161bc6>] ? mark_held_locks+0xf6/0x120
[ 3299.696632]  [<ffffffff810a8fe2>] reserve_pfn_range+0xd2/0x1e0
[ 3299.696632]  [<ffffffff810a912d>] track_pfn_vma_new+0x3d/0x80
[ 3299.696632]  [<ffffffff8120c4bc>] remap_pfn_range+0xac/0x380
[ 3299.696632]  [<ffffffff8220e016>] mtdchar_mmap+0xe6/0x100
[ 3299.696632]  [<ffffffff812145ae>] mmap_region+0x35e/0x5f0
[ 3299.696632]  [<ffffffff81214af9>] do_mmap_pgoff+0x2b9/0x350
[ 3299.696632]  [<ffffffff811ff46c>] ? vm_mmap_pgoff+0x6c/0xb0
[ 3299.696632]  [<ffffffff811ff484>] vm_mmap_pgoff+0x84/0xb0
[ 3299.696632]  [<ffffffff8124fd80>] ? fget_raw+0x260/0x260
[ 3299.696632]  [<ffffffff81211fde>] sys_mmap_pgoff+0x15e/0x190
[ 3299.696632]  [<ffffffff81985ede>] ? trace_hardirqs_on_thunk+0x3a/0x3f
[ 3299.696632]  [<ffffffff8106d4dd>] sys_mmap+0x1d/0x20
[ 3299.696632]  [<ffffffff8372a539>] system_call_fastpath+0x16/0x1b
[ 3299.696632] Code: 28 5b c9 c3 0f 1f 44 00 00 55 49 89 d0 48 89 e5 41 57 41 56 49 89 f6 41 55 49 89 fd 41 54 49 89 cc 53 48 83 ec 18 48 39 f7 72 0e <0f> 0b 0f 1f 40 00 eb fe 66 0f 1f 44 00 00 8b 3d 1a 5b e3 03 85 
[ 3299.696632] RIP  [<ffffffff810a8b62>] reserve_memtype+0x22/0x3d0
[ 3299.696632]  RSP <ffff88000ad61bc8>

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

[Sasha Levin]

Ping?

On Fri, Jun 29, 2012 at 10:48 AM, Sasha Levin <levinsasha928@gmail.com> wrote:
> Hi all,
>
> I've stumbled on the following while fuzzing with trinity in a KVM tools guest using latest linux-next:
>
> [ 3299.675163] ------------[ cut here ]------------
> [ 3299.676027] kernel BUG at arch/x86/mm/pat.c:279!
> [ 3299.676027] invalid opcode: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
> [ 3299.678596] CPU 2
> [ 3299.678596] Pid: 21541, comm: trinity-child6 Tainted: G        W    3.5.0-rc4-next-20120628-sasha-00005-g9f23eb7 #479
> [ 3299.678596] RIP: 0010:[<ffffffff810a8b62>]  [<ffffffff810a8b62>] reserve_memtype+0x22/0x3d0
> [ 3299.678596] RSP: 0018:ffff88000ad61bc8  EFLAGS: 00010286
> [ 3299.678596] RAX: 0000000000000000 RBX: fffffffffffff000 RCX: ffff88000ad61c50
> [ 3299.678596] RDX: 0000000000000010 RSI: 0000000000000000 RDI: fffffffffffff000
> [ 3299.696632] RBP: ffff88000ad61c08 R08: 0000000000000010 R09: ffff88002617d5a8
> [ 3299.696632] R10: ffff88003111edc8 R11: 0000000000000001 R12: ffff88000ad61c50
> [ 3299.696632] R13: fffffffffffff000 R14: 0000000000000000 R15: ffff88000ad61d18
> [ 3299.696632] FS:  00007f3ffc3aa700(0000) GS:ffff880029800000(0000) knlGS:0000000000000000
> [ 3299.696632] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 3299.696632] CR2: 0000000000f73ffc CR3: 000000000ad6e000 CR4: 00000000000406e0
> [ 3299.696632] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [ 3299.696632] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> [ 3299.696632] Process trinity-child6 (pid: 21541, threadinfo ffff88000ad60000, task ffff88000a390000)
> [ 3299.696632] Stack:
> [ 3299.696632]  ffff88000ad61c18 ffffffff81161bc6 ffff88000ad61c18 fffffffffffff000
> [ 3299.696632]  0000000000000010 0000000000000000 0000000000001000 ffff88000ad61d18
> [ 3299.696632]  ffff88000ad61c88 ffffffff810a8fe2 ffff88000ad61c38 0000000000000086
> [ 3299.696632] Call Trace:
> [ 3299.696632]  [<ffffffff81161bc6>] ? mark_held_locks+0xf6/0x120
> [ 3299.696632]  [<ffffffff810a8fe2>] reserve_pfn_range+0xd2/0x1e0
> [ 3299.696632]  [<ffffffff810a912d>] track_pfn_vma_new+0x3d/0x80
> [ 3299.696632]  [<ffffffff8120c4bc>] remap_pfn_range+0xac/0x380
> [ 3299.696632]  [<ffffffff8220e016>] mtdchar_mmap+0xe6/0x100
> [ 3299.696632]  [<ffffffff812145ae>] mmap_region+0x35e/0x5f0
> [ 3299.696632]  [<ffffffff81214af9>] do_mmap_pgoff+0x2b9/0x350
> [ 3299.696632]  [<ffffffff811ff46c>] ? vm_mmap_pgoff+0x6c/0xb0
> [ 3299.696632]  [<ffffffff811ff484>] vm_mmap_pgoff+0x84/0xb0
> [ 3299.696632]  [<ffffffff8124fd80>] ? fget_raw+0x260/0x260
> [ 3299.696632]  [<ffffffff81211fde>] sys_mmap_pgoff+0x15e/0x190
> [ 3299.696632]  [<ffffffff81985ede>] ? trace_hardirqs_on_thunk+0x3a/0x3f
> [ 3299.696632]  [<ffffffff8106d4dd>] sys_mmap+0x1d/0x20
> [ 3299.696632]  [<ffffffff8372a539>] system_call_fastpath+0x16/0x1b
> [ 3299.696632] Code: 28 5b c9 c3 0f 1f 44 00 00 55 49 89 d0 48 89 e5 41 57 41 56 49 89 f6 41 55 49 89 fd 41 54 49 89 cc 53 48 83 ec 18 48 39 f7 72 0e <0f> 0b 0f 1f 40 00 eb fe 66 0f 1f 44 00 00 8b 3d 1a 5b e3 03 85
> [ 3299.696632] RIP  [<ffffffff810a8b62>] reserve_memtype+0x22/0x3d0
> [ 3299.696632]  RSP <ffff88000ad61bc8>
>

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

[Sasha Levin]

Ping? Still seeing this with latest master...

On Fri, Jun 29, 2012 at 10:48 AM, Sasha Levin <levinsasha928@gmail.com> wrote:
> Hi all,
>
> I've stumbled on the following while fuzzing with trinity in a KVM tools guest using latest linux-next:
>
> [ 3299.675163] ------------[ cut here ]------------
> [ 3299.676027] kernel BUG at arch/x86/mm/pat.c:279!
> [ 3299.676027] invalid opcode: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
> [ 3299.678596] CPU 2
> [ 3299.678596] Pid: 21541, comm: trinity-child6 Tainted: G        W    3.5.0-rc4-next-20120628-sasha-00005-g9f23eb7 #479
> [ 3299.678596] RIP: 0010:[<ffffffff810a8b62>]  [<ffffffff810a8b62>] reserve_memtype+0x22/0x3d0
> [ 3299.678596] RSP: 0018:ffff88000ad61bc8  EFLAGS: 00010286
> [ 3299.678596] RAX: 0000000000000000 RBX: fffffffffffff000 RCX: ffff88000ad61c50
> [ 3299.678596] RDX: 0000000000000010 RSI: 0000000000000000 RDI: fffffffffffff000
> [ 3299.696632] RBP: ffff88000ad61c08 R08: 0000000000000010 R09: ffff88002617d5a8
> [ 3299.696632] R10: ffff88003111edc8 R11: 0000000000000001 R12: ffff88000ad61c50
> [ 3299.696632] R13: fffffffffffff000 R14: 0000000000000000 R15: ffff88000ad61d18
> [ 3299.696632] FS:  00007f3ffc3aa700(0000) GS:ffff880029800000(0000) knlGS:0000000000000000
> [ 3299.696632] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 3299.696632] CR2: 0000000000f73ffc CR3: 000000000ad6e000 CR4: 00000000000406e0
> [ 3299.696632] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [ 3299.696632] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> [ 3299.696632] Process trinity-child6 (pid: 21541, threadinfo ffff88000ad60000, task ffff88000a390000)
> [ 3299.696632] Stack:
> [ 3299.696632]  ffff88000ad61c18 ffffffff81161bc6 ffff88000ad61c18 fffffffffffff000
> [ 3299.696632]  0000000000000010 0000000000000000 0000000000001000 ffff88000ad61d18
> [ 3299.696632]  ffff88000ad61c88 ffffffff810a8fe2 ffff88000ad61c38 0000000000000086
> [ 3299.696632] Call Trace:
> [ 3299.696632]  [<ffffffff81161bc6>] ? mark_held_locks+0xf6/0x120
> [ 3299.696632]  [<ffffffff810a8fe2>] reserve_pfn_range+0xd2/0x1e0
> [ 3299.696632]  [<ffffffff810a912d>] track_pfn_vma_new+0x3d/0x80
> [ 3299.696632]  [<ffffffff8120c4bc>] remap_pfn_range+0xac/0x380
> [ 3299.696632]  [<ffffffff8220e016>] mtdchar_mmap+0xe6/0x100
> [ 3299.696632]  [<ffffffff812145ae>] mmap_region+0x35e/0x5f0
> [ 3299.696632]  [<ffffffff81214af9>] do_mmap_pgoff+0x2b9/0x350
> [ 3299.696632]  [<ffffffff811ff46c>] ? vm_mmap_pgoff+0x6c/0xb0
> [ 3299.696632]  [<ffffffff811ff484>] vm_mmap_pgoff+0x84/0xb0
> [ 3299.696632]  [<ffffffff8124fd80>] ? fget_raw+0x260/0x260
> [ 3299.696632]  [<ffffffff81211fde>] sys_mmap_pgoff+0x15e/0x190
> [ 3299.696632]  [<ffffffff81985ede>] ? trace_hardirqs_on_thunk+0x3a/0x3f
> [ 3299.696632]  [<ffffffff8106d4dd>] sys_mmap+0x1d/0x20
> [ 3299.696632]  [<ffffffff8372a539>] system_call_fastpath+0x16/0x1b
> [ 3299.696632] Code: 28 5b c9 c3 0f 1f 44 00 00 55 49 89 d0 48 89 e5 41 57 41 56 49 89 f6 41 55 49 89 fd 41 54 49 89 cc 53 48 83 ec 18 48 39 f7 72 0e <0f> 0b 0f 1f 40 00 eb fe 66 0f 1f 44 00 00 8b 3d 1a 5b e3 03 85
> [ 3299.696632] RIP  [<ffffffff810a8b62>] reserve_memtype+0x22/0x3d0
> [ 3299.696632]  RSP <ffff88000ad61bc8>
>

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

[Linus Torvalds]

Guys, this looks like a MTD and/or io_remap_pfn_range() bug, and it's
not getting any traction.

What the f*ck is mtd_mmap() doing, and why? The problem seems to be an
overflow condition, because reserve_pfn_range() does

    reserve_memtype(paddr, paddr + size, want_flags, &flags);

and then the BUG_ON() in reserve_memtype is

    BUG_ON(start >= end);

so it very much looks like a paddr+size overflow. However, that makes
little sense too, since we're working in "u64", so I suspect the
overflow has happened somewhere earlier.

I really don't see where, though. Could somebody please take a look?
The mtdchar_mmap() types seem insane (why "u32" for len, for example?
And that whole

  off = vma->vm_pgoff << PAGE_SHIFT;

thing looks like it would overflow, since the whole point of pgoff is
that if you shift it up by PAGE_SHIFT you need to also extend to
64-bit etc.

So I would *guess* that it's the mtdchar_mmap() stuff that overflows
due to bad types, but maybe it does deeper than that?

                         Linus

On Fri, Sep 7, 2012 at 9:55 AM, Sasha Levin <levinsasha928@gmail.com> wrote:
> Ping? Still seeing this with latest master...
>
> On Fri, Jun 29, 2012 at 10:48 AM, Sasha Levin <levinsasha928@gmail.com> wrote:
>> Hi all,
>>
>> I've stumbled on the following while fuzzing with trinity in a KVM tools guest using latest linux-next:
>>
>> [ 3299.675163] ------------[ cut here ]------------
>> [ 3299.676027] kernel BUG at arch/x86/mm/pat.c:279!
>> [ 3299.676027] invalid opcode: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
>> [ 3299.678596] CPU 2
>> [ 3299.678596] Pid: 21541, comm: trinity-child6 Tainted: G        W    3.5.0-rc4-next-20120628-sasha-00005-g9f23eb7 #479
>> [ 3299.678596] RIP: 0010:[<ffffffff810a8b62>]  [<ffffffff810a8b62>] reserve_memtype+0x22/0x3d0
>> [ 3299.678596] RSP: 0018:ffff88000ad61bc8  EFLAGS: 00010286
>> [ 3299.678596] RAX: 0000000000000000 RBX: fffffffffffff000 RCX: ffff88000ad61c50
>> [ 3299.678596] RDX: 0000000000000010 RSI: 0000000000000000 RDI: fffffffffffff000
>> [ 3299.696632] RBP: ffff88000ad61c08 R08: 0000000000000010 R09: ffff88002617d5a8
>> [ 3299.696632] R10: ffff88003111edc8 R11: 0000000000000001 R12: ffff88000ad61c50
>> [ 3299.696632] R13: fffffffffffff000 R14: 0000000000000000 R15: ffff88000ad61d18
>> [ 3299.696632] FS:  00007f3ffc3aa700(0000) GS:ffff880029800000(0000) knlGS:0000000000000000
>> [ 3299.696632] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [ 3299.696632] CR2: 0000000000f73ffc CR3: 000000000ad6e000 CR4: 00000000000406e0
>> [ 3299.696632] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>> [ 3299.696632] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
>> [ 3299.696632] Process trinity-child6 (pid: 21541, threadinfo ffff88000ad60000, task ffff88000a390000)
>> [ 3299.696632] Stack:
>> [ 3299.696632]  ffff88000ad61c18 ffffffff81161bc6 ffff88000ad61c18 fffffffffffff000
>> [ 3299.696632]  0000000000000010 0000000000000000 0000000000001000 ffff88000ad61d18
>> [ 3299.696632]  ffff88000ad61c88 ffffffff810a8fe2 ffff88000ad61c38 0000000000000086
>> [ 3299.696632] Call Trace:
>> [ 3299.696632]  [<ffffffff81161bc6>] ? mark_held_locks+0xf6/0x120
>> [ 3299.696632]  [<ffffffff810a8fe2>] reserve_pfn_range+0xd2/0x1e0
>> [ 3299.696632]  [<ffffffff810a912d>] track_pfn_vma_new+0x3d/0x80
>> [ 3299.696632]  [<ffffffff8120c4bc>] remap_pfn_range+0xac/0x380
>> [ 3299.696632]  [<ffffffff8220e016>] mtdchar_mmap+0xe6/0x100
>> [ 3299.696632]  [<ffffffff812145ae>] mmap_region+0x35e/0x5f0
>> [ 3299.696632]  [<ffffffff81214af9>] do_mmap_pgoff+0x2b9/0x350
>> [ 3299.696632]  [<ffffffff811ff46c>] ? vm_mmap_pgoff+0x6c/0xb0
>> [ 3299.696632]  [<ffffffff811ff484>] vm_mmap_pgoff+0x84/0xb0
>> [ 3299.696632]  [<ffffffff8124fd80>] ? fget_raw+0x260/0x260
>> [ 3299.696632]  [<ffffffff81211fde>] sys_mmap_pgoff+0x15e/0x190
>> [ 3299.696632]  [<ffffffff81985ede>] ? trace_hardirqs_on_thunk+0x3a/0x3f
>> [ 3299.696632]  [<ffffffff8106d4dd>] sys_mmap+0x1d/0x20
>> [ 3299.696632]  [<ffffffff8372a539>] system_call_fastpath+0x16/0x1b
>> [ 3299.696632] Code: 28 5b c9 c3 0f 1f 44 00 00 55 49 89 d0 48 89 e5 41 57 41 56 49 89 f6 41 55 49 89 fd 41 54 49 89 cc 53 48 83 ec 18 48 39 f7 72 0e <0f> 0b 0f 1f 40 00 eb fe 66 0f 1f 44 00 00 8b 3d 1a 5b e3 03 85
>> [ 3299.696632] RIP  [<ffffffff810a8b62>] reserve_memtype+0x22/0x3d0
>> [ 3299.696632]  RSP <ffff88000ad61bc8>
>>

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

[Suresh Siddha]

On Fri, 2012-09-07 at 11:14 -0700, Linus Torvalds wrote:
> Guys, this looks like a MTD and/or io_remap_pfn_range() bug, and it's
> not getting any traction.
> 
> What the f*ck is mtd_mmap() doing, and why? The problem seems to be an
> overflow condition, because reserve_pfn_range() does
> 
>     reserve_memtype(paddr, paddr + size, want_flags, &flags);
> 
> and then the BUG_ON() in reserve_memtype is
> 
>     BUG_ON(start >= end);
> 
> so it very much looks like a paddr+size overflow. However, that makes
> little sense too, since we're working in "u64", so I suspect the
> overflow has happened somewhere earlier.
> 
> I really don't see where, though. Could somebody please take a look?
> The mtdchar_mmap() types seem insane (why "u32" for len, for example?
> And that whole
> 
>   off = vma->vm_pgoff << PAGE_SHIFT;
> 
> thing looks like it would overflow, since the whole point of pgoff is
> that if you shift it up by PAGE_SHIFT you need to also extend to
> 64-bit etc.
> 
> So I would *guess* that it's the mtdchar_mmap() stuff that overflows
> due to bad types, but maybe it does deeper than that?
> 

I started to look into this to see if this is a PAT issue but it does
indeed appear to be a mtd mmap issue.

Sasha, Does the appended fix the issue for you?

--8<--
From: Suresh Siddha <suresh.b.siddha@intel.com>
Subject: mtd: check the starting offset to be mmap'd

We need to check if both the starting offset aswell the total length
being mmap'd are with in the limits. With a large starting offset,
offset + (length-to-be-mapped) can wrap and appear smaller than the
limit. Need to check both start and end.

Also fix the types of the variables start, off, len.

Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
---
 drivers/mtd/mtdchar.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index f2f482b..f79c0fa 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -1132,16 +1132,15 @@ static int mtdchar_mmap(struct file *file, struct vm_area_struct *vma)
 	struct mtd_file_info *mfi = file->private_data;
 	struct mtd_info *mtd = mfi->mtd;
 	struct map_info *map = mtd->priv;
-	unsigned long start;
-	unsigned long off;
-	u32 len;
+	resource_size_t start, off;
+	unsigned long len;
 
 	if (mtd->type == MTD_RAM || mtd->type == MTD_ROM) {
 		off = vma->vm_pgoff << PAGE_SHIFT;
 		start = map->phys;
 		len = PAGE_ALIGN((start & ~PAGE_MASK) + map->size);
 		start &= PAGE_MASK;
-		if ((vma->vm_end - vma->vm_start + off) > len)
+		if (off >= len || (vma->vm_end - vma->vm_start + off) > len)
 			return -EINVAL;
 
 		off += start;

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

[Linus Torvalds]

On Fri, Sep 7, 2012 at 3:42 PM, Suresh Siddha <suresh.b.siddha@intel.com> wrote:
> -       unsigned long start;
> -       unsigned long off;
> -       u32 len;
> +       resource_size_t start, off;
> +       unsigned long len;

So since the oops is on x86-64, I don't think it's the "unsigned long"
-> "resource_size_t" part (which can be an issue on 32-bit
architectures, though).

The "u32 len" -> "unsigned long len" thing *might* make a difference, though.

I also think your patch is incomplete even on 32-bit, because this:

>         if (mtd->type == MTD_RAM || mtd->type == MTD_ROM) {
>                 off = vma->vm_pgoff << PAGE_SHIFT;

is still wrong. It probably should be

    off = vma->vm_pgoff;
    off <<= PAGE_SHIFT;

because vm_pgoff may be a 32-bit type, while "resource_size_t" may be
64-bit. Shifting the 32-bit type without a cast (implicit or explicit)
isn't going to help.

That said, we have absolutely *tons* of bugs with this particular
pattern. Just do

    git grep 'vm_pgoff.*<<.*PAGE_SHIFT'

and there are distressingly few casts in there (there's a few, mainly
in fs/proc).

Now, I suspect many of them are fine just because most users probably
are size-limited anyway, but it's a bit distressing stuff. And I
suspect it means we might want to introduce a helper function like

    static inline u64 vm_offset(struct vm_area_struct *vma)
    {
        return (u64)vma->vm_pgoff << PAGE_SHIFT;
    }

or something. Maybe add the "vm_length()" helper while at it too,
since the whole "vma->vm_end - vma->vm_start" thing is so common.

Anyway, since Sasha's oops is clearly not 32-bit, the above issues
don't matter, and it would be interesting to hear if it's the 32-bit
'len' thing that triggers this problem. Still, I can't see how it
would - as far as I can tell, a truncated 'len' would at most result
in spurious early "return -EINVAL", not any real problem.

What are we missing?

Sasha, since you can apparently reproduce it, can you replace the
"BUG_ON()" with just a

 if (start >= end) {
    printf("bogus range %llx - %llx\n", start, end);
    return -EINVAL;
  }

or something.

I'm starting to suspect that maybe it's actually that the length is
*zero*, and start == end, and that we should just return zero for that
case. But let's see what Sasha finds..

              Linus

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

[Suresh Siddha]

On Fri, 2012-09-07 at 16:09 -0700, Linus Torvalds wrote:
> The "u32 len" -> "unsigned long len" thing *might* make a difference, though.

This I believe doesn't fix the reported BUG. I was trying to address
your previous comment about broken types.

> 
> I also think your patch is incomplete even on 32-bit, because this:
> 
> >         if (mtd->type == MTD_RAM || mtd->type == MTD_ROM) {
> >                 off = vma->vm_pgoff << PAGE_SHIFT;
> 
> is still wrong. It probably should be
> 
>     off = vma->vm_pgoff;
>     off <<= PAGE_SHIFT;
> 
> because vm_pgoff may be a 32-bit type, while "resource_size_t" may be
> 64-bit. Shifting the 32-bit type without a cast (implicit or explicit)
> isn't going to help.

Agree.

> That said, we have absolutely *tons* of bugs with this particular
> pattern. Just do
> 
>     git grep 'vm_pgoff.*<<.*PAGE_SHIFT'
> 
> and there are distressingly few casts in there (there's a few, mainly
> in fs/proc).
> 
> Now, I suspect many of them are fine just because most users probably
> are size-limited anyway, but it's a bit distressing stuff. And I
> suspect it means we might want to introduce a helper function like
> 
>     static inline u64 vm_offset(struct vm_area_struct *vma)
>     {
>         return (u64)vma->vm_pgoff << PAGE_SHIFT;
>     }
> 
> or something. Maybe add the "vm_length()" helper while at it too,
> since the whole "vma->vm_end - vma->vm_start" thing is so common.

Agree.

> Anyway, since Sasha's oops is clearly not 32-bit, the above issues
> don't matter, and it would be interesting to hear if it's the 32-bit
> 'len' thing that triggers this problem. Still, I can't see how it
> would - as far as I can tell, a truncated 'len' would at most result
> in spurious early "return -EINVAL", not any real problem.
> 
> What are we missing?
> 

On Fri, 2012-09-07 at 15:42 -0700, Suresh Siddha wrote:
> -               if ((vma->vm_end - vma->vm_start + off) > len)
> +               if (off >= len || (vma->vm_end - vma->vm_start + off) > len)
>                         return -EINVAL; 

This is the relevant portion that I am thinking will address the BUG.

Essentially the user is trying to mmap at a very large offset (from the
oops it appears "vma->vm_pgoff << PAGE_SHIFT + start" ends up to
"0xfffffffffffff000").

So it appears that the condition "(vma->vm_end - vma->vm_start + off) >
len" might be false because of the wraparound? and doesn't return
-EINVAL.

Let's see what Sasha finds. Anyways the patch does indeed require your
above mentioned vm_pgoff fix for the 32-bit case.

thanks,
suresh

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

[Sasha Levin]

On 09/08/2012 01:09 AM, Linus Torvalds wrote:
> Sasha, since you can apparently reproduce it, can you replace the
> "BUG_ON()" with just a
> 
>  if (start >= end) {
>     printf("bogus range %llx - %llx\n", start, end);
>     return -EINVAL;
>   }

Replacing it gives me the following:

[   36.231736] bogus range fffffffffffff000 - 0


Thanks,
Sasha

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

[Linus Torvalds]

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

On Fri, Sep 7, 2012 at 4:54 PM, Suresh Siddha <suresh.b.siddha@intel.com> wrote:
>
> Essentially the user is trying to mmap at a very large offset (from the
> oops it appears "vma->vm_pgoff << PAGE_SHIFT + start" ends up to
> "0xfffffffffffff000").

Ok, Sasha confirmed that.

> So it appears that the condition "(vma->vm_end - vma->vm_start + off) >
> len" might be false because of the wraparound? and doesn't return
> -EINVAL.

Ack.

Anyway, that means that the BUG_ON() is likely bogus, but so is the
whole calling convention.

The 4kB range starting at 0xfffffffffffff000 sounds like a *valid*
range, but that requires that we fix the calling convention to not
have that "end" (exclusive) thing. It should either be "end"
(inclusive), or just "len".

So it should either be start=0xfffffffffffff000 end=0xffffffffffffffff
or it should be start=0xfffffffffffff000 len=0x1000.

Or we need to say that we must never accept things at the end of the
64-bit range.

Whatever. Something like this (TOTALLY UNTESTED) attached patch should
get the mtdchar overflows to go away, but it does *not* fix the fact
that the MTRR start/end model is broken. It really is technically
valid to have a resource_size_t range of 0xfffffffffffff000+0x1000,
and right now it causes a BUG_ON() in pat.c.

Suresh?

                    Linus

[-- Attachment #2: patch.diff --]
[-- Type: application/octet-stream, Size: 2201 bytes --]

 drivers/mtd/mtdchar.c | 48 ++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 42 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index f2f482bec573..83dd68b80e8e 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -1123,6 +1123,33 @@ static unsigned long mtdchar_get_unmapped_area(struct file *file,
 }
 #endif
 
+static inline unsigned long get_vm_size(struct vm_area_struct *vma)
+{
+	return vma->vm_end - vma->vm_start;
+}
+
+static inline resource_size_t get_vm_offset(struct vm_area_struct *vma)
+{
+	return (resource_size_t) vma->vm_pgoff << PAGE_SHIFT;
+}
+
+/*
+ * Set a new vm offset.
+ *
+ * Verify that the incoming offset really works as a page offset,
+ * and that the offset and size fit in a resource_size_t.
+ */
+static inlint int set_vm_offset(struct vm_area_struct *vma, resource_size_t off)
+{
+	pgoff_t pgoff = off >> PAGE_SHIFT;
+	if (off != (resource_size_t) pgoff << PAGE_SHIFT)
+		return -EINVAL;
+	if (off + get_vm_size(vma) - 1 < off)
+		return -EINVAL;
+	vma->vm_pgoff = pgoff;
+	return 0;
+}
+
 /*
  * set up a mapping for shared memory segments
  */
@@ -1132,20 +1159,29 @@ static int mtdchar_mmap(struct file *file, struct vm_area_struct *vma)
 	struct mtd_file_info *mfi = file->private_data;
 	struct mtd_info *mtd = mfi->mtd;
 	struct map_info *map = mtd->priv;
-	unsigned long start;
-	unsigned long off;
-	u32 len;
+	resource_size_t start, off;
+	unsigned long len, vma_len;
 
 	if (mtd->type == MTD_RAM || mtd->type == MTD_ROM) {
-		off = vma->vm_pgoff << PAGE_SHIFT;
+		off = get_vm_offset(vma);
 		start = map->phys;
 		len = PAGE_ALIGN((start & ~PAGE_MASK) + map->size);
 		start &= PAGE_MASK;
-		if ((vma->vm_end - vma->vm_start + off) > len)
+		vma_len = get_vm_size(vma);
+
+		/* Overflow in off+len? */
+		if (vma_len + off < off)
+			return -EINVAL;
+		/* Does it fit in the mapping? */
+		if (vma_len + off > len)
 			return -EINVAL;
 
 		off += start;
-		vma->vm_pgoff = off >> PAGE_SHIFT;
+		/* Did that overflow? */
+		if (off < start)
+			return -EINVAL;
+		if (set_vm_offset(vma, off) < 0)
+			return -EINVAL;
 		vma->vm_flags |= VM_IO | VM_RESERVED;
 
 #ifdef pgprot_noncached

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

[Suresh Siddha]

On Sat, 2012-09-08 at 12:57 -0700, Linus Torvalds wrote:
> Whatever. Something like this (TOTALLY UNTESTED) attached patch should
> get the mtdchar overflows to go away, 

It looks good to me. Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>

Sasha, can you please give this a try?

> but it does *not* fix the fact
> that the MTRR start/end model is broken. It really is technically
> valid to have a resource_size_t range of 0xfffffffffffff000+0x1000,
> and right now it causes a BUG_ON() in pat.c.
> 
> Suresh?

yes but that is not a valid range I think because of the supported
physical address bit limits of the processor and also the max
architecture limit of 52 address bits.

I guess we should be checking for those limits in pat.c, especially bits
above 52 are ignored by the HW and they can easily cause conflicting
aliases with other valid regions. I will get back with a different patch
to fix this.

thanks,
suresh

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

[Linus Torvalds]

On Sun, Sep 9, 2012 at 7:56 AM, Suresh Siddha <suresh.b.siddha@intel.com> wrote:
>
> yes but that is not a valid range I think because of the supported
> physical address bit limits of the processor and also the max
> architecture limit of 52 address bits.

But how could the caller possibly know that? None of those internal
PAT limits are exposed anywhere.

So doing the BUG_ON() is wrong. I'd suggest changing it to an EINVAL.

In fact, BUG_ON() is *always* wrong, unless it's a "my internal data
structures are so messed up that I cannot continue".

                Linus

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

[H. Peter Anvin]

On 09/08/2012 12:57 PM, Linus Torvalds wrote:
>
> Ack.
>
> Anyway, that means that the BUG_ON() is likely bogus, but so is the
> whole calling convention.
>
> The 4kB range starting at 0xfffffffffffff000 sounds like a *valid*
> range, but that requires that we fix the calling convention to not
> have that "end" (exclusive) thing. It should either be "end"
> (inclusive), or just "len".
>

On x86, it is definitely NOT a valid range.  There is no physical 
addresses there, and there will never be any.

> So it should either be start=0xfffffffffffff000 end=0xffffffffffffffff
> or it should be start=0xfffffffffffff000 len=0x1000.

I would strongly object to the former; that kind of inclusive ranges 
breed a whole class of bugs by themselves.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

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

[H. Peter Anvin]

On 09/09/2012 08:31 AM, Linus Torvalds wrote:
> On Sun, Sep 9, 2012 at 7:56 AM, Suresh Siddha <suresh.b.siddha@intel.com> wrote:
>>
>> yes but that is not a valid range I think because of the supported
>> physical address bit limits of the processor and also the max
>> architecture limit of 52 address bits.
>
> But how could the caller possibly know that? None of those internal
> PAT limits are exposed anywhere.
>
> So doing the BUG_ON() is wrong. I'd suggest changing it to an EINVAL.
>
> In fact, BUG_ON() is *always* wrong, unless it's a "my internal data
> structures are so messed up that I cannot continue".
>

I suspect the right answer is doing something like:

	u64 max_phys = 1ULL << boot_cpu_data.x86_phys_bits;

	if (start >= max_phys || end > max_phys || start >= end)
		return -EINVAL;

... although max_phys perhaps should be precalculated and stored in 
struct cpuinfo_x86 instead of being generated de novo.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

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

[David Woodhouse]

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

On Sun, 2012-09-09 at 09:56 -0700, H. Peter Anvin wrote:
> 
> > So it should either be start=0xfffffffffffff000 end=0xffffffffffffffff
> > or it should be start=0xfffffffffffff000 len=0x1000.
> 
> I would strongly object to the former; that kind of inclusive ranges 
> breed a whole class of bugs by themselves.

Another alternative that avoids overflow issues is to use a PFN rather
than a byte address.

-- 
dwmw2

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

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

[H. Peter Anvin]

On 09/09/2012 12:04 PM, David Woodhouse wrote:
> On Sun, 2012-09-09 at 09:56 -0700, H. Peter Anvin wrote:
>>
>>> So it should either be start=0xfffffffffffff000 end=0xffffffffffffffff
>>> or it should be start=0xfffffffffffff000 len=0x1000.
>>
>> I would strongly object to the former; that kind of inclusive ranges
>> breed a whole class of bugs by themselves.
>
> Another alternative that avoids overflow issues is to use a PFN rather
> than a byte address.
>

Except as a result of that logic have a bunch of places which either 
have rounding errors in how they calculate PFNs, or they think they can 
stick PFNs into 32-bit numbers.  :(

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

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

[Sasha Levin]

On 09/09/2012 06:56 PM, H. Peter Anvin wrote:
>>
>> Anyway, that means that the BUG_ON() is likely bogus, but so is the
>> whole calling convention.
>>
>> The 4kB range starting at 0xfffffffffffff000 sounds like a *valid*
>> range, but that requires that we fix the calling convention to not
>> have that "end" (exclusive) thing. It should either be "end"
>> (inclusive), or just "len".
>>
> 
> On x86, it is definitely NOT a valid range.  There is no physical addresses
> there, and there will never be any.

This reminds me a similar issue: If you try to mmap /dev/kmem at an offset which
is not kernel owned (such as 0), you'll get all the way to __pa() before getting
a BUG() about addresses not making sense.

How come there's no arch-specific validation of attempts to access
virtual/physical addresses? In the kmem example I'd assume that something very
early on should be yelling at me about doing something like that, but for some
reason I get all the way to __pa() before getting a BUG() (!).


Thanks,
Sasha

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

[Sasha Levin]

On 09/09/2012 04:56 PM, Suresh Siddha wrote:
> On Sat, 2012-09-08 at 12:57 -0700, Linus Torvalds wrote:
>> > Whatever. Something like this (TOTALLY UNTESTED) attached patch should
>> > get the mtdchar overflows to go away, 
> It looks good to me. Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>
> 
> Sasha, can you please give this a try?

Sorry for the delay. It looks good here.


Thanks,
Sasha

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

[Sasha Levin]

On 09/12/2012 12:50 PM, Sasha Levin wrote:
> On 09/09/2012 04:56 PM, Suresh Siddha wrote:
>> On Sat, 2012-09-08 at 12:57 -0700, Linus Torvalds wrote:
>>>> Whatever. Something like this (TOTALLY UNTESTED) attached patch should
>>>> get the mtdchar overflows to go away, 
>> It looks good to me. Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>
>>
>> Sasha, can you please give this a try?
> 
> Sorry for the delay. It looks good here.
> 
> 
> Thanks,
> Sasha
> 

Uh... sorry again, I obviously tested the second patch sent by Linus but
mistakingly replied to the wrong mail in the thread.

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

[Sasha Levin]

On 09/12/2012 12:56 PM, Sasha Levin wrote:
> On 09/12/2012 12:50 PM, Sasha Levin wrote:
>> On 09/09/2012 04:56 PM, Suresh Siddha wrote:
>>> On Sat, 2012-09-08 at 12:57 -0700, Linus Torvalds wrote:
>>>>> Whatever. Something like this (TOTALLY UNTESTED) attached patch should
>>>>> get the mtdchar overflows to go away, 
>>> It looks good to me. Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>
>>>
>>> Sasha, can you please give this a try?
>>
>> Sorry for the delay. It looks good here.
>>
>>
>> Thanks,
>> Sasha
>>
> 
> Uh... sorry again, I obviously tested the second patch sent by Linus but
> mistakingly replied to the wrong mail in the thread.

Is anyone planning on picking up Linus' patch? This is still not in -next even.


Thanks,
Sasha

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

[Linus Torvalds]

On Fri, Sep 28, 2012 at 2:00 AM, Sasha Levin <levinsasha928@gmail.com> wrote:
>
> Is anyone planning on picking up Linus' patch? This is still not in -next even.

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.

Hmm.

           Linus

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

[Artem Bityutskiy]

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

On Fri, 2012-09-28 at 09:44 -0700, Linus Torvalds wrote:
> On Fri, Sep 28, 2012 at 2:00 AM, Sasha Levin <levinsasha928@gmail.com> wrote:
> >
> > Is anyone planning on picking up Linus' patch? This is still not in -next even.
> 
> 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.

Hi Linus,

I am not the maintainer, but please, go ahead an push your fix. I do not
have time to test it myself and it does not look like anyone else in the
small mtd community does.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[David Woodhouse]

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

On Fri, 2012-09-28 at 09:44 -0700, Linus Torvalds wrote:
> On Fri, Sep 28, 2012 at 2:00 AM, Sasha Levin <levinsasha928@gmail.com> wrote:
> >
> > Is anyone planning on picking up Linus' patch? This is still not in -next even.
> 
> 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.

-- 
dwmw2

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

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

[Linus Torvalds]

On Fri, Sep 28, 2012 at 11:05 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
>
> I am not the maintainer, but please, go ahead an push your fix. I do not
> have time to test it myself and it does not look like anyone else in the
> small mtd community does.

Grr. I told people that patch wasn't tested. I hadn't even *compiled*
it. It has a typo ("inlint" instead of "inline" - so close).

Sasha said he had tested it, but nobody even mentioned this thing. Now
I'm nervous. I had committed it in my tree and was just about to push
it out when I decided that I should at least compile it despite the
"tested-by".

Hmm? Now I really *really* want to know that it's been tested on
actual hardware too. Sasha, what patch did you actually test? Did you
just fix the "inlint" thing, or was there something else entirely?

          Linus

PS. Artem: your "Reply-to" is broken, and doesn't have your real name.
Please fix.

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

[richard -rw- weinberger]

On Fri, Sep 28, 2012 at 9:04 PM, David Woodhouse <dwmw2@infradead.org> wrote:
> On Fri, 2012-09-28 at 09:44 -0700, Linus Torvalds wrote:
>> On Fri, Sep 28, 2012 at 2:00 AM, Sasha Levin <levinsasha928@gmail.com> wrote:
>> >
>> > Is anyone planning on picking up Linus' patch? This is still not in -next even.
>>
>> 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.

If you can a easy test-case I can test it on real hardware.

-- 
Thanks,
//richard

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

[richard -rw- weinberger]

On Fri, Sep 28, 2012 at 9:15 PM, richard -rw- weinberger
<richard.weinberger@gmail.com> wrote:
> On Fri, Sep 28, 2012 at 9:04 PM, David Woodhouse <dwmw2@infradead.org> wrote:
>> On Fri, 2012-09-28 at 09:44 -0700, Linus Torvalds wrote:
>>> On Fri, Sep 28, 2012 at 2:00 AM, Sasha Levin <levinsasha928@gmail.com> wrote:
>>> >
>>> > Is anyone planning on picking up Linus' patch? This is still not in -next even.
>>>
>>> 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.
>
> If you can a easy test-case I can test it on real hardware.

Bah.
If you have an easy test case I can run it on real hardware. :)


-- 
Thanks,
//richard

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

[Sasha Levin]

On 09/28/2012 09:13 PM, Linus Torvalds wrote:
> On Fri, Sep 28, 2012 at 11:05 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
>>
>> I am not the maintainer, but please, go ahead an push your fix. I do not
>> have time to test it myself and it does not look like anyone else in the
>> small mtd community does.
> 
> Grr. I told people that patch wasn't tested. I hadn't even *compiled*
> it. It has a typo ("inlint" instead of "inline" - so close).
> 
> Sasha said he had tested it, but nobody even mentioned this thing. Now
> I'm nervous. I had committed it in my tree and was just about to push
> it out when I decided that I should at least compile it despite the
> "tested-by".
> 
> Hmm? Now I really *really* want to know that it's been tested on
> actual hardware too. Sasha, what patch did you actually test? Did you
> just fix the "inlint" thing, or was there something else entirely?

I've just fixed the inlint thing since it was pretty trivial, I didn't bother commenting on it since I figured it would turn into
an actual patch from someone who could test it on actual hardware first.

Note that I've tested it on a KVM guest, and not on real hardware - so I'm not sure how much of a test that is.


Thanks,
Sasha

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

[David Woodhouse]

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

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:

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;
 		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.

-- 
dwmw2

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

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

[David Woodhouse]

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

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.

-- 
dwmw2

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]