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

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?

Regards,
Thomas