Re: DTrace panic while probing syscall::open (and possibly many others)

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
> > _at__at_ -524,7 +524,7 _at__at_
> > 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)
_at__at_ -524,9 +524,9 _at__at_
 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