Re: [PATCH v2 4/9] ext4: fix slab-out-of-bounds in ext4_mb_find_good_group_avg_frag_lists()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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! ღ( ´・ᴗ・` )
--
With Best Regards,
Baokun Li
.




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux