diff mbox

[10/15] block, dax: fix lifetime of in-kernel dax mappings

Message ID 20150923044206.36490.79829.stgit@dwillia2-desk3.jf.intel.com (mailing list archive)
State Superseded
Headers show

Commit Message

Dan Williams Sept. 23, 2015, 4:42 a.m. UTC
The DAX implementation needs to protect new calls to ->direct_access()
and usage of its return value against unbind of the underlying block
device.  Use blk_dax_{get|put}() to either prevent blk_cleanup_queue()
from proceeding, or fail the dax_map_bh() if the request_queue is being
torn down.

Cc: Jens Axboe <axboe@kernel.dk>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Boaz Harrosh <boaz@plexistor.com>
Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 fs/dax.c |  131 ++++++++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 84 insertions(+), 47 deletions(-)

Comments

Logan Gunthorpe Oct. 7, 2015, 10:56 p.m. UTC | #1
Hi Dan,

We've uncovered another issue during testing with these patches. We get 
a kernel panic sometimes just while using a DAX filesystem. I've traced 
the issue back to this patch. (There's a stack trace at the end of this 
email.)

On 22/09/15 10:42 PM, Dan Williams wrote:
> +static void dax_unmap_bh(const struct buffer_head *bh, void __pmem *addr)
> +{
> +	struct block_device *bdev = bh->b_bdev;
> +	struct request_queue *q = bdev->bd_queue;
> +
> +	if (IS_ERR(addr))
> +		return;
> +	blk_dax_put(q);
>   }
>
> @@ -127,9 +159,8 @@ static ssize_t dax_io(struct inode *inode, struct iov_iter *iter,
>   			if (pos == bh_max) {
>   				bh->b_size = PAGE_ALIGN(end - pos);
>   				bh->b_state = 0;
> -				retval = get_block(inode, block, bh,
> -						   iov_iter_rw(iter) == WRITE);
> -				if (retval)
> +				rc = get_block(inode, block, bh, rw == WRITE);
> +				if (rc)
>   					break;
>   				if (!buffer_size_valid(bh))
>   					bh->b_size = 1 << blkbits;
> @@ -178,8 +213,9 @@ static ssize_t dax_io(struct inode *inode, struct iov_iter *iter,
>
>   	if (need_wmb)
>   		wmb_pmem();
> +	dax_unmap_bh(bh, kmap);
>
> -	return (pos == start) ? retval : pos - start;
> +	return (pos == start) ? rc : pos - start;
>   }

The problem is if get_block fails and returns an error code, it will 
still call dax_unmap_bh which tries to dereference bh->b_bdev. However, 
seeing get_block failed, that pointer is NULL. Maybe a null check in 
dax_unmap_bh would be sufficient?


Thanks,

Logan

--

> [   35.391790] BUG: unable to handle kernel NULL pointer dereference at 00000000000000a0
> [   35.393306] IP: [<ffffffff811d1731>] dax_unmap_bh+0x41/0x70
> [   35.394253] PGD 7c7ed067 PUD 7c01f067 PMD 0
> [   35.395020] Oops: 0000 [#1] SMP
> [   35.395597] Modules linked in: mtramonb(O)
> [   35.396320] CPU: 0 PID: 1501 Comm: dd Tainted: G           O    4.3.0-rc2+ #51
> [   35.397500] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.7.5-0-ge51488c-20140602_164612-nilsson.home.kraxel.org 04/01/2014
> [   35.399728] task: ffff88007bc6d600 ti: ffff880079250000 task.ti: ffff880079250000
> [   35.401006] RIP: 0010:[<ffffffff811d1731>]  [<ffffffff811d1731>] dax_unmap_bh+0x41/0x70
> [   35.402402] RSP: 0018:ffff880079253ab8  EFLAGS: 00010246
> [   35.403321] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000012
> [   35.404550] RDX: 0000000000000007 RSI: 0000000000000246 RDI: ffffffff8181f630
> [   35.405783] RBP: ffff880079253ac8 R08: 000000000000000a R09: 000000000000fffe
> [   35.407011] R10: ffff88007fc1ad90 R11: 0000000000006abc R12: fffffffffffffffb
> [   35.408237] R13: 000000000038e000 R14: 000000000038e000 R15: 000000000038e000
> [   35.409466] FS:  00007f0e81476700(0000) GS:ffff88007fc00000(0000) knlGS:0000000000000000
> [   35.410830] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   35.411796] CR2: 00000000000000a0 CR3: 000000007926f000 CR4: 00000000000006f0
> [   35.412990] Stack:
> [   35.413340]  00000000ffffffe4 ffff880079253cf0 ffff880079253be0 ffffffff811d2259
> [   35.414670]  0000000000000246 ffffffff81203120 ffff880079253e68 0000000000000000
> [   35.415981]  000000017c394800 000000000038e000 0000000100000000 fffffffffffffffb
> [   35.417298] Call Trace:
> [   35.417727]  [<ffffffff811d2259>] dax_do_io+0x199/0x700
> [   35.418615]  [<ffffffff81203120>] ? _ext4_get_block+0x200/0x200
> [   35.419819]  [<ffffffff81249700>] ? jbd2_journal_stop+0x60/0x390
> [   35.420886]  [<ffffffff8123e7fd>] ext4_ind_direct_IO+0x8d/0x410
> [   35.421908]  [<ffffffff8120205a>] ext4_direct_IO+0x2da/0x540
> [   35.422859]  [<ffffffff8120887c>] ? ext4_dirty_inode+0x5c/0x70
> [   35.423841]  [<ffffffff8113306a>] generic_file_direct_write+0xaa/0x170
> [   35.424931]  [<ffffffff811331f2>] __generic_file_write_iter+0xc2/0x1f0
> [   35.426027]  [<ffffffff811fcdcb>] ext4_file_write_iter+0x13b/0x420
> [   35.427066]  [<ffffffff81084ca2>] ? pick_next_entity+0xb2/0x190
> [   35.428061]  [<ffffffff811891f7>] __vfs_write+0xa7/0xf0
> [   35.428940]  [<ffffffff81189b59>] vfs_write+0xa9/0x190
> [   35.429810]  [<ffffffff81189a94>] ? vfs_read+0x114/0x130
> [   35.430706]  [<ffffffff8118a786>] SyS_write+0x46/0xa0
> [   35.431561]  [<ffffffff815f742e>] entry_SYSCALL_64_fastpath+0x12/0x71
> [   35.432637] Code: fe 48 c7 c7 4c 63 7e 81 e8 11 ec f5 ff 48 8b 5b 30 48 c7 c7 52 63 7e 81 31 c0 48 89 de e8 fc eb f5 ff 31 c0 48 c7 c7 30 f6 81 81 <48> 8b 9b a0 00 00 00 e8 e7 eb f5 ff 49 81 fc 00 f0 ff ff 77 08
> [   35.437029] RIP  [<ffffffff811d1731>] dax_unmap_bh+0x41/0x70
> [   35.438005]  RSP <ffff880079253ab8>
> [   35.438608] CR2: 00000000000000a0
> [   35.439194] ---[ end trace 323695b29b46dd96 ]---
Dan Williams Oct. 9, 2015, 9:12 p.m. UTC | #2
On Wed, Oct 7, 2015 at 3:56 PM, Logan Gunthorpe <logang@deltatee.com> wrote:
> Hi Dan,
>
> We've uncovered another issue during testing with these patches. We get a
> kernel panic sometimes just while using a DAX filesystem. I've traced the
> issue back to this patch. (There's a stack trace at the end of this email.)
>
> On 22/09/15 10:42 PM, Dan Williams wrote:
>>
>> +static void dax_unmap_bh(const struct buffer_head *bh, void __pmem *addr)
>> +{
>> +       struct block_device *bdev = bh->b_bdev;
>> +       struct request_queue *q = bdev->bd_queue;
>> +
>> +       if (IS_ERR(addr))
>> +               return;
>> +       blk_dax_put(q);
>>   }
>>
>> @@ -127,9 +159,8 @@ static ssize_t dax_io(struct inode *inode, struct
>> iov_iter *iter,
>>                         if (pos == bh_max) {
>>                                 bh->b_size = PAGE_ALIGN(end - pos);
>>                                 bh->b_state = 0;
>> -                               retval = get_block(inode, block, bh,
>> -                                                  iov_iter_rw(iter) ==
>> WRITE);
>> -                               if (retval)
>> +                               rc = get_block(inode, block, bh, rw ==
>> WRITE);
>> +                               if (rc)
>>                                         break;
>>                                 if (!buffer_size_valid(bh))
>>                                         bh->b_size = 1 << blkbits;
>> @@ -178,8 +213,9 @@ static ssize_t dax_io(struct inode *inode, struct
>> iov_iter *iter,
>>
>>         if (need_wmb)
>>                 wmb_pmem();
>> +       dax_unmap_bh(bh, kmap);
>>
>> -       return (pos == start) ? retval : pos - start;
>> +       return (pos == start) ? rc : pos - start;
>>   }
>
>
> The problem is if get_block fails and returns an error code, it will still
> call dax_unmap_bh which tries to dereference bh->b_bdev. However, seeing
> get_block failed, that pointer is NULL. Maybe a null check in dax_unmap_bh
> would be sufficient?

Thanks for the report, I have this fixed up in v2.  Will post shortly.
diff mbox

Patch

diff --git a/fs/dax.c b/fs/dax.c
index bcfb14bfc1e4..358eea39e982 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -63,12 +63,43 @@  int dax_clear_blocks(struct inode *inode, sector_t block, long size)
 }
 EXPORT_SYMBOL_GPL(dax_clear_blocks);
 
-static long dax_get_addr(struct buffer_head *bh, void __pmem **addr,
-		unsigned blkbits)
+static void __pmem *__dax_map_bh(const struct buffer_head *bh, unsigned blkbits,
+		unsigned long *pfn, long *len)
 {
-	unsigned long pfn;
+	long rc;
+	void __pmem *addr;
+	struct block_device *bdev = bh->b_bdev;
+	struct request_queue *q = bdev->bd_queue;
 	sector_t sector = bh->b_blocknr << (blkbits - 9);
-	return bdev_direct_access(bh->b_bdev, sector, addr, &pfn, bh->b_size);
+
+	rc = blk_dax_get(q);
+	if (rc < 0)
+		return (void __pmem *) ERR_PTR(rc);
+	rc = bdev_direct_access(bdev, sector, &addr, pfn, bh->b_size);
+	if (len)
+		*len = rc;
+	if (rc < 0) {
+		blk_dax_put(q);
+		return (void __pmem *) ERR_PTR(rc);
+	}
+	return addr;
+}
+
+static void __pmem *dax_map_bh(const struct buffer_head *bh, unsigned blkbits)
+{
+	unsigned long pfn;
+
+	return __dax_map_bh(bh, blkbits, &pfn, NULL);
+}
+
+static void dax_unmap_bh(const struct buffer_head *bh, void __pmem *addr)
+{
+	struct block_device *bdev = bh->b_bdev;
+	struct request_queue *q = bdev->bd_queue;
+
+	if (IS_ERR(addr))
+		return;
+	blk_dax_put(q);
 }
 
 /* the clear_pmem() calls are ordered by a wmb_pmem() in the caller */
@@ -104,15 +135,16 @@  static ssize_t dax_io(struct inode *inode, struct iov_iter *iter,
 		      loff_t start, loff_t end, get_block_t get_block,
 		      struct buffer_head *bh)
 {
-	ssize_t retval = 0;
-	loff_t pos = start;
-	loff_t max = start;
-	loff_t bh_max = start;
-	void __pmem *addr;
+	loff_t pos = start, max = start, bh_max = start;
+	int rw = iov_iter_rw(iter), rc;
+	long map_len = 0;
+	unsigned long pfn;
+	void __pmem *addr = NULL;
+	void __pmem *kmap = (void __pmem *) ERR_PTR(-EIO);
 	bool hole = false;
 	bool need_wmb = false;
 
-	if (iov_iter_rw(iter) != WRITE)
+	if (rw == READ)
 		end = min(end, i_size_read(inode));
 
 	while (pos < end) {
@@ -127,9 +159,8 @@  static ssize_t dax_io(struct inode *inode, struct iov_iter *iter,
 			if (pos == bh_max) {
 				bh->b_size = PAGE_ALIGN(end - pos);
 				bh->b_state = 0;
-				retval = get_block(inode, block, bh,
-						   iov_iter_rw(iter) == WRITE);
-				if (retval)
+				rc = get_block(inode, block, bh, rw == WRITE);
+				if (rc)
 					break;
 				if (!buffer_size_valid(bh))
 					bh->b_size = 1 << blkbits;
@@ -141,21 +172,25 @@  static ssize_t dax_io(struct inode *inode, struct iov_iter *iter,
 				bh->b_size -= done;
 			}
 
-			hole = iov_iter_rw(iter) != WRITE && !buffer_written(bh);
+			hole = rw == READ && !buffer_written(bh);
 			if (hole) {
 				addr = NULL;
 				size = bh->b_size - first;
 			} else {
-				retval = dax_get_addr(bh, &addr, blkbits);
-				if (retval < 0)
+				dax_unmap_bh(bh, kmap);
+				kmap = __dax_map_bh(bh, blkbits, &pfn, &map_len);
+				if (IS_ERR(kmap)) {
+					rc = PTR_ERR(kmap);
 					break;
+				}
+				addr = kmap;
 				if (buffer_unwritten(bh) || buffer_new(bh)) {
-					dax_new_buf(addr, retval, first, pos,
-									end);
+					dax_new_buf(addr, map_len, first, pos,
+							end);
 					need_wmb = true;
 				}
 				addr += first;
-				size = retval - first;
+				size = map_len - first;
 			}
 			max = min(pos + size, end);
 		}
@@ -178,8 +213,9 @@  static ssize_t dax_io(struct inode *inode, struct iov_iter *iter,
 
 	if (need_wmb)
 		wmb_pmem();
+	dax_unmap_bh(bh, kmap);
 
-	return (pos == start) ? retval : pos - start;
+	return (pos == start) ? rc : pos - start;
 }
 
 /**
@@ -274,21 +310,22 @@  static int copy_user_bh(struct page *to, struct buffer_head *bh,
 	void __pmem *vfrom;
 	void *vto;
 
-	if (dax_get_addr(bh, &vfrom, blkbits) < 0)
-		return -EIO;
+	vfrom = dax_map_bh(bh, blkbits);
+	if (IS_ERR(vfrom))
+		return PTR_ERR(vfrom);
 	vto = kmap_atomic(to);
 	copy_user_page(vto, (void __force *)vfrom, vaddr, to);
 	kunmap_atomic(vto);
+	dax_unmap_bh(bh, vfrom);
 	return 0;
 }
 
 static int dax_insert_mapping(struct inode *inode, struct buffer_head *bh,
 			struct vm_area_struct *vma, struct vm_fault *vmf)
 {
-	sector_t sector = bh->b_blocknr << (inode->i_blkbits - 9);
 	unsigned long vaddr = (unsigned long)vmf->virtual_address;
-	void __pmem *addr;
 	unsigned long pfn;
+	void __pmem *addr;
 	pgoff_t size;
 	int error;
 
@@ -305,11 +342,9 @@  static int dax_insert_mapping(struct inode *inode, struct buffer_head *bh,
 		goto out;
 	}
 
-	error = bdev_direct_access(bh->b_bdev, sector, &addr, &pfn, bh->b_size);
-	if (error < 0)
-		goto out;
-	if (error < PAGE_SIZE) {
-		error = -EIO;
+	addr = __dax_map_bh(bh, inode->i_blkbits, &pfn, NULL);
+	if (IS_ERR(addr)) {
+		error = PTR_ERR(addr);
 		goto out;
 	}
 
@@ -317,6 +352,7 @@  static int dax_insert_mapping(struct inode *inode, struct buffer_head *bh,
 		clear_pmem(addr, PAGE_SIZE);
 		wmb_pmem();
 	}
+	dax_unmap_bh(bh, addr);
 
 	error = vm_insert_mixed(vma, vaddr, pfn);
 
@@ -528,11 +564,8 @@  int __dax_pmd_fault(struct vm_area_struct *vma, unsigned long address,
 	unsigned blkbits = inode->i_blkbits;
 	unsigned long pmd_addr = address & PMD_MASK;
 	bool write = flags & FAULT_FLAG_WRITE;
-	long length;
-	void __pmem *kaddr;
 	pgoff_t size, pgoff;
-	sector_t block, sector;
-	unsigned long pfn;
+	sector_t block;
 	int result = 0;
 
 	/* Fall back to PTEs if we're going to COW */
@@ -557,8 +590,7 @@  int __dax_pmd_fault(struct vm_area_struct *vma, unsigned long address,
 
 	bh.b_size = PMD_SIZE;
 	i_mmap_lock_write(mapping);
-	length = get_block(inode, block, &bh, write);
-	if (length)
+	if (get_block(inode, block, &bh, write) != 0)
 		return VM_FAULT_SIGBUS;
 
 	/*
@@ -569,17 +601,17 @@  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;
+		long length;
+		unsigned long pfn;
+		void __pmem *kaddr = __dax_map_bh(&bh, blkbits, &pfn, &length);
 
-		length = bdev_direct_access(bh.b_bdev, sector, &kaddr, &pfn,
-						bh.b_size);
-		if (length < 0) {
+		if (IS_ERR(kaddr)) {
 			result = VM_FAULT_SIGBUS;
 			goto out;
 		}
+
 		if ((length < PMD_SIZE) || (pfn & PG_PMD_COLOUR))
 			goto fallback;
 
@@ -589,6 +621,7 @@  int __dax_pmd_fault(struct vm_area_struct *vma, unsigned long address,
 		count_vm_event(PGMAJFAULT);
 		mem_cgroup_count_vm_event(vma->vm_mm, PGMAJFAULT);
 		result |= VM_FAULT_MAJOR;
+		dax_unmap_bh(&bh, kaddr);
 	}
 
 	/*
@@ -635,12 +668,15 @@  int __dax_pmd_fault(struct vm_area_struct *vma, unsigned long address,
 		result = VM_FAULT_NOPAGE;
 		spin_unlock(ptl);
 	} else {
-		length = bdev_direct_access(bh.b_bdev, sector, &kaddr, &pfn,
-						bh.b_size);
-		if (length < 0) {
+		long length;
+		unsigned long pfn;
+		void __pmem *kaddr = __dax_map_bh(&bh, blkbits, &pfn, &length);
+
+		if (IS_ERR(kaddr)) {
 			result = VM_FAULT_SIGBUS;
 			goto out;
 		}
+		dax_unmap_bh(&bh, kaddr);
 		if ((length < PMD_SIZE) || (pfn & PG_PMD_COLOUR))
 			goto fallback;
 
@@ -746,12 +782,13 @@  int dax_zero_page_range(struct inode *inode, loff_t from, unsigned length,
 	if (err < 0)
 		return err;
 	if (buffer_written(&bh)) {
-		void __pmem *addr;
-		err = dax_get_addr(&bh, &addr, inode->i_blkbits);
-		if (err < 0)
-			return err;
+		void __pmem *addr = dax_map_bh(&bh, inode->i_blkbits);
+
+		if (IS_ERR(addr))
+			return PTR_ERR(addr);
 		clear_pmem(addr + offset, length);
 		wmb_pmem();
+		dax_unmap_bh(&bh, addr);
 	}
 
 	return 0;