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

From: David Sterba <dsterba@suse.cz>
To: Liu Bo <bo.li.liu@oracle.com>
Cc: linux-btrfs@vger.kernel.org, David Sterba <dsterba@suse.cz>
Subject: Re: [PATCH] Btrfs: fix invalid dereference in btrfs_retry_endio
Date: Thu, 6 Apr 2017 16:21:50 +0200	[thread overview]
Message-ID: <20170406142150.GN4781@twin.jikos.cz> (raw)
In-Reply-To: <1491426259-27838-1-git-send-email-bo.li.liu@oracle.com>

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.