[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <bad56f93-6ffb-9505-92ce-67ab9e006e9c@gmx.com>
Date: Wed, 24 May 2023 07:46:42 +0800
From: Qu Wenruo <quwenruo.btrfs@....com>
To: dsterba@...e.cz
Cc: pengfuyuan <pengfuyuan@...inos.cn>, Chris Mason <clm@...com>,
Josef Bacik <josef@...icpanda.com>,
David Sterba <dsterba@...e.com>, linux-btrfs@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] btrfs: Fix csum_tree_block to avoid tripping on
-Werror=array-bounds
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@...inos.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);
Powered by blists - more mailing lists