Re: [PATCH] btrfs: add special case to setget helpers for 64k pages
From: "Gustavo A. R. Silva" <gustavoars@kernel.org>
Date: 2021-07-01 21:55:49
Subsystem:
btrfs file system, filesystems (vfs and infrastructure), the rest · Maintainers:
Chris Mason, David Sterba, Alexander Viro, Christian Brauner, Linus Torvalds
On Thu, Jul 01, 2021 at 06:00:39PM +0200, David Sterba wrote:
On 64K pages the size of the extent_buffer::pages array is 1 and compilation with -Warray-bounds warns due to kaddr = page_address(eb->pages[idx + 1]); when reading byte range crossing page boundary. This does never actually overflow the array because on 64K because all the data fit in one page and bounds are checked by check_setget_bounds. To fix the reported overflow and warning add a copy of the non-crossing read/write code and put it behind a condition that's evaluated at compile time. That way only one implementation remains due to dead code elimination.
Any chance we can use a flexible-array in struct extent_buffer instead, so all the warnings are removed? Something like this:
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 62027f551b44..b82e8b694a3b 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h@@ -94,11 +94,11 @@ struct extent_buffer { struct rw_semaphore lock; - struct page *pages[INLINE_EXTENT_BUFFER_PAGES]; struct list_head release_list; #ifdef CONFIG_BTRFS_DEBUG struct list_head leak_list; #endif + struct page *pages[]; }; /*
which is actually what is needed in this case to silence the array-bounds warnings: the replacement of the one-element array with a flexible-array member[1] in struct extent_buffer. -- Gustavo [1] https://www.kernel.org/doc/html/v5.10/process/deprecated.html#zero-length-and-one-element-arrays
quoted hunk ↗ jump to hunk
Link: https://lore.kernel.org/lkml/20210623083901.1d49d19d@canb.auug.org.au/ (local) CC: Gustavo A. R. Silva <gustavoars@kernel.org> Signed-off-by: David Sterba <dsterba@suse.com> --- fs/btrfs/struct-funcs.c | 66 +++++++++++++++++++++++++---------------- 1 file changed, 41 insertions(+), 25 deletions(-)diff --git a/fs/btrfs/struct-funcs.c b/fs/btrfs/struct-funcs.c index 8260f8bb3ff0..51204b280da8 100644 --- a/fs/btrfs/struct-funcs.c +++ b/fs/btrfs/struct-funcs.c@@ -73,14 +73,18 @@ u##bits btrfs_get_token_##bits(struct btrfs_map_token *token, \ } \ token->kaddr = page_address(token->eb->pages[idx]); \ token->offset = idx << PAGE_SHIFT; \ - if (oip + size <= PAGE_SIZE) \ + if (INLINE_EXTENT_BUFFER_PAGES == 1) { \ return get_unaligned_le##bits(token->kaddr + oip); \ + } else { \ + if (oip + size <= PAGE_SIZE) \ + return get_unaligned_le##bits(token->kaddr + oip); \ \ - memcpy(lebytes, token->kaddr + oip, part); \ - token->kaddr = page_address(token->eb->pages[idx + 1]); \ - token->offset = (idx + 1) << PAGE_SHIFT; \ - memcpy(lebytes + part, token->kaddr, size - part); \ - return get_unaligned_le##bits(lebytes); \ + memcpy(lebytes, token->kaddr + oip, part); \ + token->kaddr = page_address(token->eb->pages[idx + 1]); \ + token->offset = (idx + 1) << PAGE_SHIFT; \ + memcpy(lebytes + part, token->kaddr, size - part); \ + return get_unaligned_le##bits(lebytes); \ + } \ } \ u##bits btrfs_get_##bits(const struct extent_buffer *eb, \ const void *ptr, unsigned long off) \@@ -94,13 +98,17 @@ u##bits btrfs_get_##bits(const struct extent_buffer *eb, \ u8 lebytes[sizeof(u##bits)]; \ \ ASSERT(check_setget_bounds(eb, ptr, off, size)); \ - if (oip + size <= PAGE_SIZE) \ + if (INLINE_EXTENT_BUFFER_PAGES == 1) { \ return get_unaligned_le##bits(kaddr + oip); \ + } else { \ + if (oip + size <= PAGE_SIZE) \ + return get_unaligned_le##bits(kaddr + oip); \ \ - memcpy(lebytes, kaddr + oip, part); \ - kaddr = page_address(eb->pages[idx + 1]); \ - memcpy(lebytes + part, kaddr, size - part); \ - return get_unaligned_le##bits(lebytes); \ + memcpy(lebytes, kaddr + oip, part); \ + kaddr = page_address(eb->pages[idx + 1]); \ + memcpy(lebytes + part, kaddr, size - part); \ + return get_unaligned_le##bits(lebytes); \ + } \ } \ void btrfs_set_token_##bits(struct btrfs_map_token *token, \ const void *ptr, unsigned long off, \@@ -124,15 +132,19 @@ void btrfs_set_token_##bits(struct btrfs_map_token *token, \ } \ token->kaddr = page_address(token->eb->pages[idx]); \ token->offset = idx << PAGE_SHIFT; \ - if (oip + size <= PAGE_SIZE) { \ + if (INLINE_EXTENT_BUFFER_PAGES == 1) { \ put_unaligned_le##bits(val, token->kaddr + oip); \ - return; \ + } else { \ + if (oip + size <= PAGE_SIZE) { \ + put_unaligned_le##bits(val, token->kaddr + oip); \ + return; \ + } \ + put_unaligned_le##bits(val, lebytes); \ + memcpy(token->kaddr + oip, lebytes, part); \ + token->kaddr = page_address(token->eb->pages[idx + 1]); \ + token->offset = (idx + 1) << PAGE_SHIFT; \ + memcpy(token->kaddr, lebytes + part, size - part); \ } \ - put_unaligned_le##bits(val, lebytes); \ - memcpy(token->kaddr + oip, lebytes, part); \ - token->kaddr = page_address(token->eb->pages[idx + 1]); \ - token->offset = (idx + 1) << PAGE_SHIFT; \ - memcpy(token->kaddr, lebytes + part, size - part); \ } \ void btrfs_set_##bits(const struct extent_buffer *eb, void *ptr, \ unsigned long off, u##bits val) \@@ -146,15 +158,19 @@ void btrfs_set_##bits(const struct extent_buffer *eb, void *ptr, \ u8 lebytes[sizeof(u##bits)]; \ \ ASSERT(check_setget_bounds(eb, ptr, off, size)); \ - if (oip + size <= PAGE_SIZE) { \ + if (INLINE_EXTENT_BUFFER_PAGES == 1) { \ put_unaligned_le##bits(val, kaddr + oip); \ - return; \ - } \ + } else { \ + if (oip + size <= PAGE_SIZE) { \ + put_unaligned_le##bits(val, kaddr + oip); \ + return; \ + } \ \ - put_unaligned_le##bits(val, lebytes); \ - memcpy(kaddr + oip, lebytes, part); \ - kaddr = page_address(eb->pages[idx + 1]); \ - memcpy(kaddr, lebytes + part, size - part); \ + put_unaligned_le##bits(val, lebytes); \ + memcpy(kaddr + oip, lebytes, part); \ + kaddr = page_address(eb->pages[idx + 1]); \ + memcpy(kaddr, lebytes + part, size - part); \ + } \ } DEFINE_BTRFS_SETGET_BITS(8)-- 2.31.1