Re: mm: NULL ptr deref handling mmaping of special mappings

Sasha Levin
2014-05-14T21:11:13Z

On 05/14/2014 05:03 PM, Andrew Morton wrote:
> On Wed, 14 May 2014 16:41:45 -0400 Sasha Levin <sasha.levin@oracle.com> wrote:
>
>> On 05/14/2014 04:23 PM, Andrew Morton wrote:
>>> On Wed, 14 May 2014 11:55:45 -0400 Sasha Levin <sasha.levin@oracle.com> wrote:
>>>
>>>> Hi all,
>>>>
>>>> While fuzzing with trinity inside a KVM tools guest running the latest -next
>>>> kernel I've stumbled on the following spew:
>>>>
>>>> [ 1634.969408] BUG: unable to handle kernel NULL pointer dereference at (null)
>>>> [ 1634.970538] IP: special_mapping_fault (mm/mmap.c:2961)
>>>> [ 1634.971420] PGD 3334fc067 PUD 3334cf067 PMD 0
>>>> [ 1634.972081] Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
>>>> [ 1634.972913] Dumping ftrace buffer:
>>>> [ 1634.975493] (ftrace buffer empty)
>>>> [ 1634.977470] Modules linked in:
>>>> [ 1634.977513] CPU: 6 PID: 29578 Comm: trinity-c269 Not tainted 3.15.0-rc5-next-20140513-sasha-00020-gebce144-dirty #461
>>>> [ 1634.977513] task: ffff880333158000 ti: ffff88033351e000 task.ti: ffff88033351e000
>>>> [ 1634.977513] RIP: special_mapping_fault (mm/mmap.c:2961)
>>>
>>> Somebody's gone and broken the x86 oops output. It used to say
>>> "special_mapping_fault+0x30/0x120" but the offset info has now
>>> disappeared. That was useful for guesstimating whereabouts in the
>>> function it died.
>>
>> I'm the one who "broke" the oops output, but I thought I'm helping people
>> read that output instead of making it harder...
>>
>> What happened before is that due to my rather complex .config, the offsets
>> didn't make sense to anyone who didn't build the kernel with my .config,
>> so I had to repeatedly send it out to folks who attempted to get basic
>> things like line numbers.
>>
>>> The line number isn't very useful as it's not possible (or at least,
>>> not convenient) for others to reliably reproduce your kernel.
>>
>> I don't understand that part. I'm usually stating in the beginning of my
>> mails that I run my testing on the latest -next kernel.
>
> Your "latest next kernel" apparently differes from mine ;( It would be
> useful if you could just quote the +/-5 lines, perhaps?

Oh, I see what happened. I have the remap_file_pages() get_file/fput fix
merged in which modified line count.

Yup, I'll start quoting the line themselves as well.

>> And indeed if
>> you look at today's -next, that line number would point to:
>>
>> for (pages = vma->vm_private_data; pgoff && *pages; ++pages) <=== HERE
>> pgoff--;
>>
>> So I'm not sure how replacing the offset with line numbers is making things
>> worse? previously offsets were useless for people who tried to debug these
>> spews so that's why I switched it to line numbers in the first place.
>>
>>> <scrabbles with git for a while>
>>>
>>> : static int special_mapping_fault(struct vm_area_struct *vma,
>>> : struct vm_fault *vmf)
>>> : {
>>> : pgoff_t pgoff;
>>> : struct page **pages;
>>> :
>>> : /*
>>> : * special mappings have no vm_file, and in that case, the mm
>>> : * uses vm_pgoff internally. So we have to subtract it from here.
>>> : * We are allowed to do this because we are the mm; do not copy
>>> : * this code into drivers!
>>> : */
>>> : pgoff = vmf->pgoff - vma->vm_pgoff;
>>> :
>>> : for (pages = vma->vm_private_data; pgoff && *pages; ++pages)
>>> : pgoff--;
>>> :
>>> : if (*pages) {
>>> : struct page *page = *pages;
>>> : get_page(page);
>>> : vmf->page = page;
>>> : return 0;
>>> : }
>>> :
>>> : return VM_FAULT_SIGBUS;
>>> : }
>>>
>>> OK so it might be the "if (*pages)". So vma->vm_private_data was NULL
>>> and pgoff was zero. As usual, I can't imagine what race would cause
>>> that :(
>>
>> Yup, it's the *pages part in the 'for' loop above that. I did find the
>> following in the vdso code:
>>
>> vma = _install_special_mapping(mm,
>> addr + image->size,
>> image->sym_end_mapping - image->size,
>> VM_READ,
>> NULL);
>>
>> Which installs a mapping with a NULL ptr for pages (if I understand that
>> correctly), but that code has been there for a while now.
>
> Well that's weird. I don't see anything which permits that. Maybe
> nobody faulted against that address before?
>
> It's unclear what that code's actually doing and nobody bothered
> commenting it of course. Maybe it's installing a guard page?
>
> In my linux-next all that code got deleted by Andy's "x86, vdso:
> Reimplement vdso.so preparation in build-time C" anyway. What kernel
> were you looking at?

Deleted? It appears in today's -next. arch/x86/vdso/vma.c:124 .

I don't see Andy's patch removing that code either.

I'm running next-20140514...

Thanks,
Sasha