Re: [PATCH] btrfs: Fix csum_tree_block to avoid tripping on -Werror=array-bounds

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2023/5/24 03:32, David Sterba wrote:
> On Tue, May 23, 2023 at 03:33:22PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2023/5/23 15:09, pengfuyuan wrote:
>>>
>>> When compiling on a mips 64-bit machine we get these warnings:
>>>
>>>       In file included from ./arch/mips/include/asm/cacheflush.h:13,
>>> 	             from ./include/linux/cacheflush.h:5,
>>> 	             from ./include/linux/highmem.h:8,
>>> 		     from ./include/linux/bvec.h:10,
>>> 		     from ./include/linux/blk_types.h:10,
>>>                        from ./include/linux/blkdev.h:9,
>>> 	             from fs/btrfs/disk-io.c:7:
>>>       fs/btrfs/disk-io.c: In function ‘csum_tree_block’:
>>>       fs/btrfs/disk-io.c:100:34: error: array subscript 1 is above array bounds of ‘struct page *[1]’ [-Werror=array-bounds]
>>>         100 |   kaddr = page_address(buf->pages[i]);
>>>             |                        ~~~~~~~~~~^~~
>>>       ./include/linux/mm.h:2135:48: note: in definition of macro ‘page_address’
>>>        2135 | #define page_address(page) lowmem_page_address(page)
>>>             |                                                ^~~~
>>>       cc1: all warnings being treated as errors
>>>
>>> We can check if i overflows to solve the problem. However, this doesn't make
>>> much sense, since i == 1 and num_pages == 1 doesn't execute the body of the loop.
>>> In addition, i < num_pages can also ensure that buf->pages[i] will not cross
>>> the boundary. Unfortunately, this doesn't help with the problem observed here:
>>> gcc still complains.
>>
>> So still false alerts, thus this bug should mostly be reported to GCC.
>>
>>>
>>> To fix this, start the loop at index 0 instead of 1. Also, a conditional was
>>> added to skip the case where the index is 0, so that the loop iterations follow
>>> the desired logic, and it makes all versions of gcc happy.
>>>
>>> Signed-off-by: pengfuyuan <pengfuyuan@kylinos.cn>
>>> ---
>>>    fs/btrfs/disk-io.c | 10 +++++++---
>>>    1 file changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>>> index fbf9006c6234..8b05d556d747 100644
>>> --- a/fs/btrfs/disk-io.c
>>> +++ b/fs/btrfs/disk-io.c
>>> @@ -96,9 +96,13 @@ static void csum_tree_block(struct extent_buffer *buf, u8 *result)
>>>    	crypto_shash_update(shash, kaddr + BTRFS_CSUM_SIZE,
>>>    			    first_page_part - BTRFS_CSUM_SIZE);
>>>
>>> -	for (i = 1; i < num_pages; i++) {
>>> -		kaddr = page_address(buf->pages[i]);
>>> -		crypto_shash_update(shash, kaddr, PAGE_SIZE);
>>> +	for (i = 0; i < num_pages; i++) {
>>> +		struct page *p = buf->pages[i];
>>> +
>>> +		if (i != 0) {
>>> +			kaddr = page_address(p);
>>> +			crypto_shash_update(shash, kaddr, PAGE_SIZE);
>>
>> Unfortunately this damages the readability.
>>
>> If you really want to starts from page index 0, I don't think doing this
>> is the correct way.
>>
>> Instead, you may take the chance to merge the first
>> crypto_shahs_update() call, so the overall procedure looks like this:
>>
>> static void csum_tree_block()
>> {
>> 	for (int i = 0; i < num_pages; i++) {
>> 		int page_off = whatever_to_calculate_the_offset;
>> 		int page_len = whatever_to_calculate_the_lengh;
>> 		char *kaddr = page_address(buf->pages[i]) + page_off;
>>
>> 		crypto_shash_update(shash, kaddr, page_len);
>> 	}
>> 	memset();
>> 	crypto_shash_final();
>> }
>>
>> Although even with such change, I'm still not sure if it's any better or
>> worse, as most of the calculation can still be bulky.
>
> Yeah I think the calculations would have to be conditional or keeping
> some state. I'd like to keep the structure of the first page and the
> rest.

Yeah, there would be conditional checks, but it turns out to be simpler
like the following:

	int cur = BTRFS_CSUM_SIZE;

	for (int i = 0; i < num_pages; i++) {
		int range_end = min(eb->len, (i + 1) << PAGE_SHIFT);
		int page_len = range_end - cur;
		int page_off = offset_in_page(cur);

		cypto_shash_update();
		cur = range_end;
	}

The only conditional thing is the min() call, but I'm not sure if this
is any more readable though...

Thanks,
Qu


>
> Possible ways is to add extra condition
>
> 	for (i = 1; i < num_pages && i < INLINE_EXTENT_BUFFER_PAGES; i++)
>
> which leads to dead code if page size is 64k. It still has to check two
> conditions which is not the best, so could do
>
> 	int num_pages = max(num_extent_pages(eb0, INLINE_EXTENT_BUFFER_PAGES);