Re: [PATCH] btrfs: Fix csum_tree_block to avoid tripping on -Werror=array-bounds
From: Qu Wenruo
Date: Tue May 23 2023 - 19:47:15 EST
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@xxxxxxxxxx>
---
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);