Re: [PATCH] Btrfs: fix segment fault when doing dio read

[Nikolay Borisov <n.borisov.lkml@gmail.com>]

On  7.04.2017 23:11, Liu Bo wrote:
> Commit 2dabb3248453 ("Btrfs: Direct I/O read: Work on sectorsized blocks")
> introduced this bug during iterating bio pages in dio read's endio hook,
> and it could end up with segment fault of the dio reading task.
> 
> So the reason is 'if (nr_sectors--)', and it makes the code assume that
> there is one more block in the same page, so page offset is increased and
> the bio which is created to repair the bad block then has an incorrect
> bvec.bv_offset, and a later access of the page content would throw a
> segment fault.
> 
> This also adds ASSERT to check page offset against page size.
> 
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> ---
>  fs/btrfs/inode.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index c875e68..5e71f1e 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -7972,8 +7972,10 @@ static int __btrfs_correct_data_nocsum(struct inode *inode,
>  
>  		start += sectorsize;
>  
> -		if (nr_sectors--) {
> +		nr_sectors--;
Why not if(--nr_sectors)? I know it's more of a style issue but it will
reduce the size of the diff ?
> +		if (nr_sectors) {
>  			pgoff += sectorsize;
> +			ASSERT(pgoff < PAGE_SIZE);
>  			goto next_block_or_try_again;
>  		}
>  	}
> @@ -8074,8 +8076,10 @@ static int __btrfs_subio_endio_read(struct inode *inode,
>  
>  		ASSERT(nr_sectors);
>  
> -		if (--nr_sectors) {
> +		nr_sectors--;
> +		if (nr_sectors) {
>  			pgoff += sectorsize;
> +			ASSERT(pgoff < PAGE_SIZE);
>  			goto next_block;
>  		}
>  	}
>