Re: mtd: kernel BUG at arch/x86/mm/pat.c:279!
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 > > Sasha, can you please give this a try? Sorry for the delay. It looks good here. Thanks, Sasha -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: mtd: kernel BUG at arch/x86/mm/pat.c:279!
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 >> >> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: mtd: kernel BUG at arch/x86/mm/pat.c:279!
Ping? Still seeing this with latest master... On Fri, Jun 29, 2012 at 10:48 AM, Sasha Levin 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: [#1] PREEMPT SMP DEBUG_PAGEALLOC > [ 3299.678596] CPU 2 > [ 3299.678596] Pid: 21541, comm: trinity-child6 Tainted: GW > 3.5.0-rc4-next-20120628-sasha-5-g9f23eb7 #479 > [ 3299.678596] RIP: 0010:[] [] > reserve_memtype+0x22/0x3d0 > [ 3299.678596] RSP: 0018:88000ad61bc8 EFLAGS: 00010286 > [ 3299.678596] RAX: RBX: f000 RCX: > 88000ad61c50 > [ 3299.678596] RDX: 0010 RSI: RDI: > f000 > [ 3299.696632] RBP: 88000ad61c08 R08: 0010 R09: > 88002617d5a8 > [ 3299.696632] R10: 88003111edc8 R11: 0001 R12: > 88000ad61c50 > [ 3299.696632] R13: f000 R14: R15: > 88000ad61d18 > [ 3299.696632] FS: 7f3ffc3aa700() GS:88002980() > knlGS: > [ 3299.696632] CS: 0010 DS: ES: CR0: 80050033 > [ 3299.696632] CR2: 00f73ffc CR3: 0ad6e000 CR4: > 000406e0 > [ 3299.696632] DR0: DR1: DR2: > > [ 3299.696632] DR3: DR6: 0ff0 DR7: > 0400 > [ 3299.696632] Process trinity-child6 (pid: 21541, threadinfo > 88000ad6, task 88000a39) > [ 3299.696632] Stack: > [ 3299.696632] 88000ad61c18 81161bc6 88000ad61c18 > f000 > [ 3299.696632] 0010 1000 > 88000ad61d18 > [ 3299.696632] 88000ad61c88 810a8fe2 88000ad61c38 > 0086 > [ 3299.696632] Call Trace: > [ 3299.696632] [] ? mark_held_locks+0xf6/0x120 > [ 3299.696632] [] reserve_pfn_range+0xd2/0x1e0 > [ 3299.696632] [] track_pfn_vma_new+0x3d/0x80 > [ 3299.696632] [] remap_pfn_range+0xac/0x380 > [ 3299.696632] [] mtdchar_mmap+0xe6/0x100 > [ 3299.696632] [] mmap_region+0x35e/0x5f0 > [ 3299.696632] [] do_mmap_pgoff+0x2b9/0x350 > [ 3299.696632] [] ? vm_mmap_pgoff+0x6c/0xb0 > [ 3299.696632] [] vm_mmap_pgoff+0x84/0xb0 > [ 3299.696632] [] ? fget_raw+0x260/0x260 > [ 3299.696632] [] sys_mmap_pgoff+0x15e/0x190 > [ 3299.696632] [] ? trace_hardirqs_on_thunk+0x3a/0x3f > [ 3299.696632] [] sys_mmap+0x1d/0x20 > [ 3299.696632] [] 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 [] reserve_memtype+0x22/0x3d0 > [ 3299.696632] RSP > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: mtd: kernel BUG at arch/x86/mm/pat.c:279!
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 wrote: > Ping? Still seeing this with latest master... > > On Fri, Jun 29, 2012 at 10:48 AM, Sasha Levin 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: [#1] PREEMPT SMP DEBUG_PAGEALLOC >> [ 3299.678596] CPU 2 >> [ 3299.678596] Pid: 21541, comm: trinity-child6 Tainted: GW >> 3.5.0-rc4-next-20120628-sasha-5-g9f23eb7 #479 >> [ 3299.678596] RIP: 0010:[] [] >> reserve_memtype+0x22/0x3d0 >> [ 3299.678596] RSP: 0018:88000ad61bc8 EFLAGS: 00010286 >> [ 3299.678596] RAX: RBX: f000 RCX: >> 88000ad61c50 >> [ 3299.678596] RDX: 0010 RSI: RDI: >> f000 >> [ 3299.696632] RBP: 88000ad61c08 R08: 0010 R09: >> 88002617d5a8 >> [ 3299.696632] R10: 88003111edc8 R11: 0001 R12: >> 88000ad61c50 >> [ 3299.696632] R13: f000 R14: R15: >> 88000ad61d18 >> [ 3299.696632] FS: 7f3ffc3aa700() GS:88002980() >> knlGS: >> [ 3299.696632] CS: 0010 DS: ES: CR0: 80050033 >> [ 3299.696632] CR2: 00f73ffc CR3: 0ad6e000 CR4: >> 000406e0 >> [ 3299.696632] DR0: DR1: DR2: >> >> [ 3299.696632] DR3: DR6: 0ff0 DR7: >> 0400 >> [ 3299.696632] Process trinity-child6 (pid: 21541, threadinfo >> 88000ad6, task 88000a39) >> [ 3299.696632] Stack: >> [ 3299.696632] 88000ad61c18 81161bc6 88000ad61c18 >> f000 >> [ 3299.696632] 0010 1000 >> 88000ad61d18 >> [ 3299.696632] 88000ad61c88 810a8fe2 88000ad61c38 >> 0086 >> [ 3299.696632] Call Trace: >> [ 3299.696632] [] ? mark_held_locks+0xf6/0x120 >> [ 3299.696632] [] reserve_pfn_range+0xd2/0x1e0 >> [ 3299.696632] [] track_pfn_vma_new+0x3d/0x80 >> [ 3299.696632] [] remap_pfn_range+0xac/0x380 >> [ 3299.696632] [] mtdchar_mmap+0xe6/0x100 >> [ 3299.696632] [] mmap_region+0x35e/0x5f0 >> [ 3299.696632] [] do_mmap_pgoff+0x2b9/0x350 >> [ 3299.696632] [] ? vm_mmap_pgoff+0x6c/0xb0 >> [ 3299.696632] [] vm_mmap_pgoff+0x84/0xb0 >> [ 3299.696632] [] ? fget_raw+0x260/0x260 >> [ 3299.696632] [] sys_mmap_pgoff+0x15e/0x190 >> [ 3299.696632] [] ? trace_hardirqs_on_thunk+0x3a/0x3f >> [ 3299.696632] [] sys_mmap+0x1d/0x20 >> [ 3299.696632] [] 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 [] reserve_memtype+0x22/0x3d0 >> [ 3299.696632] RSP >> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: mtd: kernel BUG at arch/x86/mm/pat.c:279!
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 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 --- 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; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: mtd: kernel BUG at arch/x86/mm/pat.c:279!
On Fri, Sep 7, 2012 at 3:42 PM, Suresh Siddha 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: mtd: kernel BUG at arch/x86/mm/pat.c:279!
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 "0xf000"). 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: mtd: kernel BUG at arch/x86/mm/pat.c:279!
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 f000 - 0 Thanks, Sasha -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: mtd: kernel BUG at arch/x86/mm/pat.c:279!
On Fri, Sep 7, 2012 at 4:54 PM, Suresh Siddha 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 > "0xf000"). 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 0xf000 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=0xf000 end=0x or it should be start=0xf000 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 0xf000+0x1000, and right now it causes a BUG_ON() in pat.c. Suresh? Linus patch.diff Description: Binary data
Re: mtd: kernel BUG at arch/x86/mm/pat.c:279!
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 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 0xf000+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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: mtd: kernel BUG at arch/x86/mm/pat.c:279!
On Sun, Sep 9, 2012 at 7:56 AM, Suresh Siddha 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: mtd: kernel BUG at arch/x86/mm/pat.c:279!
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 0xf000 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=0xf000 end=0x or it should be start=0xf000 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: mtd: kernel BUG at arch/x86/mm/pat.c:279!
On 09/09/2012 08:31 AM, Linus Torvalds wrote: On Sun, Sep 9, 2012 at 7:56 AM, Suresh Siddha 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: mtd: kernel BUG at arch/x86/mm/pat.c:279!
On Sun, 2012-09-09 at 09:56 -0700, H. Peter Anvin wrote: > > > So it should either be start=0xf000 end=0x > > or it should be start=0xf000 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 smime.p7s Description: S/MIME cryptographic signature
Re: mtd: kernel BUG at arch/x86/mm/pat.c:279!
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=0xf000 end=0x or it should be start=0xf000 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: mtd: kernel BUG at arch/x86/mm/pat.c:279!
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 0xf000 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: mtd: kernel BUG at arch/x86/mm/pat.c:279!
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 >>> >>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: mtd: kernel BUG at arch/x86/mm/pat.c:279!
On Fri, Sep 28, 2012 at 2:00 AM, Sasha Levin 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: mtd: kernel BUG at arch/x86/mm/pat.c:279!
On Fri, 2012-09-28 at 09:44 -0700, Linus Torvalds wrote: > On Fri, Sep 28, 2012 at 2:00 AM, Sasha Levin 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 signature.asc Description: This is a digitally signed message part
Re: mtd: kernel BUG at arch/x86/mm/pat.c:279!
On Fri, Sep 28, 2012 at 11:05 AM, Artem Bityutskiy 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: mtd: kernel BUG at arch/x86/mm/pat.c:279!
On Fri, Sep 28, 2012 at 9:04 PM, David Woodhouse wrote: > On Fri, 2012-09-28 at 09:44 -0700, Linus Torvalds wrote: >> On Fri, Sep 28, 2012 at 2:00 AM, Sasha Levin 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: mtd: kernel BUG at arch/x86/mm/pat.c:279!
On Fri, Sep 28, 2012 at 9:15 PM, richard -rw- weinberger wrote: > On Fri, Sep 28, 2012 at 9:04 PM, David Woodhouse wrote: >> On Fri, 2012-09-28 at 09:44 -0700, Linus Torvalds wrote: >>> On Fri, Sep 28, 2012 at 2:00 AM, Sasha Levin >>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: mtd: kernel BUG at arch/x86/mm/pat.c:279!
On Fri, 2012-09-28 at 09:44 -0700, Linus Torvalds wrote: > On Fri, Sep 28, 2012 at 2:00 AM, Sasha Levin 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 smime.p7s Description: S/MIME cryptographic signature
Re: mtd: kernel BUG at arch/x86/mm/pat.c:279!
On 09/28/2012 09:13 PM, Linus Torvalds wrote: > On Fri, Sep 28, 2012 at 11:05 AM, Artem Bityutskiy > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: mtd: kernel BUG at arch/x86/mm/pat.c:279!
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 smime.p7s Description: S/MIME cryptographic signature
Re: mtd: kernel BUG at arch/x86/mm/pat.c:279!
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 smime.p7s Description: S/MIME cryptographic signature
Re: mtd: kernel BUG at arch/x86/mm/pat.c:279!
Ping? On Fri, Jun 29, 2012 at 10:48 AM, Sasha Levin 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: [#1] PREEMPT SMP DEBUG_PAGEALLOC > [ 3299.678596] CPU 2 > [ 3299.678596] Pid: 21541, comm: trinity-child6 Tainted: GW > 3.5.0-rc4-next-20120628-sasha-5-g9f23eb7 #479 > [ 3299.678596] RIP: 0010:[] [] > reserve_memtype+0x22/0x3d0 > [ 3299.678596] RSP: 0018:88000ad61bc8 EFLAGS: 00010286 > [ 3299.678596] RAX: RBX: f000 RCX: > 88000ad61c50 > [ 3299.678596] RDX: 0010 RSI: RDI: > f000 > [ 3299.696632] RBP: 88000ad61c08 R08: 0010 R09: > 88002617d5a8 > [ 3299.696632] R10: 88003111edc8 R11: 0001 R12: > 88000ad61c50 > [ 3299.696632] R13: f000 R14: R15: > 88000ad61d18 > [ 3299.696632] FS: 7f3ffc3aa700() GS:88002980() > knlGS: > [ 3299.696632] CS: 0010 DS: ES: CR0: 80050033 > [ 3299.696632] CR2: 00f73ffc CR3: 0ad6e000 CR4: > 000406e0 > [ 3299.696632] DR0: DR1: DR2: > > [ 3299.696632] DR3: DR6: 0ff0 DR7: > 0400 > [ 3299.696632] Process trinity-child6 (pid: 21541, threadinfo > 88000ad6, task 88000a39) > [ 3299.696632] Stack: > [ 3299.696632] 88000ad61c18 81161bc6 88000ad61c18 > f000 > [ 3299.696632] 0010 1000 > 88000ad61d18 > [ 3299.696632] 88000ad61c88 810a8fe2 88000ad61c38 > 0086 > [ 3299.696632] Call Trace: > [ 3299.696632] [] ? mark_held_locks+0xf6/0x120 > [ 3299.696632] [] reserve_pfn_range+0xd2/0x1e0 > [ 3299.696632] [] track_pfn_vma_new+0x3d/0x80 > [ 3299.696632] [] remap_pfn_range+0xac/0x380 > [ 3299.696632] [] mtdchar_mmap+0xe6/0x100 > [ 3299.696632] [] mmap_region+0x35e/0x5f0 > [ 3299.696632] [] do_mmap_pgoff+0x2b9/0x350 > [ 3299.696632] [] ? vm_mmap_pgoff+0x6c/0xb0 > [ 3299.696632] [] vm_mmap_pgoff+0x84/0xb0 > [ 3299.696632] [] ? fget_raw+0x260/0x260 > [ 3299.696632] [] sys_mmap_pgoff+0x15e/0x190 > [ 3299.696632] [] ? trace_hardirqs_on_thunk+0x3a/0x3f > [ 3299.696632] [] sys_mmap+0x1d/0x20 > [ 3299.696632] [] 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 [] reserve_memtype+0x22/0x3d0 > [ 3299.696632] RSP > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/