Received-SPF: pass (google.com: domain of linux-kernel+bounces-103249-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <d166d7e6-bc55-8718-19a9-6bd97f4bd032@huawei.com>
Date: Thu, 14 Mar 2024 20:37:38 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.1.2
Subject: Re: [PATCH v2 4/9] ext4: fix slab-out-of-bounds in
 ext4_mb_find_good_group_avg_frag_lists()
Content-Language: en-US
To: Jan Kara <jack@suse.cz>
CC: <linux-ext4@vger.kernel.org>, <tytso@mit.edu>, <adilger.kernel@dilger.ca>,
	<ritesh.list@gmail.com>, <ojaswin@linux.ibm.com>, <adobriyan@gmail.com>,
	<linux-kernel@vger.kernel.org>, <yi.zhang@huawei.com>,
	<yangerkun@huawei.com>, <stable@vger.kernel.org>, Baokun Li
	<libaokun1@huawei.com>
References: <20240227091148.178435-1-libaokun1@huawei.com>
 <20240227091148.178435-5-libaokun1@huawei.com>
 <20240314103056.rykwi2hhfm7v575a@quack3>
 <50f9333b-831a-8b4b-a6f2-ae79ab46a88b@huawei.com>
 <20240314120011.xggrokdfuu6fh4uv@quack3>
From: Baokun Li <libaokun1@huawei.com>
In-Reply-To: <20240314120011.xggrokdfuu6fh4uv@quack3>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit

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
.