Re: [PATCH] ext4: fix bug_on in __es_tree_search

[Jan Kara]

On Wed 18-05-22 20:08:16, Baokun Li wrote:
> Hulk Robot reported a BUG_ON:
> ==================================================================
> kernel BUG at fs/ext4/extents_status.c:199!
> [...]
> RIP: 0010:ext4_es_end fs/ext4/extents_status.c:199 [inline]
> RIP: 0010:__es_tree_search+0x1e0/0x260 fs/ext4/extents_status.c:217
> [...]
> Call Trace:
>  ext4_es_cache_extent+0x109/0x340 fs/ext4/extents_status.c:766
>  ext4_cache_extents+0x239/0x2e0 fs/ext4/extents.c:561
>  ext4_find_extent+0x6b7/0xa20 fs/ext4/extents.c:964
>  ext4_ext_map_blocks+0x16b/0x4b70 fs/ext4/extents.c:4384
>  ext4_map_blocks+0xe26/0x19f0 fs/ext4/inode.c:567
>  ext4_getblk+0x320/0x4c0 fs/ext4/inode.c:980
>  ext4_bread+0x2d/0x170 fs/ext4/inode.c:1031
>  ext4_quota_read+0x248/0x320 fs/ext4/super.c:6257
>  v2_read_header+0x78/0x110 fs/quota/quota_v2.c:63
>  v2_check_quota_file+0x76/0x230 fs/quota/quota_v2.c:82
>  vfs_load_quota_inode+0x5d1/0x1530 fs/quota/dquot.c:2368
>  dquot_enable+0x28a/0x330 fs/quota/dquot.c:2490
>  ext4_quota_enable fs/ext4/super.c:6137 [inline]
>  ext4_enable_quotas+0x5d7/0x960 fs/ext4/super.c:6163
>  ext4_fill_super+0xa7c9/0xdc00 fs/ext4/super.c:4754
>  mount_bdev+0x2e9/0x3b0 fs/super.c:1158
>  mount_fs+0x4b/0x1e4 fs/super.c:1261
> [...]
> ==================================================================
> 
> Above issue may happen as follows:
> -------------------------------------
> ext4_fill_super
>  ext4_enable_quotas
>   ext4_quota_enable
>    ext4_iget
>     __ext4_iget
>      ext4_ext_check_inode
>       ext4_ext_check
>        __ext4_ext_check
>         ext4_valid_extent_entries
>          Check for overlapping extents does't take effect
>    dquot_enable
>     vfs_load_quota_inode
>      v2_check_quota_file
>       v2_read_header
>        ext4_quota_read
>         ext4_bread
>          ext4_getblk
>           ext4_map_blocks
>            ext4_ext_map_blocks
>             ext4_find_extent
>              ext4_cache_extents
>               ext4_es_cache_extent
>                ext4_es_cache_extent
>                 __es_tree_search
>                  ext4_es_end
>                   BUG_ON(es->es_lblk + es->es_len < es->es_lblk)
> 
> The error ext4 extents is as follows:
> 0af3 0300 0400 0000 00000000    extent_header
> 00000000 0100 0000 12000000     extent1
> 00000000 0100 0000 18000000     extent2
> 02000000 0400 0000 14000000     extent3
> 
> In the ext4_valid_extent_entries function,
> if prev is 0, no error is returned even if lblock<=prev.
> This was intended to skip the check on the first extent, but
> in the error image above, prev=0+1-1=0 when checking the second extent,
> so even though lblock<=prev, the function does not return an error.
> As a result, bug_ON occurs in __es_tree_search and the system panics.
> 
> To solve this problem, we only need to check that:
> 1. The lblock of the first extent is not less than 0.
> 2. The lblock of the next extent  is not less than
>    the next block of the previous extent.
> The same applies to extent_idx.
> 
> Fixes: 5946d089379a ("ext4: check for overlapping extents in ext4_valid_extent_entries()")
> Reported-by: Hulk Robot <hulkci@xxxxxxxxxx>
> Signed-off-by: Baokun Li <libaokun1@xxxxxxxxxx>

Thanks! The patch looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza


> ---
>  fs/ext4/extents.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index e473fde6b64b..86ba0db20968 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -372,7 +372,7 @@ static int ext4_valid_extent_entries(struct inode *inode,
>  {
>  	unsigned short entries;
>  	ext4_lblk_t lblock = 0;
> -	ext4_lblk_t prev = 0;
> +	ext4_lblk_t cur = 0;
>  
>  	if (eh->eh_entries == 0)
>  		return 1;
> @@ -396,11 +396,11 @@ static int ext4_valid_extent_entries(struct inode *inode,
>  
>  			/* Check for overlapping extents */
>  			lblock = le32_to_cpu(ext->ee_block);
> -			if ((lblock <= prev) && prev) {
> +			if (lblock < cur) {
>  				*pblk = ext4_ext_pblock(ext);
>  				return 0;
>  			}
> -			prev = lblock + ext4_ext_get_actual_len(ext) - 1;
> +			cur = lblock + ext4_ext_get_actual_len(ext);
>  			ext++;
>  			entries--;
>  		}
> @@ -420,13 +420,13 @@ static int ext4_valid_extent_entries(struct inode *inode,
>  
>  			/* Check for overlapping index extents */
>  			lblock = le32_to_cpu(ext_idx->ei_block);
> -			if ((lblock <= prev) && prev) {
> +			if (lblock < cur) {
>  				*pblk = ext4_idx_pblock(ext_idx);
>  				return 0;
>  			}
>  			ext_idx++;
>  			entries--;
> -			prev = lblock;
> +			cur = lblock + 1;
>  		}
>  	}
>  	return 1;
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR