On Wed, Aug 14, 2024 at 10:26:47AM +0530, Ojaswin Mujoo wrote:
> On Wed, Aug 14, 2024 at 10:12:00AM +0800, Edward Adam Davis wrote:
> > Before determining that the goal length is a multiple of the stripe size,
> > check CR_GOAL_LEN_FAST and CR_BEST_AVAIL_LEN first.
> >
> > Fixes: 1f6bc02f1848 ("ext4: fallback to complex scan if aligned scan doesn't work")
> > Reported-and-tested-by: syzbot+1ad8bac5af24d01e2cbd@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Closes: https://syzkaller.appspot.com/bug?extid=1ad8bac5af24d01e2cbd
> > Signed-off-by: Edward Adam Davis <eadavis@xxxxxx>
> > ---
> > fs/ext4/mballoc.c | 7 +++----
> > 1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> > index 9dda9cd68ab2..451f92cde461 100644
> > --- a/fs/ext4/mballoc.c
> > +++ b/fs/ext4/mballoc.c
> > @@ -2928,13 +2928,12 @@ 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 = (cr == CR_GOAL_LEN_FAST ||
> > + cr == CR_BEST_AVAIL_LEN) && sbi->s_stripe &&
> > !(ac->ac_g_ex.fe_len %
> > EXT4_B2C(sbi, sbi->s_stripe));
>
> Hi Edward,
>
> Thanks for the patch. So I didn't get a chance to look at syszcaller
> report but assuming that EXT4_B2C(sbi, sbi->s_stripe) is becoming 0,
> I'm not understanding how is this patch fixing the bug?
>
> It just seems to short circuit the actual bug but we might still hit it
> right?
>
> As for EXT4_B2C(stripe) becoming zero, I have 2 observations:
>
> 1. We should definitely be using EXT4_NUM_B2C() here to make sure we
> don't get 0 if stripe is less than cluster size.
>
> 2. That being saidIm not sure if it's even possible for this to become zero
> because we do check that stripe size is a multiple of cluster size in
> ext4_fill_super, else we disable it:
So I figured out the issue. It is indeed possible for this to happen
since we forgot to add the below check on remount path in the following
patch:
c3defd99d58c ("ext4: treat stripe in block unit")
The patch at the end of the mail should fix this issue. Once syscaller
tests it I'll send out the patch addressing this as well as making the
change mentioned in point 1 of previous email.
>
> /*
> * 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) {
> ext4_msg(sb, KERN_WARNING,
> "stripe (%lu) is not aligned with cluster size (%u), "
> "stripe is disabled",
> sbi->s_stripe, sbi->s_cluster_ratio);
> sbi->s_stripe = 0;
> }
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.
#syz test: https://github.com/torvalds/linux master
Reported-by: syzbot+1ad8bac5af24d01e2cbd@xxxxxxxxxxxxxxxxxxxxxxxxx
Fixes: c3defd99d58c ("ext4: treat stripe in block unit")
Signed-off-by: Ojaswin Mujoo <ojaswin@xxxxxxxxxxxxx>
---
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..8ca6bbc337a6 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.39.3
