From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S934836AbbIVVNg (ORCPT <rfc822;w@1wt.eu>);
	Tue, 22 Sep 2015 17:13:36 -0400
Received: from mail.linuxfoundation.org ([140.211.169.12]:36513 "EHLO
	mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S934652AbbIVVNe (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Tue, 22 Sep 2015 17:13:34 -0400
Date: Tue, 22 Sep 2015 14:13:33 -0700
From: Andrew Morton <akpm@linux-foundation.org>
To: Ross Zwisler <ross.zwisler@linux.intel.com>
Cc: linux-kernel@vger.kernel.org, Alexander Viro <viro@zeniv.linux.org.uk>,
        Matthew Wilcox <willy@linux.intel.com>, linux-fsdevel@vger.kernel.org,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        linux-nvdimm@ml01.01.org, Dan Williams <dan.j.williams@intel.com>,
        Dave Chinner <david@fromorbit.com>
Subject: Re: [PATCH v2] dax: fix NULL pointer in __dax_pmd_fault()
Message-Id: <20150922141333.c28e3c5d800267937ca7b29a@linux-foundation.org>
In-Reply-To: <1442950582-10140-1-git-send-email-ross.zwisler@linux.intel.com>
References: <1442950582-10140-1-git-send-email-ross.zwisler@linux.intel.com>
X-Mailer: Sylpheed 3.4.1 (GTK+ 2.24.23; x86_64-pc-linux-gnu)
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, 22 Sep 2015 13:36:22 -0600 Ross Zwisler <ross.zwisler@linux.intel.com> 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?