[PATCH] Btrfs: fix invalid dereference in btrfs_retry_endio

[PATCH] Btrfs: fix invalid dereference in btrfs_retry_endio

[Liu Bo]

When doing directIO repair, we have this oops

[ 1458.532816] general protection fault: 0000 [#1] SMP
...
[ 1458.536291] Workqueue: btrfs-endio-repair btrfs_endio_repair_helper [btrfs]
[ 1458.536893] task: ffff88082a42d100 task.stack: ffffc90002b3c000
[ 1458.537499] RIP: 0010:btrfs_retry_endio+0x7e/0x1a0 [btrfs]
...
[ 1458.543261] Call Trace:
[ 1458.543958]  ? rcu_read_lock_sched_held+0xc4/0xd0
[ 1458.544374]  bio_endio+0xed/0x100
[ 1458.544750]  end_workqueue_fn+0x3c/0x40 [btrfs]
[ 1458.545257]  normal_work_helper+0x9f/0x900 [btrfs]
[ 1458.545762]  btrfs_endio_repair_helper+0x12/0x20 [btrfs]
[ 1458.546224]  process_one_work+0x34d/0xb70
[ 1458.546570]  ? process_one_work+0x29e/0xb70
[ 1458.546938]  worker_thread+0x1cf/0x960
[ 1458.547263]  ? process_one_work+0xb70/0xb70
[ 1458.547624]  kthread+0x17d/0x180
[ 1458.547909]  ? kthread_create_on_node+0x70/0x70
[ 1458.548300]  ret_from_fork+0x31/0x40

It turns out that btrfs_retry_endio is trying to get inode from a directIO
page.

This fixes the problem by using the preservd inode pointer, done->inode.
btrfs_retry_endio_nocsum has the same problem, and it's fixed as well.

Also cleanup unused @start(which is too trivial for a separate patch).

Cc: David Sterba <dsterba@suse.cz>
Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 fs/btrfs/inode.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 3ec5a05..8e71ed7 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -7910,7 +7910,6 @@ struct btrfs_retry_complete {
 static void btrfs_retry_endio_nocsum(struct bio *bio)
 {
 	struct btrfs_retry_complete *done = bio->bi_private;
-	struct inode *inode;
 	struct bio_vec *bvec;
 	int i;
 
@@ -7918,12 +7917,12 @@ static void btrfs_retry_endio_nocsum(struct bio *bio)
 		goto end;
 
 	ASSERT(bio->bi_vcnt == 1);
-	inode = bio->bi_io_vec->bv_page->mapping->host;
-	ASSERT(bio->bi_io_vec->bv_len == btrfs_inode_sectorsize(inode));
+	ASSERT(bio->bi_io_vec->bv_len == btrfs_inode_sectorsize(done->inode));
 
 	done->uptodate = 1;
 	bio_for_each_segment_all(bvec, bio, i)
-	clean_io_failure(BTRFS_I(done->inode), done->start, bvec->bv_page, 0);
+		clean_io_failure(BTRFS_I(done->inode), done->start,
+				 bvec->bv_page, 0);
 end:
 	complete(&done->done);
 	bio_put(bio);
@@ -7986,9 +7985,7 @@ static void btrfs_retry_endio(struct bio *bio)
 {
 	struct btrfs_retry_complete *done = bio->bi_private;
 	struct btrfs_io_bio *io_bio = btrfs_io_bio(bio);
-	struct inode *inode;
 	struct bio_vec *bvec;
-	u64 start;
 	int uptodate;
 	int ret;
 	int i;
@@ -7998,11 +7995,8 @@ static void btrfs_retry_endio(struct bio *bio)
 
 	uptodate = 1;
 
-	start = done->start;
-
 	ASSERT(bio->bi_vcnt == 1);
-	inode = bio->bi_io_vec->bv_page->mapping->host;
-	ASSERT(bio->bi_io_vec->bv_len == btrfs_inode_sectorsize(inode));
+	ASSERT(bio->bi_io_vec->bv_len == btrfs_inode_sectorsize(done->inode));
 
 	bio_for_each_segment_all(bvec, bio, i) {
 		ret = __readpage_endio_check(done->inode, io_bio, i,
-- 
2.5.5

Re: [PATCH] Btrfs: fix invalid dereference in btrfs_retry_endio

[David Sterba]

On Wed, Apr 05, 2017 at 02:04:19PM -0700, Liu Bo wrote:
> When doing directIO repair, we have this oops
> 
> [ 1458.532816] general protection fault: 0000 [#1] SMP
> ...
> [ 1458.536291] Workqueue: btrfs-endio-repair btrfs_endio_repair_helper [btrfs]
> [ 1458.536893] task: ffff88082a42d100 task.stack: ffffc90002b3c000
> [ 1458.537499] RIP: 0010:btrfs_retry_endio+0x7e/0x1a0 [btrfs]
> ...
> [ 1458.543261] Call Trace:
> [ 1458.543958]  ? rcu_read_lock_sched_held+0xc4/0xd0
> [ 1458.544374]  bio_endio+0xed/0x100
> [ 1458.544750]  end_workqueue_fn+0x3c/0x40 [btrfs]
> [ 1458.545257]  normal_work_helper+0x9f/0x900 [btrfs]
> [ 1458.545762]  btrfs_endio_repair_helper+0x12/0x20 [btrfs]
> [ 1458.546224]  process_one_work+0x34d/0xb70
> [ 1458.546570]  ? process_one_work+0x29e/0xb70
> [ 1458.546938]  worker_thread+0x1cf/0x960
> [ 1458.547263]  ? process_one_work+0xb70/0xb70
> [ 1458.547624]  kthread+0x17d/0x180
> [ 1458.547909]  ? kthread_create_on_node+0x70/0x70
> [ 1458.548300]  ret_from_fork+0x31/0x40
> 
> It turns out that btrfs_retry_endio is trying to get inode from a directIO
> page.
> 
> This fixes the problem by using the preservd inode pointer, done->inode.
> btrfs_retry_endio_nocsum has the same problem, and it's fixed as well.
> 
> Also cleanup unused @start(which is too trivial for a separate patch).
> 
> Cc: David Sterba <dsterba@suse.cz>
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>

Reviewed-by: David Sterba <dsterba@suse.com>

> ---
>  fs/btrfs/inode.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 3ec5a05..8e71ed7 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -7910,7 +7910,6 @@ struct btrfs_retry_complete {
>  static void btrfs_retry_endio_nocsum(struct bio *bio)
>  {
>  	struct btrfs_retry_complete *done = bio->bi_private;
> -	struct inode *inode;
>  	struct bio_vec *bvec;
>  	int i;
>  
> @@ -7918,12 +7917,12 @@ static void btrfs_retry_endio_nocsum(struct bio *bio)
>  		goto end;
>  
>  	ASSERT(bio->bi_vcnt == 1);
> -	inode = bio->bi_io_vec->bv_page->mapping->host;
> -	ASSERT(bio->bi_io_vec->bv_len == btrfs_inode_sectorsize(inode));
> +	ASSERT(bio->bi_io_vec->bv_len == btrfs_inode_sectorsize(done->inode));

I was interested how it got here in the first place, inode from mapping
hasn't been used in the initial support for DIO repair. So it was added
in the subpage blocksize preparation patchset (2dabb3248453b), slipped
through the review.