[PATCH v2 1/2] ext4: Check stripe size compatibility on remount as well

[PATCH v2 1/2] ext4: Check stripe size compatibility on remount as well

[Ojaswin Mujoo]

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.

Reported-by: syzbot+1ad8bac5af24d01e2cbd@syzkaller.appspotmail.com
Tested-by: syzbot+1ad8bac5af24d01e2cbd@syzkaller.appspotmail.com
Reviewed-by: Kemeng Shi <shikemeng@huaweicloud.com>
Fixes: c3defd99d58c ("ext4: treat stripe in block unit")
Signed-off-by: Ojaswin Mujoo <ojaswin@linux.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
-- 
2.43.5

[PATCH v2 2/2] ext4: Convert EXT4_B2C(sbi->s_stripe) users to EXT4_NUM_B2C

[Ojaswin Mujoo]

Although we have checks to make sure s_stripe is a multiple of cluster
size, in case we accidentally end up with a scenario where this is not
the case, use EXT4_NUM_B2C() so that we don't end up with unexpected
cases where EXT4_B2C(stripe) becomes 0.

Also make the is_stripe_aligned check in regular_allocator a bit more
robust while we are at it. This should ideally have no functional change
unless we have a bug somewhere causing (stripe % cluster_size != 0)

Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 fs/ext4/mballoc.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 9dda9cd68ab2..99d1a8c730e0 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -2553,7 +2553,7 @@ void ext4_mb_scan_aligned(struct ext4_allocation_context *ac,
 	do_div(a, sbi->s_stripe);
 	i = (a * sbi->s_stripe) - first_group_block;
 
-	stripe = EXT4_B2C(sbi, sbi->s_stripe);
+	stripe = EXT4_NUM_B2C(sbi, sbi->s_stripe);
 	i = EXT4_B2C(sbi, i);
 	while (i < EXT4_CLUSTERS_PER_GROUP(sb)) {
 		if (!mb_test_bit(i, bitmap)) {
@@ -2928,9 +2928,11 @@ ext4_mb_regular_allocator(struct ext4_allocation_context *ac)
 			if (cr == CR_POWER2_ALIGNED)
 				ext4_mb_simple_scan_group(ac, &e4b);
 			else {
-				bool is_stripe_aligned = sbi->s_stripe &&
+				bool is_stripe_aligned =
+					(sbi->s_stripe >=
+					 sbi->s_cluster_ratio) &&
 					!(ac->ac_g_ex.fe_len %
-					  EXT4_B2C(sbi, sbi->s_stripe));
+					  EXT4_NUM_B2C(sbi, sbi->s_stripe));
 
 				if ((cr == CR_GOAL_LEN_FAST ||
 				     cr == CR_BEST_AVAIL_LEN) &&
@@ -3707,7 +3709,7 @@ int ext4_mb_init(struct super_block *sb)
 	 */
 	if (sbi->s_stripe > 1) {
 		sbi->s_mb_group_prealloc = roundup(
-			sbi->s_mb_group_prealloc, EXT4_B2C(sbi, sbi->s_stripe));
+			sbi->s_mb_group_prealloc, EXT4_NUM_B2C(sbi, sbi->s_stripe));
 	}
 
 	sbi->s_locality_groups = alloc_percpu(struct ext4_locality_group);
-- 
2.43.5

Re: [PATCH v2 2/2] ext4: Convert EXT4_B2C(sbi->s_stripe) users to EXT4_NUM_B2C

[Kemeng Shi]

on 8/20/2024 5:27 PM, Ojaswin Mujoo wrote:
> Although we have checks to make sure s_stripe is a multiple of cluster
> size, in case we accidentally end up with a scenario where this is not
> the case, use EXT4_NUM_B2C() so that we don't end up with unexpected
> cases where EXT4_B2C(stripe) becomes 0.
> 
> Also make the is_stripe_aligned check in regular_allocator a bit more
> robust while we are at it. This should ideally have no functional change
> unless we have a bug somewhere causing (stripe % cluster_size != 0)
> 
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>Looks good to me. Feel free to add:

Reviewed-by: Kemeng Shi <shikemeng@huaweicloud.com>
> ---
>  fs/ext4/mballoc.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 9dda9cd68ab2..99d1a8c730e0 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -2553,7 +2553,7 @@ void ext4_mb_scan_aligned(struct ext4_allocation_context *ac,
>  	do_div(a, sbi->s_stripe);
>  	i = (a * sbi->s_stripe) - first_group_block;
>  
> -	stripe = EXT4_B2C(sbi, sbi->s_stripe);
> +	stripe = EXT4_NUM_B2C(sbi, sbi->s_stripe);
>  	i = EXT4_B2C(sbi, i);
>  	while (i < EXT4_CLUSTERS_PER_GROUP(sb)) {
>  		if (!mb_test_bit(i, bitmap)) {
> @@ -2928,9 +2928,11 @@ ext4_mb_regular_allocator(struct ext4_allocation_context *ac)
>  			if (cr == CR_POWER2_ALIGNED)
>  				ext4_mb_simple_scan_group(ac, &e4b);
>  			else {
> -				bool is_stripe_aligned = sbi->s_stripe &&
> +				bool is_stripe_aligned =
> +					(sbi->s_stripe >=
> +					 sbi->s_cluster_ratio) &&
>  					!(ac->ac_g_ex.fe_len %
> -					  EXT4_B2C(sbi, sbi->s_stripe));
> +					  EXT4_NUM_B2C(sbi, sbi->s_stripe));
>  
>  				if ((cr == CR_GOAL_LEN_FAST ||
>  				     cr == CR_BEST_AVAIL_LEN) &&
> @@ -3707,7 +3709,7 @@ int ext4_mb_init(struct super_block *sb)
>  	 */
>  	if (sbi->s_stripe > 1) {
>  		sbi->s_mb_group_prealloc = roundup(
> -			sbi->s_mb_group_prealloc, EXT4_B2C(sbi, sbi->s_stripe));
> +			sbi->s_mb_group_prealloc, EXT4_NUM_B2C(sbi, sbi->s_stripe));
>  	}
>  
>  	sbi->s_locality_groups = alloc_percpu(struct ext4_locality_group);
> 

Re: [PATCH v2 2/2] ext4: Convert EXT4_B2C(sbi->s_stripe) users to EXT4_NUM_B2C

[Ritesh Harjani]

Ojaswin Mujoo <ojaswin@linux.ibm.com> writes:

> Although we have checks to make sure s_stripe is a multiple of cluster
> size, in case we accidentally end up with a scenario where this is not
> the case, use EXT4_NUM_B2C() so that we don't end up with unexpected
> cases where EXT4_B2C(stripe) becomes 0.

man page of strip=n mount options says...
       stripe=n
              Number of file system blocks that mballoc will try to use
              for allocation size and alignment. For RAID5/6 systems
              this should be the number of data disks * RAID chunk size
              in file system blocks.

... So stripe is anyways the no. of filesystem blocks. Making it
EXT4_NUM_B2C() make sense to me.

However, there is one more user that remains in ext4_mb_find_by_goal(),
right?

-ritesh

>
> Also make the is_stripe_aligned check in regular_allocator a bit more
> robust while we are at it. This should ideally have no functional change
> unless we have a bug somewhere causing (stripe % cluster_size != 0)
>
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> ---
>  fs/ext4/mballoc.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)

Re: [PATCH v2 2/2] ext4: Convert EXT4_B2C(sbi->s_stripe) users to EXT4_NUM_B2C

[Ojaswin Mujoo]

On Wed, Aug 28, 2024 at 02:58:13PM +0530, Ritesh Harjani wrote:
> Ojaswin Mujoo <ojaswin@linux.ibm.com> writes:
> 
> > Although we have checks to make sure s_stripe is a multiple of cluster
> > size, in case we accidentally end up with a scenario where this is not
> > the case, use EXT4_NUM_B2C() so that we don't end up with unexpected
> > cases where EXT4_B2C(stripe) becomes 0.
> 
> man page of strip=n mount options says...
>        stripe=n
>               Number of file system blocks that mballoc will try to use
>               for allocation size and alignment. For RAID5/6 systems
>               this should be the number of data disks * RAID chunk size
>               in file system blocks.
> 
> ... So stripe is anyways the no. of filesystem blocks. Making it
> EXT4_NUM_B2C() make sense to me.
> 
> However, there is one more user that remains in ext4_mb_find_by_goal(),
> right?

Oh right, I'll fix that in v3. Thanks! 

> 
> -ritesh
> 
> >
> > Also make the is_stripe_aligned check in regular_allocator a bit more
> > robust while we are at it. This should ideally have no functional change
> > unless we have a bug somewhere causing (stripe % cluster_size != 0)
> >
> > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> > ---
> >  fs/ext4/mballoc.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)

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

[Ritesh Harjani]

Ojaswin Mujoo <ojaswin@linux.ibm.com> writes:

> 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.
>
> Reported-by: syzbot+1ad8bac5af24d01e2cbd@syzkaller.appspotmail.com
> Tested-by: syzbot+1ad8bac5af24d01e2cbd@syzkaller.appspotmail.com
> Reviewed-by: Kemeng Shi <shikemeng@huaweicloud.com>
> Fixes: c3defd99d58c ("ext4: treat stripe in block unit")
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> ---
>  fs/ext4/super.c | 29 ++++++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 7 deletions(-)

Minor nits below, but otherwise looks good to me.

Please feel free to add - 
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

>
> 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

s/simpfy/simplify

> + * stripe aligned allocation which will rarely successes.

s/successes/succeed

-ritesh

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

[Ojaswin Mujoo]

On Wed, Aug 28, 2024 at 03:03:39PM +0530, Ritesh Harjani wrote:
> Ojaswin Mujoo <ojaswin@linux.ibm.com> writes:
> 
> > 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.
> >
> > Reported-by: syzbot+1ad8bac5af24d01e2cbd@syzkaller.appspotmail.com
> > Tested-by: syzbot+1ad8bac5af24d01e2cbd@syzkaller.appspotmail.com
> > Reviewed-by: Kemeng Shi <shikemeng@huaweicloud.com>
> > Fixes: c3defd99d58c ("ext4: treat stripe in block unit")
> > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> > ---
> >  fs/ext4/super.c | 29 ++++++++++++++++++++++-------
> >  1 file changed, 22 insertions(+), 7 deletions(-)
> 
> Minor nits below, but otherwise looks good to me.
> 
> Please feel free to add - 
> Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

Thanks for the review Ritesh, I'll fix these in next revision

> 
> >
> > 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
> 
> s/simpfy/simplify
> 
> > + * stripe aligned allocation which will rarely successes.
> 
> s/successes/succeed
> 
> -ritesh