Re: [PATCH] btrfs: add special case to setget helpers for 64k pages
From: Qu Wenruo <hidden>
Date: 2021-07-02 00:39:31
On 2021/7/2 上午8:39, Gustavo A. R. Silva wrote:
quoted hunk ↗ jump to hunk
On Fri, Jul 02, 2021 at 08:21:31AM +0800, Qu Wenruo wrote:quoted
On 2021/7/2 上午8:09, Gustavo A. R. Silva wrote:quoted
On 7/1/21 18:59, Qu Wenruo wrote:quoted
On 2021/7/2 上午5:57, Gustavo A. R. Silva wrote:quoted
On Thu, Jul 01, 2021 at 06:00:39PM +0200, David Sterba wrote:quoted
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[]; };But wouldn't that make the the size of extent_buffer structure change and affect the kmem cache for it?Could you please point out the places in the code that would be affected?Sure, the direct code get affected is here: extent_io.c: int __init extent_io_init(void) { extent_buffer_cache = kmem_cache_create("btrfs_extent_buffer", sizeof(struct extent_buffer), 0, SLAB_MEM_SPREAD, NULL); So here we can no longer use sizeof(struct extent_buffer); Furthermore, this function is called at btrfs module load time, at that time we have no idea how large the extent buffer could be, thus we must allocate a large enough cache for extent buffer. Thus the size will be fixed to the current size, no matter if we use flex array or not. Though I'm not sure if using such flex array with fixed real size can silent the warning though.Yeah; I think this might be the right solution:diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 9e81d25dea70..4cf0b72fdd9f 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c@@ -232,8 +232,9 @@ int __init extent_state_cache_init(void) int __init extent_io_init(void) { extent_buffer_cache = kmem_cache_create("btrfs_extent_buffer", - sizeof(struct extent_buffer), 0, - SLAB_MEM_SPREAD, NULL); + struct_size((struct extent_buffer *)0, pages, + INLINE_EXTENT_BUFFER_PAGES), + 0, SLAB_MEM_SPREAD, NULL); if (!extent_buffer_cache) return -ENOMEM;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[]; }; /*I see zero warnings by building with make ARCH=powerpc CROSS_COMPILE=powerpc64-linux-gnu- ppc64_defconfig and -Warray-bounds enabled by default:diff --git a/Makefile b/Makefile index c2cc2fa28525..310452119ab5 100644 --- a/Makefile +++ b/Makefile@@ -1068,7 +1068,6 @@ KBUILD_CFLAGS += $(call cc-disable-warning, stringop-truncation) # We'll want to enable this eventually, but it's not going away for 5.7 at least KBUILD_CFLAGS += $(call cc-disable-warning, zero-length-bounds) -KBUILD_CFLAGS += $(call cc-disable-warning, array-bounds) KBUILD_CFLAGS += $(call cc-disable-warning, stringop-overflow) # Another good warning that we'll want to enable eventuallyDo you think there is any other place where we should update the total size for extent_buffer?
Nope, that should be enough. Thanks, Qu
-- Gustavoquoted
Thanks, Ququoted
I'm trying to understand this code and see the possibility of using a flex-array together with proper memory allocation, so we can avoid having one-element array in extent_buffer. Not sure at what extent this would be possible. So, any pointer is greatly appreciate it. :) Thanks -- Gustavoquoted
Thanks, Ququoted
/* 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-arraysquoted
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