lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20231218151455.yqph44iz4ihsujz5@quack3>
Date: Mon, 18 Dec 2023 16:14:55 +0100
From: Jan Kara <jack@...e.cz>
To: Baokun Li <libaokun1@...wei.com>
Cc: linux-ext4@...r.kernel.org, tytso@....edu, adilger.kernel@...ger.ca,
	jack@...e.cz, ritesh.list@...il.com, linux-kernel@...r.kernel.org,
	yi.zhang@...wei.com, yangerkun@...wei.com, yukuai3@...wei.com,
	stable@...r.kernel.org
Subject: Re: [PATCH 3/4] ext4: avoid bb_free and bb_fragments inconsistency
 in mb_free_blocks()

On Mon 18-12-23 22:18:13, Baokun Li wrote:
> After updating bb_free in mb_free_blocks, it is possible to return without
> updating bb_fragments because the block being freed is found to have
> already been freed, which leads to inconsistency between bb_free and
> bb_fragments.
> 
> Since the group may be unlocked in ext4_grp_locked_error(), this can lead
> to problems such as dividing by zero when calculating the average fragment
> length. Therefore, to ensure consistency, move the update of bb_free to
> after the block double-free check.
> 
> Fixes: eabe0444df90 ("ext4: speed-up releasing blocks on commit")
> CC: stable@...r.kernel.org # 3.10
> Signed-off-by: Baokun Li <libaokun1@...wei.com>

I agree there's no point in updating the allocation info if the bitmap is
corrupted. We will not try to allocate (or free) blocks in that group
anymore. I'm just a bit unsure about the EXT4_FC_REPLAY state where we
don't mark the bitmap as corrupted although some blocks were already marked
as freed. So in this case the free space statistics tracking will go
permanently wrong. I'm kind of wondering in which case does fast-commit
free already freed blocks. Ted, any idea?

								Honza


> ---
>  fs/ext4/mballoc.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index a95fa6e2b0f9..2fbee0f0f5c3 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -1892,11 +1892,6 @@ static void mb_free_blocks(struct inode *inode, struct ext4_buddy *e4b,
>  	mb_check_buddy(e4b);
>  	mb_free_blocks_double(inode, e4b, first, count);
>  
> -	this_cpu_inc(discard_pa_seq);
> -	e4b->bd_info->bb_free += count;
> -	if (first < e4b->bd_info->bb_first_free)
> -		e4b->bd_info->bb_first_free = first;
> -
>  	/* access memory sequentially: check left neighbour,
>  	 * clear range and then check right neighbour
>  	 */
> @@ -1922,9 +1917,14 @@ static void mb_free_blocks(struct inode *inode, struct ext4_buddy *e4b,
>  				sb, e4b->bd_group,
>  				EXT4_GROUP_INFO_BBITMAP_CORRUPT);
>  		}
> -		goto done;
> +		return;
>  	}
>  
> +	this_cpu_inc(discard_pa_seq);
> +	e4b->bd_info->bb_free += count;
> +	if (first < e4b->bd_info->bb_first_free)
> +		e4b->bd_info->bb_first_free = first;
> +
>  	/* let's maintain fragments counter */
>  	if (left_is_free && right_is_free)
>  		e4b->bd_info->bb_fragments--;
> @@ -1949,7 +1949,6 @@ static void mb_free_blocks(struct inode *inode, struct ext4_buddy *e4b,
>  	if (first <= last)
>  		mb_buddy_mark_free(e4b, first >> 1, last >> 1);
>  
> -done:
>  	mb_set_largest_free_order(sb, e4b->bd_info);
>  	mb_update_avg_fragment_size(sb, e4b->bd_info);
>  	mb_check_buddy(e4b);
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ