linux-kernel - Re: [PATCH 1/2] ext4: Check stripe size compatibility on remount as well

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: <2475f15a-d332-bef1-4ea6-70461f7ef3bb@huaweicloud.com>
Date: Mon, 19 Aug 2024 20:31:26 +0800
From: Kemeng Shi <shikemeng@...weicloud.com>
To: Ojaswin Mujoo <ojaswin@...ux.ibm.com>, linux-ext4@...r.kernel.org,
 Theodore Ts'o <tytso@....edu>
Cc: Ritesh Harjani <ritesh.list@...il.com>, linux-kernel@...r.kernel.org,
 syzbot+1ad8bac5af24d01e2cbd@...kaller.appspotmail.com
Subject: Re: [PATCH 1/2] ext4: Check stripe size compatibility on remount as
 well



on 8/16/2024 3:57 PM, Ojaswin Mujoo wrote:
> We disable stripe size in __ext4_fill_super if it is not a multiple of
> the cluster ratio however this check is missed when trying to remount.
> This can leave us with cases where stripe < cluster_ratio after
> remount:set making EXT4_B2C(sbi->s_stripe) become 0 that can cause some
> unforeseen bugs like divide by 0.
> 
> Fix that by adding the check in remount path as well.
> 
> Additionally, change the users of EXT4_B2C(sbi->s_stripe) to
> EXT4_NUM_B2C() so that if we ever accidentally hit this again, we can
> avoid the value becoming 0. This should not change existing functionality.
It's better to mention this change is in following patch or simply remove
it from this patch.

Other than that, looks good to me. Feel free to add:

Reviewed-by: Kemeng Shi <shikemeng@...weicloud.com>
> 
> Reported-by: syzbot+1ad8bac5af24d01e2cbd@...kaller.appspotmail.com
> Tested-by: syzbot+1ad8bac5af24d01e2cbd@...kaller.appspotmail.com
> Fixes: c3defd99d58c ("ext4: treat stripe in block unit")
> Signed-off-by: Ojaswin Mujoo <ojaswin@...ux.ibm.com>
> ---
>  fs/ext4/super.c | 29 ++++++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index e72145c4ae5a..9d495d78d262 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5165,6 +5165,18 @@ static int ext4_block_group_meta_init(struct super_block *sb, int silent)
>  	return 0;
>  }
>  
> +/*
> + * It's hard to get stripe aligned blocks if stripe is not aligned with
> + * cluster, just disable stripe and alert user to simpfy code and avoid
> + * stripe aligned allocation which will rarely successes.
> + */
> +static bool ext4_is_stripe_incompatible(struct super_block *sb, unsigned long stripe)
> +{
> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +	return (stripe > 0 && sbi->s_cluster_ratio > 1 &&
> +		stripe % sbi->s_cluster_ratio != 0);
> +}
> +
>  static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  {
>  	struct ext4_super_block *es = NULL;
> @@ -5272,13 +5284,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  		goto failed_mount3;
>  
>  	sbi->s_stripe = ext4_get_stripe_size(sbi);
> -	/*
> -	 * It's hard to get stripe aligned blocks if stripe is not aligned with
> -	 * cluster, just disable stripe and alert user to simpfy code and avoid
> -	 * stripe aligned allocation which will rarely successes.
> -	 */
> -	if (sbi->s_stripe > 0 && sbi->s_cluster_ratio > 1 &&
> -	    sbi->s_stripe % sbi->s_cluster_ratio != 0) {
> +	if (ext4_is_stripe_incompatible(sb, sbi->s_stripe)) {
>  		ext4_msg(sb, KERN_WARNING,
>  			 "stripe (%lu) is not aligned with cluster size (%u), "
>  			 "stripe is disabled",
> @@ -6441,6 +6447,15 @@ static int __ext4_remount(struct fs_context *fc, struct super_block *sb)
>  
>  	}
>  
> +	if ((ctx->spec & EXT4_SPEC_s_stripe) &&
> +	    ext4_is_stripe_incompatible(sb, ctx->s_stripe)) {
> +		ext4_msg(sb, KERN_WARNING,
> +			 "stripe (%lu) is not aligned with cluster size (%u), "
> +			 "stripe is disabled",
> +			 ctx->s_stripe, sbi->s_cluster_ratio);
> +		ctx->s_stripe = 0;
> +	}
> +
>  	/*
>  	 * Changing the DIOREAD_NOLOCK or DELALLOC mount options may cause
>  	 * two calls to ext4_should_dioread_nolock() to return inconsistent
>