Date: Tue, 23 Jun 2009 20:35:31 -0400 From: Wesley Shields <wxs@FreeBSD.org> To: Thomas Backman <serenity@exscape.org> Cc: Alan Cox <alc@freebsd.org>, John Birrell <jb@freebsd.org>, FreeBSD current <freebsd-current@freebsd.org> Subject: Re: DTrace panic while probing syscall::open (and possibly many others) Message-ID: <20090624003531.GA63536@atarininja.org> In-Reply-To: <F55615D8-8AE3-41C6-BD2F-0DB911918465@exscape.org> References: <60173AF0-7E54-4BDD-8927-0DADA9DAD1B4@exscape.org> <20090522200306.GE2630@atarininja.org> <20090617225849.GB28509@atarininja.org> <B2F32C8F-810B-4EA5-9E34-39ADD5E5CED4@exscape.org> <4A3A1D27.4010802@icyb.net.ua> <DD13EADF-CE41-465A-8D70-53DCDEFD65A7@exscape.org> <4A3BBF5A.6060702@icyb.net.ua> <4A3BC481.1010600@cs.rice.edu> <F72EDBE7-9A0A-44AB-AF58-23CE34CC93A0@exscape.org> <F55615D8-8AE3-41C6-BD2F-0DB911918465@exscape.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Jun 19, 2009 at 07:41:29PM +0200, Thomas Backman wrote:
> On Jun 19, 2009, at 07:32 PM, Thomas Backman wrote:
>
> >
> > On Jun 19, 2009, at 07:01 PM, Alan Cox wrote:
> >
> >> Andriy Gapon wrote:
> >>> on 18/06/2009 14:42 Thomas Backman said the following:
> >>>
> >>>> On Jun 18, 2009, at 12:55 PM, Andriy Gapon wrote:
> >>>>
> >>>>
> >>>>> on 18/06/2009 12:43 Thomas Backman said the following:
> >>>>>
> >>>>>> at dtrace_isa.c:527
> >>>>>> #14 0xffffffff816b31fc in dtrace_copyinstr (uaddr=34365163021,
> >>>>>> kaddr=18446743524025463312, size=256, flags=0xffffffff8146e0c0)
> >>>>>> at dtrace_isa.c:558
> >>>>>>
> >>>>> kaddr=18446743524025463312 == FFFFFF8004467210
> >>>>> I think kernelbase on amd64 is 0xFFFFFFFF80000000.
> >>>>> FFFFFF8004467210 kaddr
> >>>>> is smaller than
> >>>>> FFFFFFFF80000000 kernelbase
> >>>>>
> >>>>> The numbers do look suspiciously similar, so I am not sure if
> >>>>> you are
> >>>>> seeing a
> >>>>> race or a real bug somewhere.
> >>>>> --
> >>>>> Andriy Gapon
> >>>>>
> >>>> Hmmm...
> >>>> Looking around a bit for these numbers, I found, in
> >>>> /sys/amd64/include/vmparam.h:
> >>>>
> >>>> /*
> >>>> * Virtual addresses of things. Derived from the page directory and
> >>>> * page table indexes from pmap.h for precision.
> >>>> *
> >>>> * 0x0000000000000000 - 0x00007fffffffffff user map
> >>>> * 0x0000800000000000 - 0xffff7fffffffffff does not exist (hole)
> >>>> * 0xffff800000000000 - 0xffff804020100fff recursive page table
> >>>> (512GB
> >>>> slot)
> >>>> * 0xffff804020101000 - 0xfffffeffffffffff unused
> >>>> * 0xffffff0000000000 - 0xffffff7fffffffff 512GB direct map
> >>>> mappings
> >>>> * 0xffffff8000000000 - 0xffffffffffffffff 512GB kernel map
> >>>> *
> >>>> * Within the kernel map:
> >>>> *
> >>>> * 0xffffffff80000000 KERNBASE
> >>>> */
> >>>>
> >>>> So, kaddr is inside the "kernel map", but not KERNBASE. What this
> >>>> means,
> >>>> I have no clue whatsoever. (I'm not a kernel developer and I
> >>>> don't know
> >>>> too much about (virtual) memory either!)
> >>>>
> >>>
> >>> Thomas,
> >>>
> >>> I think that you were correct that one needs to be somewhat of a
> >>> VM expert here.
> >>> It seems that amd64 is the only[?] platform where KERNBASE !=
> >>> VM_MIN_KERNEL_ADDRESS (0xffffffff80000000 and 0xffffff8000000000
> >>> correspondingly).
> >>> That makes the assert in sys/cddl/dev/dtrace/amd64/dtrace_isa.c
> >>> bogus in my opinion:
> >>> static int
> >>> dtrace_copycheck(uintptr_t uaddr, uintptr_t kaddr, size_t size)
> >>> {
> >>> ASSERT(kaddr >= kernelbase && kaddr + size >= kaddr);
> >>>
> >>> If the purpose of the assert is to ensure that [kaddr:kaddr+size)
> >>> is within kernel
> >>> memory, then it should use VM_MIN_KERNEL_ADDRESS instead of
> >>> KERNBASE. Or maybe
> >>> even use something like the macro in sys/amd64/include/stack.h:
> >>> #define INKERNEL(va) (((va) >= DMAP_MIN_ADDRESS && (va) <
> >>> DMAP_MAX_ADDRESS) \
> >>> || ((va) >= VM_MIN_KERNEL_ADDRESS && (va) <
> >>> VM_MAX_KERNEL_ADDRESS))
> >>>
> >>>
> >>
> >> Yes. Your analysis is correct.
> >>
> >> Alan
> > Very interesting.
> > I replaced the ASSERT line temporarily:
> >
> > --- ../src_r194478-UNTOUCHED/sys/cddl/dev/dtrace/amd64/
> > dtrace_isa.c 2009-06-19 13:10:05.661079736 +0200
> > +++ sys/cddl/dev/dtrace/amd64/dtrace_isa.c 2009-06-19
> > 19:24:42.362125129 +0200
> > @@ -524,7 +524,7 @@
> > static int
> > dtrace_copycheck(uintptr_t uaddr, uintptr_t kaddr, size_t size)
> > {
> > - ASSERT(kaddr >= kernelbase && kaddr + size >= kaddr);
> > + ASSERT(kaddr >= 0xffffff8000000000 && kaddr + size >= kaddr);
> >
> > if (uaddr + size >= kernelbase || uaddr + size < uaddr) {
> > DTRACE_CPUFLAG_SET(CPU_DTRACE_BADADDR);
> >
> > ... and it works! I obviously haven't tried it for extended periods
> > or anything, but at least it's working so far.
> > Should the ASSERT simply use this (as a #define somewhere) or the
> > INKERNEL macro, though?
> BTW... Should "kernelbase" in the line following the ASSERT also be
> replaced, or not? As far as I can understand (not too far in these
> contexts ;) it (should) check/s to see whether the userspace data, to
> be copied, is inside the kernel *map*(?)... which at the moment, I
> guess it doesn't. Correct?
This patch makes it work for me and uses INKERNEL. I have no idea if
it's correct or not...
Index: sys/cddl/dev/dtrace/amd64/dtrace_isa.c
===================================================================
--- sys/cddl/dev/dtrace/amd64/dtrace_isa.c (revision 194740)
+++ sys/cddl/dev/dtrace/amd64/dtrace_isa.c (working copy)
@@ -524,9 +524,9 @@
static int
dtrace_copycheck(uintptr_t uaddr, uintptr_t kaddr, size_t size)
{
- ASSERT(kaddr >= kernelbase && kaddr + size >= kaddr);
+ ASSERT(INKERNEL(kaddr) && kaddr + size >= kaddr);
- if (uaddr + size >= kernelbase || uaddr + size < uaddr) {
+ if (INKERNEL(uaddr + size) || uaddr + size < uaddr) {
DTRACE_CPUFLAG_SET(CPU_DTRACE_BADADDR);
cpu_core[curcpu].cpuc_dtrace_illval = uaddr;
return (0);
I've put a copy up at http://people.freebsd.org/~wxs/dtrace.diff too.
-- WXS
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090624003531.GA63536>