Messages in this thread | | | Date | Wed, 24 May 2023 07:46:42 +0800 | From | Qu Wenruo <> | 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@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);
| |