Messages in this thread | | | Date | Wed, 24 May 2023 11:29:38 +0200 | From | David Sterba <> | Subject | Re: [PATCH] btrfs: Fix csum_tree_block to avoid tripping on -Werror=array-bounds |
| |
On Wed, May 24, 2023 at 07:46:42AM +0800, Qu Wenruo wrote: > > > 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...
And then comes some joker and says "why don't you just handle the first page separately and then loop over full pages" :)
We could also put the whole loop under #if INLINE_EB_PAGES > 1. I've checked that this type of iteration over the pages is only present in this function so this should not become a pattern that would spread elsewhere.
| |