Re: [PATCH v2] dax: fix NULL pointer in __dax_pmd_fault()

[Dan Williams]

On Tue, Sep 22, 2015 at 2:13 PM, Andrew Morton
<akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Tue, 22 Sep 2015 13:36:22 -0600 Ross Zwisler <ross.zwisler@xxxxxxxxxxxxxxx> wrote:
>
>> The following commit:
>>
>> commit 46c043ede471 ("mm: take i_mmap_lock in unmap_mapping_range() for
>>       DAX")
>>
>> moved some code in __dax_pmd_fault() that was responsible for zeroing
>> newly allocated PMD pages.  The new location didn't properly set up
>> 'kaddr', though, so when run this code resulted in a NULL pointer BUG.
>>
>> Fix this by getting the correct 'kaddr' via bdev_direct_access().
>
> Why the heck didn't gcc warn?
>
> I had a fiddle:
>
> --- a/fs/dax.c~a
> +++ a/fs/dax.c
> @@ -529,15 +529,18 @@ int __dax_pmd_fault(struct vm_area_struc
>         unsigned long pmd_addr = address & PMD_MASK;
>         bool write = flags & FAULT_FLAG_WRITE;
>         long length;
> -       void __pmem *kaddr;
> +       void *kaddr;
>         pgoff_t size, pgoff;
>         sector_t block, sector;
>         unsigned long pfn;
>         int result = 0;
>
> +//     printk("%p\n", kaddr);
> +
>         /* Fall back to PTEs if we're going to COW */
>         if (write && !(vma->vm_flags & VM_SHARED))
>                 return VM_FAULT_FALLBACK;
> +       printk("%p\n", kaddr);
>         /* If the PMD would extend outside the VMA */
>         if (pmd_addr < vma->vm_start)
>                 return VM_FAULT_FALLBACK;
>
> gcc warns about the first printk, but not about the second.  So that
> "if (...) return ..." seems to have defeated gcc uninitialized-var
> detection.  wtf?
>
>> --- a/fs/dax.c
>> +++ b/fs/dax.c
>> @@ -569,8 +569,20 @@ int __dax_pmd_fault(struct vm_area_struct *vma, unsigned long address,
>>       if (!buffer_size_valid(&bh) || bh.b_size < PMD_SIZE)
>>               goto fallback;
>>
>> +     sector = bh.b_blocknr << (blkbits - 9);
>> +
>>       if (buffer_unwritten(&bh) || buffer_new(&bh)) {
>>               int i;
>> +
>> +             length = bdev_direct_access(bh.b_bdev, sector, &kaddr, &pfn,
>> +                                             bh.b_size);
>> +             if (length < 0) {
>> +                     result = VM_FAULT_SIGBUS;
>> +                     goto out;
>> +             }
>> +             if ((length < PMD_SIZE) || (pfn & PG_PMD_COLOUR))
>> +                     goto fallback;
>> +
>>               for (i = 0; i < PTRS_PER_PMD; i++)
>>                       clear_pmem(kaddr + i * PAGE_SIZE, PAGE_SIZE);
>>               wmb_pmem();
>
> hm, that's a lot of copy-n-paste.  Do we really need to run
> bdev_direct_access() twice?  Will `kaddr' and `pfn' change?
>

They shouldn't change, but I'm working on a fix for handling the race
of unbinding the pmem device while that kaddr is in use (unbind
invalidates kaddr).  The proposal is a dax_map_bh()/dax_unmap_bh()
interface to temporarily pin the mapping around each usage.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/