On Thu 14-03-24 20:37:38, Baokun Li wrote:
> On 2024/3/14 20:00, Jan Kara wrote:
> > On Thu 14-03-24 19:24:56, Baokun Li wrote:
> > > Hi Jan,
> > >
> > > On 2024/3/14 18:30, Jan Kara wrote:
> > > > On Tue 27-02-24 17:11:43, Baokun Li wrote:
> > > >
> > > >
> > > > At 4k block size, the length of the s_mb_avg_fragment_size list is 14,
> > > > but an oversized s_mb_group_prealloc is set, causing slab-out-of-bounds
> > > > to be triggered by an attempt to access an element at index 29.
> > > >
> > > > Add a new attr_id attr_clusters_in_group with values in the range
> > > > [0, sbi->s_clusters_per_group] and declare mb_group_prealloc as
> > > > that type to fix the issue. In addition avoid returning an order
> > > > from mb_avg_fragment_size_order() greater than MB_NUM_ORDERS(sb)
> > > > and reduce some useless loops.
> > > >
> > > > Fixes: 7e170922f06b ("ext4: Add allocation criteria 1.5 (CR1_5)")
> > > > CC: stable@xxxxxxxxxxxxxxx
> > > > Signed-off-by: Baokun Li <libaokun1@xxxxxxxxxx>
> > > > Looks good. Just one nit below. Otherwise feel free to add:
> > > >
> > > > Reviewed-by: Jan Kara <jack@xxxxxxx>
> > > >
> > > > > ---
> > > > > fs/ext4/mballoc.c | 6 ++++++
> > > > > fs/ext4/sysfs.c | 13 ++++++++++++-
> > > > > 2 files changed, 18 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> > > > > index 85a91a61b761..7ad089df2408 100644
> > > > > --- a/fs/ext4/mballoc.c
> > > > > +++ b/fs/ext4/mballoc.c
> > > > > @@ -831,6 +831,8 @@ static int mb_avg_fragment_size_order(struct super_block *sb, ext4_grpblk_t len)
> > > > > return 0;
> > > > > if (order == MB_NUM_ORDERS(sb))
> > > > > order--;
> > > > > + if (WARN_ON_ONCE(order > MB_NUM_ORDERS(sb)))
> > > > > + order = MB_NUM_ORDERS(sb) - 1;
> > > > > return order;
> > > > > }
> > > > > @@ -1057,6 +1059,10 @@ static void ext4_mb_choose_next_group_best_avail(struct ext4_allocation_context
> > > > > ac->ac_flags |= EXT4_MB_CR_BEST_AVAIL_LEN_OPTIMIZED;
> > > > > return;
> > > > > }
> > > > > +
> > > > > + /* Skip some unnecessary loops. */
> > > > > + if (unlikely(i > MB_NUM_ORDERS(ac->ac_sb)))
> > > > > + i = MB_NUM_ORDERS(ac->ac_sb);
> > > > How can this possibly trigger now? MB_NUM_ORDERS is sb->s_blocksize_bits +
> > > > 2. 'i' is starting at fls(ac->ac_g_ex.fe_len) and ac_g_ex.fe_len shouldn't
> > > > be larger than clusters per group, hence fls() should be less than
> > > > sb->s_blocksize_bits? Am I missing something? And if yes, we should rather
> > > > make sure 'order' is never absurdly big?
> > > >
> > > > I suspect this code is defensive upto a point of being confusing :)
> > > >
> > > > Honza
> > > Yes, this is indeed defensive code! Only walk into this branch when
> > > WARN_ON_ONCE(order > MB_NUM_ORDERS(sb)) is triggered.
> > > As previously mentioned by ojaswin in the following link:
> > >
> > > "The reason for this is that otherwise when order is large eg 29,
> > > we would unnecessarily loop from i=29 to i=13 while always
> > > looking at the same avg_fragment_list[13]."
> > >
> > > Link：https://lore.kernel.org/lkml/ZdQ7FEA7KC4eAMpg@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
> > >
> > > Thank you so much for the review! ღ( ´･ᴗ･` )
> > Thanks for the link. So what Ojaswin has suggested has been slightly
> > different though. He suggested to trim the order before the for loop, not
> > after the first iteration as you do which is what was confusing me. I'd
> > even suggest to replace your check with:
> >
> > /*
> > * mb_avg_fragment_size_order() returns order in a way that makes
> > * retrieving back the length using (1 << order) inaccurate. Hence, use
> > * fls() instead since we need to know the actual length while modifying
> > * goal length.
> > */
> > - order = fls(ac->ac_g_ex.fe_len) - 1;
> > + order = min(fls(ac->ac_g_ex.fe_len), MB_NUM_ORDERS(ac->ac_sb)) - 1;
> > min_order = order - sbi->s_mb_best_avail_max_trim_order;
> > if (min_order < 0)
> > min_order = 0;
> >
> > Honza
> Yes, I changed it that way because it only happens when an exception
> somewhere causes fe_len to be a huge value. I think in this case we
> should report the exception via WARN_ON_ONCE(), and trimming the
> order before the for loop will bypass WARN_ON_ONCE and not report
> any errors.
Fair enough. Then:
/*
* mb_avg_fragment_size_order() returns order in a way that makes
* retrieving back the length using (1 << order) inaccurate. Hence, use
* fls() instead since we need to know the actual length while modifying
* goal length.
*/
order = fls(ac->ac_g_ex.fe_len) - 1;
+ if (WARN_ON_ONCE(order > MB_NUM_ORDERS(ac->ac_sb) - 1))
+ order = MB_NUM_ORDERS(ac->ac_sb) - 1;
min_order = order - sbi->s_mb_best_avail_max_trim_order;
if (min_order < 0)
min_order = 0;
Still much less confusing...
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
