lkml.org 
[lkml]   [2023]   [May]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@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);

\
 
 \ /
  Last update: 2023-05-24 01:48    [W:2.121 / U:0.900 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site