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

[Qu Wenruo]

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.

I'll let David to give the final call.

Thanks,
Qu
> +		}
>   	}
>   	memset(result, 0, BTRFS_CSUM_SIZE);
>   	crypto_shash_final(shash, result);
>
>
> Content-type: Text/plain
>
> No virus found
> 		Checked by Hillstone Network AntiVirus