From owner-freebsd-current@FreeBSD.ORG Wed Jun 24 00:35:33 2009 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 0F66B1065673 for ; Wed, 24 Jun 2009 00:35:33 +0000 (UTC) (envelope-from wxs@atarininja.org) Received: from syn.atarininja.org (syn.csh.rit.edu [129.21.60.158]) by mx1.freebsd.org (Postfix) with ESMTP id CCDA98FC1B for ; Wed, 24 Jun 2009 00:35:32 +0000 (UTC) (envelope-from wxs@atarininja.org) Received: by syn.atarininja.org (Postfix, from userid 1001) id DAE0E5C3B; Tue, 23 Jun 2009 20:35:31 -0400 (EDT) Date: Tue, 23 Jun 2009 20:35:31 -0400 From: Wesley Shields To: Thomas Backman Message-ID: <20090624003531.GA63536@atarininja.org> References: <60173AF0-7E54-4BDD-8927-0DADA9DAD1B4@exscape.org> <20090522200306.GE2630@atarininja.org> <20090617225849.GB28509@atarininja.org> <4A3A1D27.4010802@icyb.net.ua> <4A3BBF5A.6060702@icyb.net.ua> <4A3BC481.1010600@cs.rice.edu> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.19 (2009-01-05) Cc: Alan Cox , John Birrell , FreeBSD current Subject: Re: DTrace panic while probing syscall::open (and possibly many others) X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 24 Jun 2009 00:35:33 -0000 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