[PATCH v2 0/9] ext4: avoid sysfs variables overflow causing BUG_ON/SOOB

[PATCH v2 0/9] ext4: avoid sysfs variables overflow causing BUG_ON/SOOB

[Baokun Li]

Hello everyone,

This patchset is intended to avoid variables that can be modified via sysfs
from overflowing when stored or used and thus causing various problems.

"kvm-xfstests -c ext4/all -g auto" has been executed with no new failures.

V1->V2:
  Patch 1: Use kstrtouint() as suggested by Alexey and Honza.
  Patch 2: Adapted to patch 1 changes.
  Patch 3: Add Reviewed-by tag.
  Patch 4: Avoid useless loops as suggested by Ojaswin and rename
	   attr_group_prealloc to attr_clusters_in_group.
  Patch 5: New patch added to limit mb_best_avail_max_trim_order < 64
	   as Honza's suggestion. 
  Patch 6: Reordered and updated description.
  Patch 7: Add Reviewed-by tag.
  Patch 8: Keep unrelated variables on different lines as suggested by Honza.
  Patch 9: New patch to fix warnings found during compile checking.

[V1]: https://lore.kernel.org/all/20240126085716.1363019-1-libaokun1@huawei.com/

Baokun Li (9):
  ext4: avoid overflow when setting values via sysfs
  ext4: refactor out ext4_generic_attr_store()
  ext4: refactor out ext4_generic_attr_show()
  ext4: fix slab-out-of-bounds in
    ext4_mb_find_good_group_avg_frag_lists()
  ext4: add new attr pointer attr_mb_order
  ext4: add positive int attr pointer to avoid sysfs variables overflow
  ext4: set type of ac_groups_linear_remaining to __u32 to avoid
    overflow
  ext4: set the type of max_zeroout to unsigned int to avoid overflow
  ext4: clean up s_mb_rb_lock to fix build warnings with C=1

 fs/ext4/extents.c |   3 +-
 fs/ext4/mballoc.c |   7 +-
 fs/ext4/mballoc.h |   2 +-
 fs/ext4/sysfs.c   | 174 ++++++++++++++++++++++++++++------------------
 4 files changed, 114 insertions(+), 72 deletions(-)

-- 
2.31.1

[PATCH v2 1/9] ext4: avoid overflow when setting values via sysfs

[Baokun Li]

When setting values of type unsigned int through sysfs, we use kstrtoul()
to parse it and then truncate part of it as the final set value, when the
set value is greater than UINT_MAX, the set value will not match what we
see because of the truncation. As follows:

  $ echo 4294967296 > /sys/fs/ext4/sda/mb_max_linear_groups
  $ cat /sys/fs/ext4/sda/mb_max_linear_groups
    0

So we use kstrtouint() to parse the attr_pointer_ui type to avoid the
inconsistency described above. In addition, a judgment is added to avoid
setting s_resv_clusters less than 0.

Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/sysfs.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
index 6d332dff79dd..ca820620b974 100644
--- a/fs/ext4/sysfs.c
+++ b/fs/ext4/sysfs.c
@@ -104,7 +104,7 @@ static ssize_t reserved_clusters_store(struct ext4_sb_info *sbi,
 	int ret;
 
 	ret = kstrtoull(skip_spaces(buf), 0, &val);
-	if (ret || val >= clusters)
+	if (ret || val >= clusters || (s64)val < 0)
 		return -EINVAL;
 
 	atomic64_set(&sbi->s_resv_clusters, val);
@@ -451,7 +451,8 @@ static ssize_t ext4_attr_store(struct kobject *kobj,
 						s_kobj);
 	struct ext4_attr *a = container_of(attr, struct ext4_attr, attr);
 	void *ptr = calc_ptr(a, sbi);
-	unsigned long t;
+	unsigned int t;
+	unsigned long lt;
 	int ret;
 
 	switch (a->attr_id) {
@@ -460,7 +461,7 @@ static ssize_t ext4_attr_store(struct kobject *kobj,
 	case attr_pointer_ui:
 		if (!ptr)
 			return 0;
-		ret = kstrtoul(skip_spaces(buf), 0, &t);
+		ret = kstrtouint(skip_spaces(buf), 0, &t);
 		if (ret)
 			return ret;
 		if (a->attr_ptr == ptr_ext4_super_block_offset)
@@ -471,10 +472,10 @@ static ssize_t ext4_attr_store(struct kobject *kobj,
 	case attr_pointer_ul:
 		if (!ptr)
 			return 0;
-		ret = kstrtoul(skip_spaces(buf), 0, &t);
+		ret = kstrtoul(skip_spaces(buf), 0, &lt);
 		if (ret)
 			return ret;
-		*((unsigned long *) ptr) = t;
+		*((unsigned long *) ptr) = lt;
 		return len;
 	case attr_inode_readahead:
 		return inode_readahead_blks_store(sbi, buf, len);
-- 
2.31.1

Re: [PATCH v2 1/9] ext4: avoid overflow when setting values via sysfs

[Jan Kara]

On Tue 27-02-24 17:11:40, Baokun Li wrote:
> When setting values of type unsigned int through sysfs, we use kstrtoul()
> to parse it and then truncate part of it as the final set value, when the
> set value is greater than UINT_MAX, the set value will not match what we
> see because of the truncation. As follows:
> 
>   $ echo 4294967296 > /sys/fs/ext4/sda/mb_max_linear_groups
>   $ cat /sys/fs/ext4/sda/mb_max_linear_groups
>     0
> 
> So we use kstrtouint() to parse the attr_pointer_ui type to avoid the
> inconsistency described above. In addition, a judgment is added to avoid
> setting s_resv_clusters less than 0.
> 
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Thanks! The patch looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/sysfs.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
> index 6d332dff79dd..ca820620b974 100644
> --- a/fs/ext4/sysfs.c
> +++ b/fs/ext4/sysfs.c
> @@ -104,7 +104,7 @@ static ssize_t reserved_clusters_store(struct ext4_sb_info *sbi,
>  	int ret;
>  
>  	ret = kstrtoull(skip_spaces(buf), 0, &val);
> -	if (ret || val >= clusters)
> +	if (ret || val >= clusters || (s64)val < 0)
>  		return -EINVAL;
>  
>  	atomic64_set(&sbi->s_resv_clusters, val);
> @@ -451,7 +451,8 @@ static ssize_t ext4_attr_store(struct kobject *kobj,
>  						s_kobj);
>  	struct ext4_attr *a = container_of(attr, struct ext4_attr, attr);
>  	void *ptr = calc_ptr(a, sbi);
> -	unsigned long t;
> +	unsigned int t;
> +	unsigned long lt;
>  	int ret;
>  
>  	switch (a->attr_id) {
> @@ -460,7 +461,7 @@ static ssize_t ext4_attr_store(struct kobject *kobj,
>  	case attr_pointer_ui:
>  		if (!ptr)
>  			return 0;
> -		ret = kstrtoul(skip_spaces(buf), 0, &t);
> +		ret = kstrtouint(skip_spaces(buf), 0, &t);
>  		if (ret)
>  			return ret;
>  		if (a->attr_ptr == ptr_ext4_super_block_offset)
> @@ -471,10 +472,10 @@ static ssize_t ext4_attr_store(struct kobject *kobj,
>  	case attr_pointer_ul:
>  		if (!ptr)
>  			return 0;
> -		ret = kstrtoul(skip_spaces(buf), 0, &t);
> +		ret = kstrtoul(skip_spaces(buf), 0, &lt);
>  		if (ret)
>  			return ret;
> -		*((unsigned long *) ptr) = t;
> +		*((unsigned long *) ptr) = lt;
>  		return len;
>  	case attr_inode_readahead:
>  		return inode_readahead_blks_store(sbi, buf, len);
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v2 2/9] ext4: refactor out ext4_generic_attr_store()

[Baokun Li]

Refactor out the function ext4_generic_attr_store() to handle the setting
of values of various common types, with no functional changes.

Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/sysfs.c | 40 +++++++++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 15 deletions(-)

diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
index ca820620b974..2b1c529b7fdf 100644
--- a/fs/ext4/sysfs.c
+++ b/fs/ext4/sysfs.c
@@ -443,24 +443,20 @@ static ssize_t ext4_attr_show(struct kobject *kobj,
 	return 0;
 }
 
-static ssize_t ext4_attr_store(struct kobject *kobj,
-			       struct attribute *attr,
-			       const char *buf, size_t len)
+static ssize_t ext4_generic_attr_store(struct ext4_attr *a,
+				       struct ext4_sb_info *sbi,
+				       const char *buf, size_t len)
 {
-	struct ext4_sb_info *sbi = container_of(kobj, struct ext4_sb_info,
-						s_kobj);
-	struct ext4_attr *a = container_of(attr, struct ext4_attr, attr);
-	void *ptr = calc_ptr(a, sbi);
+	int ret;
 	unsigned int t;
 	unsigned long lt;
-	int ret;
+	void *ptr = calc_ptr(a, sbi);
+
+	if (!ptr)
+		return 0;
 
 	switch (a->attr_id) {
-	case attr_reserved_clusters:
-		return reserved_clusters_store(sbi, buf, len);
 	case attr_pointer_ui:
-		if (!ptr)
-			return 0;
 		ret = kstrtouint(skip_spaces(buf), 0, &t);
 		if (ret)
 			return ret;
@@ -470,19 +466,33 @@ static ssize_t ext4_attr_store(struct kobject *kobj,
 			*((unsigned int *) ptr) = t;
 		return len;
 	case attr_pointer_ul:
-		if (!ptr)
-			return 0;
 		ret = kstrtoul(skip_spaces(buf), 0, &lt);
 		if (ret)
 			return ret;
 		*((unsigned long *) ptr) = lt;
 		return len;
+	}
+	return 0;
+}
+
+static ssize_t ext4_attr_store(struct kobject *kobj,
+			       struct attribute *attr,
+			       const char *buf, size_t len)
+{
+	struct ext4_sb_info *sbi = container_of(kobj, struct ext4_sb_info,
+						s_kobj);
+	struct ext4_attr *a = container_of(attr, struct ext4_attr, attr);
+
+	switch (a->attr_id) {
+	case attr_reserved_clusters:
+		return reserved_clusters_store(sbi, buf, len);
 	case attr_inode_readahead:
 		return inode_readahead_blks_store(sbi, buf, len);
 	case attr_trigger_test_error:
 		return trigger_test_error(sbi, buf, len);
+	default:
+		return ext4_generic_attr_store(a, sbi, buf, len);
 	}
-	return 0;
 }
 
 static void ext4_sb_release(struct kobject *kobj)
-- 
2.31.1

Re: [PATCH v2 2/9] ext4: refactor out ext4_generic_attr_store()

[Jan Kara]

On Tue 27-02-24 17:11:41, Baokun Li wrote:
> Refactor out the function ext4_generic_attr_store() to handle the setting
> of values of various common types, with no functional changes.
> 
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Looks good! Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/sysfs.c | 40 +++++++++++++++++++++++++---------------
>  1 file changed, 25 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
> index ca820620b974..2b1c529b7fdf 100644
> --- a/fs/ext4/sysfs.c
> +++ b/fs/ext4/sysfs.c
> @@ -443,24 +443,20 @@ static ssize_t ext4_attr_show(struct kobject *kobj,
>  	return 0;
>  }
>  
> -static ssize_t ext4_attr_store(struct kobject *kobj,
> -			       struct attribute *attr,
> -			       const char *buf, size_t len)
> +static ssize_t ext4_generic_attr_store(struct ext4_attr *a,
> +				       struct ext4_sb_info *sbi,
> +				       const char *buf, size_t len)
>  {
> -	struct ext4_sb_info *sbi = container_of(kobj, struct ext4_sb_info,
> -						s_kobj);
> -	struct ext4_attr *a = container_of(attr, struct ext4_attr, attr);
> -	void *ptr = calc_ptr(a, sbi);
> +	int ret;
>  	unsigned int t;
>  	unsigned long lt;
> -	int ret;
> +	void *ptr = calc_ptr(a, sbi);
> +
> +	if (!ptr)
> +		return 0;
>  
>  	switch (a->attr_id) {
> -	case attr_reserved_clusters:
> -		return reserved_clusters_store(sbi, buf, len);
>  	case attr_pointer_ui:
> -		if (!ptr)
> -			return 0;
>  		ret = kstrtouint(skip_spaces(buf), 0, &t);
>  		if (ret)
>  			return ret;
> @@ -470,19 +466,33 @@ static ssize_t ext4_attr_store(struct kobject *kobj,
>  			*((unsigned int *) ptr) = t;
>  		return len;
>  	case attr_pointer_ul:
> -		if (!ptr)
> -			return 0;
>  		ret = kstrtoul(skip_spaces(buf), 0, &lt);
>  		if (ret)
>  			return ret;
>  		*((unsigned long *) ptr) = lt;
>  		return len;
> +	}
> +	return 0;
> +}
> +
> +static ssize_t ext4_attr_store(struct kobject *kobj,
> +			       struct attribute *attr,
> +			       const char *buf, size_t len)
> +{
> +	struct ext4_sb_info *sbi = container_of(kobj, struct ext4_sb_info,
> +						s_kobj);
> +	struct ext4_attr *a = container_of(attr, struct ext4_attr, attr);
> +
> +	switch (a->attr_id) {
> +	case attr_reserved_clusters:
> +		return reserved_clusters_store(sbi, buf, len);
>  	case attr_inode_readahead:
>  		return inode_readahead_blks_store(sbi, buf, len);
>  	case attr_trigger_test_error:
>  		return trigger_test_error(sbi, buf, len);
> +	default:
> +		return ext4_generic_attr_store(a, sbi, buf, len);
>  	}
> -	return 0;
>  }
>  
>  static void ext4_sb_release(struct kobject *kobj)
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v2 3/9] ext4: refactor out ext4_generic_attr_show()

[Baokun Li]

Refactor out the function ext4_generic_attr_show() to handle the reading
of values of various common types, with no functional changes.

Signed-off-by: Baokun Li <libaokun1@huawei.com>
Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/sysfs.c | 74 +++++++++++++++++++++----------------------------
 1 file changed, 32 insertions(+), 42 deletions(-)

diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
index 2b1c529b7fdf..7f455b5f22c0 100644
--- a/fs/ext4/sysfs.c
+++ b/fs/ext4/sysfs.c
@@ -366,13 +366,42 @@ static ssize_t __print_tstamp(char *buf, __le32 lo, __u8 hi)
 #define print_tstamp(buf, es, tstamp) \
 	__print_tstamp(buf, (es)->tstamp, (es)->tstamp ## _hi)
 
+static ssize_t ext4_generic_attr_show(struct ext4_attr *a,
+				      struct ext4_sb_info *sbi, char *buf)
+{
+	void *ptr = calc_ptr(a, sbi);
+
+	if (!ptr)
+		return 0;
+
+	switch (a->attr_id) {
+	case attr_inode_readahead:
+	case attr_pointer_ui:
+		if (a->attr_ptr == ptr_ext4_super_block_offset)
+			return sysfs_emit(buf, "%u\n", le32_to_cpup(ptr));
+		return sysfs_emit(buf, "%u\n", *((unsigned int *) ptr));
+	case attr_pointer_ul:
+		return sysfs_emit(buf, "%lu\n", *((unsigned long *) ptr));
+	case attr_pointer_u8:
+		return sysfs_emit(buf, "%u\n", *((unsigned char *) ptr));
+	case attr_pointer_u64:
+		if (a->attr_ptr == ptr_ext4_super_block_offset)
+			return sysfs_emit(buf, "%llu\n", le64_to_cpup(ptr));
+		return sysfs_emit(buf, "%llu\n", *((unsigned long long *) ptr));
+	case attr_pointer_string:
+		return sysfs_emit(buf, "%.*s\n", a->attr_size, (char *) ptr);
+	case attr_pointer_atomic:
+		return sysfs_emit(buf, "%d\n", atomic_read((atomic_t *) ptr));
+	}
+	return 0;
+}
+
 static ssize_t ext4_attr_show(struct kobject *kobj,
 			      struct attribute *attr, char *buf)
 {
 	struct ext4_sb_info *sbi = container_of(kobj, struct ext4_sb_info,
 						s_kobj);
 	struct ext4_attr *a = container_of(attr, struct ext4_attr, attr);
-	void *ptr = calc_ptr(a, sbi);
 
 	switch (a->attr_id) {
 	case attr_delayed_allocation_blocks:
@@ -391,45 +420,6 @@ static ssize_t ext4_attr_show(struct kobject *kobj,
 		return sysfs_emit(buf, "%llu\n",
 				(unsigned long long)
 			percpu_counter_sum(&sbi->s_sra_exceeded_retry_limit));
-	case attr_inode_readahead:
-	case attr_pointer_ui:
-		if (!ptr)
-			return 0;
-		if (a->attr_ptr == ptr_ext4_super_block_offset)
-			return sysfs_emit(buf, "%u\n",
-					le32_to_cpup(ptr));
-		else
-			return sysfs_emit(buf, "%u\n",
-					*((unsigned int *) ptr));
-	case attr_pointer_ul:
-		if (!ptr)
-			return 0;
-		return sysfs_emit(buf, "%lu\n",
-				*((unsigned long *) ptr));
-	case attr_pointer_u8:
-		if (!ptr)
-			return 0;
-		return sysfs_emit(buf, "%u\n",
-				*((unsigned char *) ptr));
-	case attr_pointer_u64:
-		if (!ptr)
-			return 0;
-		if (a->attr_ptr == ptr_ext4_super_block_offset)
-			return sysfs_emit(buf, "%llu\n",
-					le64_to_cpup(ptr));
-		else
-			return sysfs_emit(buf, "%llu\n",
-					*((unsigned long long *) ptr));
-	case attr_pointer_string:
-		if (!ptr)
-			return 0;
-		return sysfs_emit(buf, "%.*s\n", a->attr_size,
-				(char *) ptr);
-	case attr_pointer_atomic:
-		if (!ptr)
-			return 0;
-		return sysfs_emit(buf, "%d\n",
-				atomic_read((atomic_t *) ptr));
 	case attr_feature:
 		return sysfs_emit(buf, "supported\n");
 	case attr_first_error_time:
@@ -438,9 +428,9 @@ static ssize_t ext4_attr_show(struct kobject *kobj,
 		return print_tstamp(buf, sbi->s_es, s_last_error_time);
 	case attr_journal_task:
 		return journal_task_show(sbi, buf);
+	default:
+		return ext4_generic_attr_show(a, sbi, buf);
 	}
-
-	return 0;
 }
 
 static ssize_t ext4_generic_attr_store(struct ext4_attr *a,
-- 
2.31.1

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

[Baokun Li]

We can trigger a slab-out-of-bounds with the following commands:

    mkfs.ext4 -F /dev/$disk 10G
    mount /dev/$disk /tmp/test
    echo 2147483647 > /sys/fs/ext4/$disk/mb_group_prealloc
    echo test > /tmp/test/file && sync

==================================================================
BUG: KASAN: slab-out-of-bounds in ext4_mb_find_good_group_avg_frag_lists+0x8a/0x200 [ext4]
Read of size 8 at addr ffff888121b9d0f0 by task kworker/u2:0/11
CPU: 0 PID: 11 Comm: kworker/u2:0 Tainted: GL 6.7.0-next-20240118 #521
Call Trace:
 dump_stack_lvl+0x2c/0x50
 kasan_report+0xb6/0xf0
 ext4_mb_find_good_group_avg_frag_lists+0x8a/0x200 [ext4]
 ext4_mb_regular_allocator+0x19e9/0x2370 [ext4]
 ext4_mb_new_blocks+0x88a/0x1370 [ext4]
 ext4_ext_map_blocks+0x14f7/0x2390 [ext4]
 ext4_map_blocks+0x569/0xea0 [ext4]
 ext4_do_writepages+0x10f6/0x1bc0 [ext4]
[...]
==================================================================

The flow of issue triggering is as follows:

// Set s_mb_group_prealloc to 2147483647 via sysfs
ext4_mb_new_blocks
  ext4_mb_normalize_request
    ext4_mb_normalize_group_request
      ac->ac_g_ex.fe_len = EXT4_SB(sb)->s_mb_group_prealloc
  ext4_mb_regular_allocator
    ext4_mb_choose_next_group
      ext4_mb_choose_next_group_best_avail
        mb_avg_fragment_size_order
          order = fls(len) - 2 = 29
        ext4_mb_find_good_group_avg_frag_lists
          frag_list = &sbi->s_mb_avg_fragment_size[order]
          if (list_empty(frag_list)) // Trigger SOOB!

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@vger.kernel.org
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 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);
 	}
 
 	/* Reset goal length to original goal length before falling into CR_GOAL_LEN_SLOW */
diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
index 7f455b5f22c0..ddd71673176c 100644
--- a/fs/ext4/sysfs.c
+++ b/fs/ext4/sysfs.c
@@ -29,6 +29,7 @@ typedef enum {
 	attr_trigger_test_error,
 	attr_first_error_time,
 	attr_last_error_time,
+	attr_clusters_in_group,
 	attr_feature,
 	attr_pointer_ui,
 	attr_pointer_ul,
@@ -207,13 +208,14 @@ EXT4_ATTR_FUNC(sra_exceeded_retry_limit, 0444);
 
 EXT4_ATTR_OFFSET(inode_readahead_blks, 0644, inode_readahead,
 		 ext4_sb_info, s_inode_readahead_blks);
+EXT4_ATTR_OFFSET(mb_group_prealloc, 0644, clusters_in_group,
+		 ext4_sb_info, s_mb_group_prealloc);
 EXT4_RW_ATTR_SBI_UI(inode_goal, s_inode_goal);
 EXT4_RW_ATTR_SBI_UI(mb_stats, s_mb_stats);
 EXT4_RW_ATTR_SBI_UI(mb_max_to_scan, s_mb_max_to_scan);
 EXT4_RW_ATTR_SBI_UI(mb_min_to_scan, s_mb_min_to_scan);
 EXT4_RW_ATTR_SBI_UI(mb_order2_req, s_mb_order2_reqs);
 EXT4_RW_ATTR_SBI_UI(mb_stream_req, s_mb_stream_request);
-EXT4_RW_ATTR_SBI_UI(mb_group_prealloc, s_mb_group_prealloc);
 EXT4_RW_ATTR_SBI_UI(mb_max_linear_groups, s_mb_max_linear_groups);
 EXT4_RW_ATTR_SBI_UI(extent_max_zeroout_kb, s_extent_max_zeroout_kb);
 EXT4_ATTR(trigger_fs_error, 0200, trigger_test_error);
@@ -376,6 +378,7 @@ static ssize_t ext4_generic_attr_show(struct ext4_attr *a,
 
 	switch (a->attr_id) {
 	case attr_inode_readahead:
+	case attr_clusters_in_group:
 	case attr_pointer_ui:
 		if (a->attr_ptr == ptr_ext4_super_block_offset)
 			return sysfs_emit(buf, "%u\n", le32_to_cpup(ptr));
@@ -455,6 +458,14 @@ static ssize_t ext4_generic_attr_store(struct ext4_attr *a,
 		else
 			*((unsigned int *) ptr) = t;
 		return len;
+	case attr_clusters_in_group:
+		ret = kstrtouint(skip_spaces(buf), 0, &t);
+		if (ret)
+			return ret;
+		if (t > sbi->s_clusters_per_group)
+			return -EINVAL;
+		*((unsigned int *) ptr) = t;
+		return len;
 	case attr_pointer_ul:
 		ret = kstrtoul(skip_spaces(buf), 0, &lt);
 		if (ret)
-- 
2.31.1

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

[Jan Kara]

On Tue 27-02-24 17:11:43, Baokun Li wrote:
> We can trigger a slab-out-of-bounds with the following commands:
> 
>     mkfs.ext4 -F /dev/$disk 10G
>     mount /dev/$disk /tmp/test
>     echo 2147483647 > /sys/fs/ext4/$disk/mb_group_prealloc
>     echo test > /tmp/test/file && sync
> 
> ==================================================================
> BUG: KASAN: slab-out-of-bounds in ext4_mb_find_good_group_avg_frag_lists+0x8a/0x200 [ext4]
> Read of size 8 at addr ffff888121b9d0f0 by task kworker/u2:0/11
> CPU: 0 PID: 11 Comm: kworker/u2:0 Tainted: GL 6.7.0-next-20240118 #521
> Call Trace:
>  dump_stack_lvl+0x2c/0x50
>  kasan_report+0xb6/0xf0
>  ext4_mb_find_good_group_avg_frag_lists+0x8a/0x200 [ext4]
>  ext4_mb_regular_allocator+0x19e9/0x2370 [ext4]
>  ext4_mb_new_blocks+0x88a/0x1370 [ext4]
>  ext4_ext_map_blocks+0x14f7/0x2390 [ext4]
>  ext4_map_blocks+0x569/0xea0 [ext4]
>  ext4_do_writepages+0x10f6/0x1bc0 [ext4]
> [...]
> ==================================================================
> 
> The flow of issue triggering is as follows:
> 
> // Set s_mb_group_prealloc to 2147483647 via sysfs
> ext4_mb_new_blocks
>   ext4_mb_normalize_request
>     ext4_mb_normalize_group_request
>       ac->ac_g_ex.fe_len = EXT4_SB(sb)->s_mb_group_prealloc
>   ext4_mb_regular_allocator
>     ext4_mb_choose_next_group
>       ext4_mb_choose_next_group_best_avail
>         mb_avg_fragment_size_order
>           order = fls(len) - 2 = 29
>         ext4_mb_find_good_group_avg_frag_lists
>           frag_list = &sbi->s_mb_avg_fragment_size[order]
>           if (list_empty(frag_list)) // Trigger SOOB!
> 
> 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@vger.kernel.org
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Looks good. Just one nit below. Otherwise feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

> ---
>  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
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Baokun Li]

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@vger.kernel.org
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> Looks good. Just one nit below. Otherwise feel free to add:
>
> Reviewed-by: Jan Kara <jack@suse.cz>
>
>> ---
>>   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@li-bb2b2a4c-3307-11b2-a85c-8fa5c3a69313.ibm.com/

Thank you so much for the review! ღ( ´･ᴗ･` )
-- 
With Best Regards,
Baokun Li
.

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

[Jan Kara]

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@vger.kernel.org
> > Signed-off-by: Baokun Li <libaokun1@huawei.com>
> > Looks good. Just one nit below. Otherwise feel free to add:
> > 
> > Reviewed-by: Jan Kara <jack@suse.cz>
> > 
> > > ---
> > >   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@li-bb2b2a4c-3307-11b2-a85c-8fa5c3a69313.ibm.com/
> 
> 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
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Baokun Li]

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@vger.kernel.org
>>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
>>> Looks good. Just one nit below. Otherwise feel free to add:
>>>
>>> Reviewed-by: Jan Kara <jack@suse.cz>
>>>
>>>> ---
>>>>    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@li-bb2b2a4c-3307-11b2-a85c-8fa5c3a69313.ibm.com/
>>
>> 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.

Thanks!
-- 
With Best Regards,
Baokun Li
.

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

[Jan Kara]

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@vger.kernel.org
> > > > Signed-off-by: Baokun Li <libaokun1@huawei.com>
> > > > Looks good. Just one nit below. Otherwise feel free to add:
> > > > 
> > > > Reviewed-by: Jan Kara <jack@suse.cz>
> > > > 
> > > > > ---
> > > > >    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@li-bb2b2a4c-3307-11b2-a85c-8fa5c3a69313.ibm.com/
> > > 
> > > 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@suse.com>
SUSE Labs, CR

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

[Baokun Li]

On 2024/3/14 20:50, Jan Kara wrote:
> 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@vger.kernel.org
>>>>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
>>>>> Looks good. Just one nit below. Otherwise feel free to add:
>>>>>
>>>>> Reviewed-by: Jan Kara <jack@suse.cz>
>>>>>
>>>>>> ---
>>>>>>     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@li-bb2b2a4c-3307-11b2-a85c-8fa5c3a69313.ibm.com/
>>>>
>>>> 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
Yes this does look much better!
Let me send v3!

Thanks for the suggestion!
-- 
With Best Regards,
Baokun Li
.

[PATCH v2 5/9] ext4: add new attr pointer attr_mb_order

[Baokun Li]

The s_mb_best_avail_max_trim_order is of type unsigned int, and has a
range of values well beyond the normal use of the mb_order. Although the
mballoc code is careful enough that large numbers don't matter there, but
this can mislead the sysadmin into thinking that it's normal to set such
values. Hence add a new attr_id attr_mb_order with values in the range
[0, 64] to avoid storing garbage values and make us more resilient to
surprises in the future.

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/sysfs.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
index ddd71673176c..8e0473169458 100644
--- a/fs/ext4/sysfs.c
+++ b/fs/ext4/sysfs.c
@@ -30,6 +30,7 @@ typedef enum {
 	attr_first_error_time,
 	attr_last_error_time,
 	attr_clusters_in_group,
+	attr_mb_order,
 	attr_feature,
 	attr_pointer_ui,
 	attr_pointer_ul,
@@ -210,6 +211,8 @@ EXT4_ATTR_OFFSET(inode_readahead_blks, 0644, inode_readahead,
 		 ext4_sb_info, s_inode_readahead_blks);
 EXT4_ATTR_OFFSET(mb_group_prealloc, 0644, clusters_in_group,
 		 ext4_sb_info, s_mb_group_prealloc);
+EXT4_ATTR_OFFSET(mb_best_avail_max_trim_order, 0644, mb_order,
+		 ext4_sb_info, s_mb_best_avail_max_trim_order);
 EXT4_RW_ATTR_SBI_UI(inode_goal, s_inode_goal);
 EXT4_RW_ATTR_SBI_UI(mb_stats, s_mb_stats);
 EXT4_RW_ATTR_SBI_UI(mb_max_to_scan, s_mb_max_to_scan);
@@ -225,7 +228,6 @@ EXT4_RW_ATTR_SBI_UI(warning_ratelimit_interval_ms, s_warning_ratelimit_state.int
 EXT4_RW_ATTR_SBI_UI(warning_ratelimit_burst, s_warning_ratelimit_state.burst);
 EXT4_RW_ATTR_SBI_UI(msg_ratelimit_interval_ms, s_msg_ratelimit_state.interval);
 EXT4_RW_ATTR_SBI_UI(msg_ratelimit_burst, s_msg_ratelimit_state.burst);
-EXT4_RW_ATTR_SBI_UI(mb_best_avail_max_trim_order, s_mb_best_avail_max_trim_order);
 #ifdef CONFIG_EXT4_DEBUG
 EXT4_RW_ATTR_SBI_UL(simulate_fail, s_simulate_fail);
 #endif
@@ -379,6 +381,7 @@ static ssize_t ext4_generic_attr_show(struct ext4_attr *a,
 	switch (a->attr_id) {
 	case attr_inode_readahead:
 	case attr_clusters_in_group:
+	case attr_mb_order:
 	case attr_pointer_ui:
 		if (a->attr_ptr == ptr_ext4_super_block_offset)
 			return sysfs_emit(buf, "%u\n", le32_to_cpup(ptr));
@@ -458,6 +461,14 @@ static ssize_t ext4_generic_attr_store(struct ext4_attr *a,
 		else
 			*((unsigned int *) ptr) = t;
 		return len;
+	case attr_mb_order:
+		ret = kstrtouint(skip_spaces(buf), 0, &t);
+		if (ret)
+			return ret;
+		if (t > 64)
+			return -EINVAL;
+		*((unsigned int *) ptr) = t;
+		return len;
 	case attr_clusters_in_group:
 		ret = kstrtouint(skip_spaces(buf), 0, &t);
 		if (ret)
-- 
2.31.1

Re: [PATCH v2 5/9] ext4: add new attr pointer attr_mb_order

[Jan Kara]

On Tue 27-02-24 17:11:44, Baokun Li wrote:
> The s_mb_best_avail_max_trim_order is of type unsigned int, and has a
> range of values well beyond the normal use of the mb_order. Although the
> mballoc code is careful enough that large numbers don't matter there, but
> this can mislead the sysadmin into thinking that it's normal to set such
> values. Hence add a new attr_id attr_mb_order with values in the range
> [0, 64] to avoid storing garbage values and make us more resilient to
> surprises in the future.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/sysfs.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
> index ddd71673176c..8e0473169458 100644
> --- a/fs/ext4/sysfs.c
> +++ b/fs/ext4/sysfs.c
> @@ -30,6 +30,7 @@ typedef enum {
>  	attr_first_error_time,
>  	attr_last_error_time,
>  	attr_clusters_in_group,
> +	attr_mb_order,
>  	attr_feature,
>  	attr_pointer_ui,
>  	attr_pointer_ul,
> @@ -210,6 +211,8 @@ EXT4_ATTR_OFFSET(inode_readahead_blks, 0644, inode_readahead,
>  		 ext4_sb_info, s_inode_readahead_blks);
>  EXT4_ATTR_OFFSET(mb_group_prealloc, 0644, clusters_in_group,
>  		 ext4_sb_info, s_mb_group_prealloc);
> +EXT4_ATTR_OFFSET(mb_best_avail_max_trim_order, 0644, mb_order,
> +		 ext4_sb_info, s_mb_best_avail_max_trim_order);
>  EXT4_RW_ATTR_SBI_UI(inode_goal, s_inode_goal);
>  EXT4_RW_ATTR_SBI_UI(mb_stats, s_mb_stats);
>  EXT4_RW_ATTR_SBI_UI(mb_max_to_scan, s_mb_max_to_scan);
> @@ -225,7 +228,6 @@ EXT4_RW_ATTR_SBI_UI(warning_ratelimit_interval_ms, s_warning_ratelimit_state.int
>  EXT4_RW_ATTR_SBI_UI(warning_ratelimit_burst, s_warning_ratelimit_state.burst);
>  EXT4_RW_ATTR_SBI_UI(msg_ratelimit_interval_ms, s_msg_ratelimit_state.interval);
>  EXT4_RW_ATTR_SBI_UI(msg_ratelimit_burst, s_msg_ratelimit_state.burst);
> -EXT4_RW_ATTR_SBI_UI(mb_best_avail_max_trim_order, s_mb_best_avail_max_trim_order);
>  #ifdef CONFIG_EXT4_DEBUG
>  EXT4_RW_ATTR_SBI_UL(simulate_fail, s_simulate_fail);
>  #endif
> @@ -379,6 +381,7 @@ static ssize_t ext4_generic_attr_show(struct ext4_attr *a,
>  	switch (a->attr_id) {
>  	case attr_inode_readahead:
>  	case attr_clusters_in_group:
> +	case attr_mb_order:
>  	case attr_pointer_ui:
>  		if (a->attr_ptr == ptr_ext4_super_block_offset)
>  			return sysfs_emit(buf, "%u\n", le32_to_cpup(ptr));
> @@ -458,6 +461,14 @@ static ssize_t ext4_generic_attr_store(struct ext4_attr *a,
>  		else
>  			*((unsigned int *) ptr) = t;
>  		return len;
> +	case attr_mb_order:
> +		ret = kstrtouint(skip_spaces(buf), 0, &t);
> +		if (ret)
> +			return ret;
> +		if (t > 64)
> +			return -EINVAL;
> +		*((unsigned int *) ptr) = t;
> +		return len;
>  	case attr_clusters_in_group:
>  		ret = kstrtouint(skip_spaces(buf), 0, &t);
>  		if (ret)
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v2 6/9] ext4: add positive int attr pointer to avoid sysfs variables overflow

[Baokun Li]

The following variables controlled by the sysfs interface are of type
int and are normally used in the range [0, INT_MAX], but are declared as
attr_pointer_ui, and thus may be set to values that exceed INT_MAX and
result in overflows to get negative values.

  err_ratelimit_burst
  msg_ratelimit_burst
  warning_ratelimit_burst
  err_ratelimit_interval_ms
  msg_ratelimit_interval_ms
  warning_ratelimit_interval_ms

Therefore, we add attr_pointer_pi (aka positive int attr pointer) with a
value range of 0-INT_MAX to avoid overflow.

Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/sysfs.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
index 8e0473169458..ddb54608ca2e 100644
--- a/fs/ext4/sysfs.c
+++ b/fs/ext4/sysfs.c
@@ -32,6 +32,7 @@ typedef enum {
 	attr_clusters_in_group,
 	attr_mb_order,
 	attr_feature,
+	attr_pointer_pi,
 	attr_pointer_ui,
 	attr_pointer_ul,
 	attr_pointer_u64,
@@ -180,6 +181,9 @@ static struct ext4_attr ext4_attr_##_name = {			\
 #define EXT4_RO_ATTR_ES_STRING(_name,_elname,_size)			\
 	EXT4_ATTR_STRING(_name, 0444, _size, ext4_super_block, _elname)
 
+#define EXT4_RW_ATTR_SBI_PI(_name,_elname)      \
+	EXT4_ATTR_OFFSET(_name, 0644, pointer_pi, ext4_sb_info, _elname)
+
 #define EXT4_RW_ATTR_SBI_UI(_name,_elname)	\
 	EXT4_ATTR_OFFSET(_name, 0644, pointer_ui, ext4_sb_info, _elname)
 
@@ -222,12 +226,12 @@ EXT4_RW_ATTR_SBI_UI(mb_stream_req, s_mb_stream_request);
 EXT4_RW_ATTR_SBI_UI(mb_max_linear_groups, s_mb_max_linear_groups);
 EXT4_RW_ATTR_SBI_UI(extent_max_zeroout_kb, s_extent_max_zeroout_kb);
 EXT4_ATTR(trigger_fs_error, 0200, trigger_test_error);
-EXT4_RW_ATTR_SBI_UI(err_ratelimit_interval_ms, s_err_ratelimit_state.interval);
-EXT4_RW_ATTR_SBI_UI(err_ratelimit_burst, s_err_ratelimit_state.burst);
-EXT4_RW_ATTR_SBI_UI(warning_ratelimit_interval_ms, s_warning_ratelimit_state.interval);
-EXT4_RW_ATTR_SBI_UI(warning_ratelimit_burst, s_warning_ratelimit_state.burst);
-EXT4_RW_ATTR_SBI_UI(msg_ratelimit_interval_ms, s_msg_ratelimit_state.interval);
-EXT4_RW_ATTR_SBI_UI(msg_ratelimit_burst, s_msg_ratelimit_state.burst);
+EXT4_RW_ATTR_SBI_PI(err_ratelimit_interval_ms, s_err_ratelimit_state.interval);
+EXT4_RW_ATTR_SBI_PI(err_ratelimit_burst, s_err_ratelimit_state.burst);
+EXT4_RW_ATTR_SBI_PI(warning_ratelimit_interval_ms, s_warning_ratelimit_state.interval);
+EXT4_RW_ATTR_SBI_PI(warning_ratelimit_burst, s_warning_ratelimit_state.burst);
+EXT4_RW_ATTR_SBI_PI(msg_ratelimit_interval_ms, s_msg_ratelimit_state.interval);
+EXT4_RW_ATTR_SBI_PI(msg_ratelimit_burst, s_msg_ratelimit_state.burst);
 #ifdef CONFIG_EXT4_DEBUG
 EXT4_RW_ATTR_SBI_UL(simulate_fail, s_simulate_fail);
 #endif
@@ -382,6 +386,7 @@ static ssize_t ext4_generic_attr_show(struct ext4_attr *a,
 	case attr_inode_readahead:
 	case attr_clusters_in_group:
 	case attr_mb_order:
+	case attr_pointer_pi:
 	case attr_pointer_ui:
 		if (a->attr_ptr == ptr_ext4_super_block_offset)
 			return sysfs_emit(buf, "%u\n", le32_to_cpup(ptr));
@@ -452,6 +457,14 @@ static ssize_t ext4_generic_attr_store(struct ext4_attr *a,
 		return 0;
 
 	switch (a->attr_id) {
+	case attr_pointer_pi:
+		ret = kstrtouint(skip_spaces(buf), 0, &t);
+		if (ret)
+			return ret;
+		if ((int)t < 0)
+			return -EINVAL;
+		*((unsigned int *) ptr) = t;
+		return len;
 	case attr_pointer_ui:
 		ret = kstrtouint(skip_spaces(buf), 0, &t);
 		if (ret)
-- 
2.31.1

Re: [PATCH v2 6/9] ext4: add positive int attr pointer to avoid sysfs variables overflow

[Jan Kara]

On Tue 27-02-24 17:11:45, Baokun Li wrote:
> The following variables controlled by the sysfs interface are of type
> int and are normally used in the range [0, INT_MAX], but are declared as
> attr_pointer_ui, and thus may be set to values that exceed INT_MAX and
> result in overflows to get negative values.
> 
>   err_ratelimit_burst
>   msg_ratelimit_burst
>   warning_ratelimit_burst
>   err_ratelimit_interval_ms
>   msg_ratelimit_interval_ms
>   warning_ratelimit_interval_ms
> 
> Therefore, we add attr_pointer_pi (aka positive int attr pointer) with a
> value range of 0-INT_MAX to avoid overflow.
> 
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/sysfs.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
> index 8e0473169458..ddb54608ca2e 100644
> --- a/fs/ext4/sysfs.c
> +++ b/fs/ext4/sysfs.c
> @@ -32,6 +32,7 @@ typedef enum {
>  	attr_clusters_in_group,
>  	attr_mb_order,
>  	attr_feature,
> +	attr_pointer_pi,
>  	attr_pointer_ui,
>  	attr_pointer_ul,
>  	attr_pointer_u64,
> @@ -180,6 +181,9 @@ static struct ext4_attr ext4_attr_##_name = {			\
>  #define EXT4_RO_ATTR_ES_STRING(_name,_elname,_size)			\
>  	EXT4_ATTR_STRING(_name, 0444, _size, ext4_super_block, _elname)
>  
> +#define EXT4_RW_ATTR_SBI_PI(_name,_elname)      \
> +	EXT4_ATTR_OFFSET(_name, 0644, pointer_pi, ext4_sb_info, _elname)
> +
>  #define EXT4_RW_ATTR_SBI_UI(_name,_elname)	\
>  	EXT4_ATTR_OFFSET(_name, 0644, pointer_ui, ext4_sb_info, _elname)
>  
> @@ -222,12 +226,12 @@ EXT4_RW_ATTR_SBI_UI(mb_stream_req, s_mb_stream_request);
>  EXT4_RW_ATTR_SBI_UI(mb_max_linear_groups, s_mb_max_linear_groups);
>  EXT4_RW_ATTR_SBI_UI(extent_max_zeroout_kb, s_extent_max_zeroout_kb);
>  EXT4_ATTR(trigger_fs_error, 0200, trigger_test_error);
> -EXT4_RW_ATTR_SBI_UI(err_ratelimit_interval_ms, s_err_ratelimit_state.interval);
> -EXT4_RW_ATTR_SBI_UI(err_ratelimit_burst, s_err_ratelimit_state.burst);
> -EXT4_RW_ATTR_SBI_UI(warning_ratelimit_interval_ms, s_warning_ratelimit_state.interval);
> -EXT4_RW_ATTR_SBI_UI(warning_ratelimit_burst, s_warning_ratelimit_state.burst);
> -EXT4_RW_ATTR_SBI_UI(msg_ratelimit_interval_ms, s_msg_ratelimit_state.interval);
> -EXT4_RW_ATTR_SBI_UI(msg_ratelimit_burst, s_msg_ratelimit_state.burst);
> +EXT4_RW_ATTR_SBI_PI(err_ratelimit_interval_ms, s_err_ratelimit_state.interval);
> +EXT4_RW_ATTR_SBI_PI(err_ratelimit_burst, s_err_ratelimit_state.burst);
> +EXT4_RW_ATTR_SBI_PI(warning_ratelimit_interval_ms, s_warning_ratelimit_state.interval);
> +EXT4_RW_ATTR_SBI_PI(warning_ratelimit_burst, s_warning_ratelimit_state.burst);
> +EXT4_RW_ATTR_SBI_PI(msg_ratelimit_interval_ms, s_msg_ratelimit_state.interval);
> +EXT4_RW_ATTR_SBI_PI(msg_ratelimit_burst, s_msg_ratelimit_state.burst);
>  #ifdef CONFIG_EXT4_DEBUG
>  EXT4_RW_ATTR_SBI_UL(simulate_fail, s_simulate_fail);
>  #endif
> @@ -382,6 +386,7 @@ static ssize_t ext4_generic_attr_show(struct ext4_attr *a,
>  	case attr_inode_readahead:
>  	case attr_clusters_in_group:
>  	case attr_mb_order:
> +	case attr_pointer_pi:
>  	case attr_pointer_ui:
>  		if (a->attr_ptr == ptr_ext4_super_block_offset)
>  			return sysfs_emit(buf, "%u\n", le32_to_cpup(ptr));
> @@ -452,6 +457,14 @@ static ssize_t ext4_generic_attr_store(struct ext4_attr *a,
>  		return 0;
>  
>  	switch (a->attr_id) {
> +	case attr_pointer_pi:
> +		ret = kstrtouint(skip_spaces(buf), 0, &t);
> +		if (ret)
> +			return ret;
> +		if ((int)t < 0)
> +			return -EINVAL;
> +		*((unsigned int *) ptr) = t;
> +		return len;
>  	case attr_pointer_ui:
>  		ret = kstrtouint(skip_spaces(buf), 0, &t);
>  		if (ret)
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v2 7/9] ext4: set type of ac_groups_linear_remaining to __u32 to avoid overflow

[Baokun Li]

Now ac_groups_linear_remaining is of type __u16 and s_mb_max_linear_groups
is of type unsigned int, so an overflow occurs when setting a value above
65535 through the mb_max_linear_groups sysfs interface. Therefore, the
type of ac_groups_linear_remaining is set to __u32 to avoid overflow.

Fixes: 196e402adf2e ("ext4: improve cr 0 / cr 1 group scanning")
CC: stable@kernel.org
Signed-off-by: Baokun Li <libaokun1@huawei.com>
Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/mballoc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/mballoc.h b/fs/ext4/mballoc.h
index 56938532b4ce..7bfc5fb5a128 100644
--- a/fs/ext4/mballoc.h
+++ b/fs/ext4/mballoc.h
@@ -193,8 +193,8 @@ struct ext4_allocation_context {
 	ext4_grpblk_t	ac_orig_goal_len;
 
 	__u32 ac_flags;		/* allocation hints */
+	__u32 ac_groups_linear_remaining;
 	__u16 ac_groups_scanned;
-	__u16 ac_groups_linear_remaining;
 	__u16 ac_found;
 	__u16 ac_cX_found[EXT4_MB_NUM_CRS];
 	__u16 ac_tail;
-- 
2.31.1

[PATCH v2 8/9] ext4: set the type of max_zeroout to unsigned int to avoid overflow

[Baokun Li]

The max_zeroout is of type int and the s_extent_max_zeroout_kb is of
type uint, and the s_extent_max_zeroout_kb can be freely modified via
the sysfs interface. When the block size is 1024, max_zeroout may
overflow, so declare it as unsigned int to avoid overflow.

Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/extents.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index e57054bdc5fd..e067f2dd0335 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3402,9 +3402,10 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
 	struct ext4_extent *ex, *abut_ex;
 	ext4_lblk_t ee_block, eof_block;
 	unsigned int ee_len, depth, map_len = map->m_len;
-	int allocated = 0, max_zeroout = 0;
 	int err = 0;
 	int split_flag = EXT4_EXT_DATA_VALID2;
+	int allocated = 0;
+	unsigned int max_zeroout = 0;
 
 	ext_debug(inode, "logical block %llu, max_blocks %u\n",
 		  (unsigned long long)map->m_lblk, map_len);
-- 
2.31.1

Re: [PATCH v2 8/9] ext4: set the type of max_zeroout to unsigned int to avoid overflow

[Jan Kara]

On Tue 27-02-24 17:11:47, Baokun Li wrote:
> The max_zeroout is of type int and the s_extent_max_zeroout_kb is of
> type uint, and the s_extent_max_zeroout_kb can be freely modified via
> the sysfs interface. When the block size is 1024, max_zeroout may
> overflow, so declare it as unsigned int to avoid overflow.
> 
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Indeed. The patch looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/extents.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index e57054bdc5fd..e067f2dd0335 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -3402,9 +3402,10 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
>  	struct ext4_extent *ex, *abut_ex;
>  	ext4_lblk_t ee_block, eof_block;
>  	unsigned int ee_len, depth, map_len = map->m_len;
> -	int allocated = 0, max_zeroout = 0;
>  	int err = 0;
>  	int split_flag = EXT4_EXT_DATA_VALID2;
> +	int allocated = 0;
> +	unsigned int max_zeroout = 0;
>  
>  	ext_debug(inode, "logical block %llu, max_blocks %u\n",
>  		  (unsigned long long)map->m_lblk, map_len);
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v2 9/9] ext4: clean up s_mb_rb_lock to fix build warnings with C=1

[Baokun Li]

Running sparse (make C=1) on mballoc.c we get the following warning:

 fs/ext4/mballoc.c:3194:13: warning: context imbalance in
  'ext4_mb_seq_structs_summary_start' - wrong count at exit

This is because __acquires(&EXT4_SB(sb)->s_mb_rb_lock) was called in
ext4_mb_seq_structs_summary_start(), but s_mb_rb_lock was removed in commit
83e80a6e3543 ("ext4: use buckets for cr 1 block scan instead of rbtree"),
so remove the __acquires to silence the warning.

Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/mballoc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 7ad089df2408..7ae8e04d6ef4 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -3192,7 +3192,6 @@ int ext4_seq_mb_stats_show(struct seq_file *seq, void *offset)
 }
 
 static void *ext4_mb_seq_structs_summary_start(struct seq_file *seq, loff_t *pos)
-__acquires(&EXT4_SB(sb)->s_mb_rb_lock)
 {
 	struct super_block *sb = pde_data(file_inode(seq->file));
 	unsigned long position;
-- 
2.31.1

Re: [PATCH v2 9/9] ext4: clean up s_mb_rb_lock to fix build warnings with C=1

[Jan Kara]

On Tue 27-02-24 17:11:48, Baokun Li wrote:
> Running sparse (make C=1) on mballoc.c we get the following warning:
> 
>  fs/ext4/mballoc.c:3194:13: warning: context imbalance in
>   'ext4_mb_seq_structs_summary_start' - wrong count at exit
> 
> This is because __acquires(&EXT4_SB(sb)->s_mb_rb_lock) was called in
> ext4_mb_seq_structs_summary_start(), but s_mb_rb_lock was removed in commit
> 83e80a6e3543 ("ext4: use buckets for cr 1 block scan instead of rbtree"),
> so remove the __acquires to silence the warning.
> 
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/mballoc.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 7ad089df2408..7ae8e04d6ef4 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -3192,7 +3192,6 @@ int ext4_seq_mb_stats_show(struct seq_file *seq, void *offset)
>  }
>  
>  static void *ext4_mb_seq_structs_summary_start(struct seq_file *seq, loff_t *pos)
> -__acquires(&EXT4_SB(sb)->s_mb_rb_lock)
>  {
>  	struct super_block *sb = pde_data(file_inode(seq->file));
>  	unsigned long position;
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR