Re: [PATCH] btrfs: Fix csum_tree_block to avoid tripping on -Werror=array-bounds

[David Sterba]

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.

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);