| Message ID | 20210701160039.12518-1-dsterba@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: add special case to setget helpers for 64k pages | expand |
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 > > Link: https://lore.kernel.org/lkml/20210623083901.1d49d19d@canb.auug.org.au/ > 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 >
On 2021/7/2 上午5:57, Gustavo A. R. Silva wrote: > 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[]; > }; But wouldn't that make the the size of extent_buffer structure change and affect the kmem cache for it? Thanks, Qu > > /* > > 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 > >> >> Link: https://lore.kernel.org/lkml/20210623083901.1d49d19d@canb.auug.org.au/ >> 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 >>
On 7/1/21 18:59, Qu Wenruo wrote: > > > On 2021/7/2 上午5:57, Gustavo A. R. Silva wrote: >> 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[]; >> }; > > 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? 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 -- Gustavo > > Thanks, > Qu >> >> /* >> >> 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 >> >>> >>> Link: https://lore.kernel.org/lkml/20210623083901.1d49d19d@canb.auug.org.au/ >>> 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 >>>
On 2021/7/2 上午8:09, Gustavo A. R. Silva wrote: > > > On 7/1/21 18:59, Qu Wenruo wrote: >> >> >> On 2021/7/2 上午5:57, Gustavo A. R. Silva wrote: >>> 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[]; >>> }; >> >> 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. Thanks, Qu > > 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 > -- > Gustavo > >> >> Thanks, >> Qu >>> >>> /* >>> >>> 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 >>> >>>> >>>> Link: https://lore.kernel.org/lkml/20210623083901.1d49d19d@canb.auug.org.au/ >>>> 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 >>>>
On 2021/7/2 上午8:39, Gustavo A. R. Silva wrote: > On Fri, Jul 02, 2021 at 08:21:31AM +0800, Qu Wenruo wrote: >> >> >> On 2021/7/2 上午8:09, Gustavo A. R. Silva wrote: >>> >>> >>> On 7/1/21 18:59, Qu Wenruo wrote: >>>> >>>> >>>> On 2021/7/2 上午5:57, Gustavo A. R. Silva wrote: >>>>> 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[]; >>>>> }; >>>> >>>> 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 eventually > > Do you think there is any other place where we should update > the total size for extent_buffer? Nope, that should be enough. Thanks, Qu > > -- > Gustavo > > >> >> Thanks, >> Qu >>> >>> 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 >>> -- >>> Gustavo >>> >>>> >>>> Thanks, >>>> Qu >>>>> >>>>> /* >>>>> >>>>> 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 >>>>> >>>>>> >>>>>> Link: https://lore.kernel.org/lkml/20210623083901.1d49d19d@canb.auug.org.au/ >>>>>> 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 >>>>>>
On Fri, Jul 02, 2021 at 08:21:31AM +0800, Qu Wenruo wrote: > > > On 2021/7/2 上午8:09, Gustavo A. R. Silva wrote: > > > > > > On 7/1/21 18:59, Qu Wenruo wrote: > > > > > > > > > On 2021/7/2 上午5:57, Gustavo A. R. Silva wrote: > > > > 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[]; > > > > }; > > > > > > 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 eventually Do you think there is any other place where we should update the total size for extent_buffer? -- Gustavo > > Thanks, > Qu > > > > 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 > > -- > > Gustavo > > > > > > > > Thanks, > > > Qu > > > > > > > > /* > > > > > > > > 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 > > > > > > > > > > > > > > Link: https://lore.kernel.org/lkml/20210623083901.1d49d19d@canb.auug.org.au/ > > > > > 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 > > > > >
On Fri, Jul 02, 2021 at 08:39:06AM +0800, Qu Wenruo wrote: > > Do you think there is any other place where we should update > > the total size for extent_buffer? > > Nope, that should be enough. Awesome. :) Here is the proper patch: https://lore.kernel.org/linux-btrfs/20210702010653.GA84106@embeddedor/ Thanks, Qu. -- Gustavo
> + if (INLINE_EXTENT_BUFFER_PAGES == 1) { \ > return get_unaligned_le##bits(token->kaddr + oip); \ > + } else { \ No need for an else after the return and thus no need for all the reformatting.
On Thu, Jul 01, 2021 at 07:39:36PM -0500, Gustavo A. R. Silva wrote: > On Fri, Jul 02, 2021 at 08:21:31AM +0800, Qu Wenruo wrote: > > On 2021/7/2 上午8:09, Gustavo A. R. Silva wrote: > > > On 7/1/21 18:59, Qu Wenruo wrote: > > > > On 2021/7/2 上午5:57, Gustavo A. R. Silva wrote: > > > > > 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[]; > > > > > }; > > > > > > > > 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[]; IMHO this is going the wrong way, INLINE_EXTENT_BUFFER_PAGES is a compile time constant and the array is not variable sized at all, so adding the end member and using struct_size is just manually coding what would compiler do for free.
On Fri, Jul 02, 2021 at 08:10:50AM +0100, Christoph Hellwig wrote: > > + if (INLINE_EXTENT_BUFFER_PAGES == 1) { \ > > return get_unaligned_le##bits(token->kaddr + oip); \ > > + } else { \ > > No need for an else after the return and thus no need for all the > reformatting. That leads to worse code, compiler does not eliminate the block that would otherwise be in the else block. Measured on x86_64 with instrumented code to force INLINE_EXTENT_BUFFER_PAGES = 1 this adds +1100 bytes of code and has impact on stack consumption. That the code that is in two branches that do not share any code is maybe not pretty but the compiler did what I expected. The set/get helpers get called a lot and are performance sensitive. This patch pre (original version), post (with dropped else): 1156210 19305 14912 1190427 122a1b pre/btrfs.ko 1157386 19305 14912 1191603 122eb3 post/btrfs.ko DELTA: +1176 And effect on function stacks: btrfs_set_token_32 +8 (48 -> 56) btrfs_set_token_64 +16 (48 -> 64) btrfs_set_32 +32 (32 -> 64) btrfs_set_16 +32 (32 -> 64) btrfs_set_token_16 +8 (48 -> 56) btrfs_set_64 +40 (32 -> 72) btrfs_set_token_8 -8 (48 -> 40)
On Fri, Jul 02, 2021 at 01:06:30PM +0200, David Sterba wrote: > On Fri, Jul 02, 2021 at 08:10:50AM +0100, Christoph Hellwig wrote: > > > + if (INLINE_EXTENT_BUFFER_PAGES == 1) { \ > > > return get_unaligned_le##bits(token->kaddr + oip); \ > > > + } else { \ > > > > No need for an else after the return and thus no need for all the > > reformatting. > > That leads to worse code, compiler does not eliminate the block that > would otherwise be in the else block. Measured on x86_64 with > instrumented code to force INLINE_EXTENT_BUFFER_PAGES = 1 this adds > +1100 bytes of code and has impact on stack consumption. > > That the code that is in two branches that do not share any code is > maybe not pretty but the compiler did what I expected. The set/get > helpers get called a lot and are performance sensitive. > > This patch pre (original version), post (with dropped else): > > 1156210 19305 14912 1190427 122a1b pre/btrfs.ko > 1157386 19305 14912 1191603 122eb3 post/btrfs.ko For the obvious trivial patch (see below) I see the following difference, which actually makes the simple change smaller: text data bss dec hex filename 1322580 112183 27600 1462363 16505b fs/btrfs/btrfs.o.hch 1322832 112183 27600 1462615 165157 fs/btrfs/btrfs.o.dave This is sing the following compiler: gcc version 10.2.1 20210110 (Debian 10.2.1-6) --- diff --git a/fs/btrfs/struct-funcs.c b/fs/btrfs/struct-funcs.c index 8260f8bb3ff0..a20954f06ec8 100644 --- a/fs/btrfs/struct-funcs.c +++ b/fs/btrfs/struct-funcs.c @@ -73,7 +73,7 @@ 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 || oip + size <= PAGE_SIZE) \ return get_unaligned_le##bits(token->kaddr + oip); \ \ memcpy(lebytes, token->kaddr + oip, part); \ @@ -94,7 +94,7 @@ 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 || oip + size <= PAGE_SIZE) \ return get_unaligned_le##bits(kaddr + oip); \ \ memcpy(lebytes, kaddr + oip, part); \ @@ -124,7 +124,7 @@ 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 || oip + size <= PAGE_SIZE) { \ put_unaligned_le##bits(val, token->kaddr + oip); \ return; \ } \ @@ -146,7 +146,7 @@ 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 || oip + size <= PAGE_SIZE) { \ put_unaligned_le##bits(val, kaddr + oip); \ return; \ } \
On Mon, Jul 05, 2021 at 09:33:34AM +0100, Christoph Hellwig wrote: > On Fri, Jul 02, 2021 at 01:06:30PM +0200, David Sterba wrote: > > On Fri, Jul 02, 2021 at 08:10:50AM +0100, Christoph Hellwig wrote: > > > > + if (INLINE_EXTENT_BUFFER_PAGES == 1) { \ > > > > return get_unaligned_le##bits(token->kaddr + oip); \ > > > > + } else { \ > > > > > > No need for an else after the return and thus no need for all the > > > reformatting. > > > > That leads to worse code, compiler does not eliminate the block that > > would otherwise be in the else block. Measured on x86_64 with > > instrumented code to force INLINE_EXTENT_BUFFER_PAGES = 1 this adds > > +1100 bytes of code and has impact on stack consumption. > > > > That the code that is in two branches that do not share any code is > > maybe not pretty but the compiler did what I expected. The set/get > > helpers get called a lot and are performance sensitive. > > > > This patch pre (original version), post (with dropped else): > > > > 1156210 19305 14912 1190427 122a1b pre/btrfs.ko > > 1157386 19305 14912 1191603 122eb3 post/btrfs.ko > > For the obvious trivial patch (see below) I see the following > difference, which actually makes the simple change smaller: > > text data bss dec hex filename > 1322580 112183 27600 1462363 16505b fs/btrfs/btrfs.o.hch > 1322832 112183 27600 1462615 165157 fs/btrfs/btrfs.o.dave This was on x86_64 and without any further changes to the extent_buffer::pages, right? I've tested your version with the following diff emulating the single page that would be on ppc: --- a/fs/btrfs/extent_io.h +++ b/fs/btrfs/extent_io.h @@ -94,7 +94,8 @@ struct extent_buffer { struct rw_semaphore lock; - struct page *pages[INLINE_EXTENT_BUFFER_PAGES]; + struct page *pages[1]; + /* struct page *pages[INLINE_EXTENT_BUFFER_PAGES]; */ struct list_head release_list; #ifdef CONFIG_BTRFS_DEBUG struct list_head leak_list; diff --git a/fs/btrfs/struct-funcs.c b/fs/btrfs/struct-funcs.c index 8260f8bb3ff0..4f8e8f7b29d1 100644 --- a/fs/btrfs/struct-funcs.c +++ b/fs/btrfs/struct-funcs.c @@ -52,6 +52,8 @@ static bool check_setget_bounds(const struct extent_buffer *eb, * from 0 to metadata node size. */ +#define _INLINE_EXTENT_BUFFER_PAGES 1 ... --- And replacing _INLINE_EXTENT_BUFFER_PAGES in the checks. This leads to the same result as in my original version with the copied blocks: text data bss dec hex filename 1161350 19305 14912 1195567 123e2f pre/btrfs.ko 1156090 19305 14912 1190307 1229a3 post/btrfs.ko DELTA: -5260 ie. compiler properly removed the dead code after evaluating the conditions. As your change is simpler code I'll take it, tahnks for the suggestion.
David, Is it OK with you if we proceed to enable -Warray-bounds in linux-next, in the meantime? Apparently, these are the last warnings remaining to be fixed before we can globally enable that compiler option and, it will be really helpful to at least have it enabled in linux-next for the rest of the development cycle, in case there are some other corner cases that we are not aware of yet. Thanks -- Gustavo On 7/8/21 09:34, David Sterba wrote: > On Mon, Jul 05, 2021 at 09:33:34AM +0100, Christoph Hellwig wrote: >> On Fri, Jul 02, 2021 at 01:06:30PM +0200, David Sterba wrote: >>> On Fri, Jul 02, 2021 at 08:10:50AM +0100, Christoph Hellwig wrote: >>>>> + if (INLINE_EXTENT_BUFFER_PAGES == 1) { \ >>>>> return get_unaligned_le##bits(token->kaddr + oip); \ >>>>> + } else { \ >>>> >>>> No need for an else after the return and thus no need for all the >>>> reformatting. >>> >>> That leads to worse code, compiler does not eliminate the block that >>> would otherwise be in the else block. Measured on x86_64 with >>> instrumented code to force INLINE_EXTENT_BUFFER_PAGES = 1 this adds >>> +1100 bytes of code and has impact on stack consumption. >>> >>> That the code that is in two branches that do not share any code is >>> maybe not pretty but the compiler did what I expected. The set/get >>> helpers get called a lot and are performance sensitive. >>> >>> This patch pre (original version), post (with dropped else): >>> >>> 1156210 19305 14912 1190427 122a1b pre/btrfs.ko >>> 1157386 19305 14912 1191603 122eb3 post/btrfs.ko >> >> For the obvious trivial patch (see below) I see the following >> difference, which actually makes the simple change smaller: >> >> text data bss dec hex filename >> 1322580 112183 27600 1462363 16505b fs/btrfs/btrfs.o.hch >> 1322832 112183 27600 1462615 165157 fs/btrfs/btrfs.o.dave > > This was on x86_64 and without any further changes to the > extent_buffer::pages, right? > > I've tested your version with the following diff emulating the single > page that would be on ppc: > > --- a/fs/btrfs/extent_io.h > +++ b/fs/btrfs/extent_io.h > @@ -94,7 +94,8 @@ struct extent_buffer { > > struct rw_semaphore lock; > > - struct page *pages[INLINE_EXTENT_BUFFER_PAGES]; > + struct page *pages[1]; > + /* struct page *pages[INLINE_EXTENT_BUFFER_PAGES]; */ > struct list_head release_list; > #ifdef CONFIG_BTRFS_DEBUG > struct list_head leak_list; > diff --git a/fs/btrfs/struct-funcs.c b/fs/btrfs/struct-funcs.c > index 8260f8bb3ff0..4f8e8f7b29d1 100644 > --- a/fs/btrfs/struct-funcs.c > +++ b/fs/btrfs/struct-funcs.c > @@ -52,6 +52,8 @@ static bool check_setget_bounds(const struct extent_buffer *eb, > * from 0 to metadata node size. > */ > > +#define _INLINE_EXTENT_BUFFER_PAGES 1 > ... > --- > > And replacing _INLINE_EXTENT_BUFFER_PAGES in the checks. This leads to > the same result as in my original version with the copied blocks: > > text data bss dec hex filename > 1161350 19305 14912 1195567 123e2f pre/btrfs.ko > 1156090 19305 14912 1190307 1229a3 post/btrfs.ko > > DELTA: -5260 > > ie. compiler properly removed the dead code after evaluating the > conditions. As your change is simpler code I'll take it, tahnks for the > suggestion. >
On Wed, Jul 14, 2021 at 06:37:01PM -0500, Gustavo A. R. Silva wrote: > Is it OK with you if we proceed to enable -Warray-bounds in linux-next, > in the meantime? Yes, I've checked the development queue and there are no warnings from fs/btrfs/.
On Wed, Jul 28, 2021 at 05:32:42PM +0200, David Sterba wrote: > On Wed, Jul 14, 2021 at 06:37:01PM -0500, Gustavo A. R. Silva wrote: > > Is it OK with you if we proceed to enable -Warray-bounds in linux-next, > > in the meantime? > > Yes, I've checked the development queue and there are no warnings from > fs/btrfs/. Sorry, not yet, there's the other issue that happens on 64k pages that lead to a 1 element extent_buffer::pages. I can emulate that and see the warning fs/btrfs/disk-io.c: In function ‘csum_tree_block’: fs/btrfs/disk-io.c:226:34: warning: array subscript 1 is above array bounds of ‘struct page *[1]’ [-Warray-bounds] 226 | kaddr = page_address(buf->pages[i]); | ~~~~~~~~~~^~~ ./include/linux/mm.h:1630:48: note: in definition of macro ‘page_address’ 1630 | #define page_address(page) lowmem_page_address(page) | ^~~~ In file included from fs/btrfs/ctree.h:32, from fs/btrfs/disk-io.c:23: fs/btrfs/extent_io.h:98:15: note: while referencing ‘pages’ 98 | struct page *pages[1]; | ^~~~~ but that's easy to fix with --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -210,7 +210,7 @@ void btrfs_set_buffer_lockdep_class(u64 objectid, struct extent_buffer *eb, static void csum_tree_block(struct extent_buffer *buf, u8 *result) { struct btrfs_fs_info *fs_info = buf->fs_info; - const int num_pages = fs_info->nodesize >> PAGE_SHIFT; + const int num_pages = num_extent_pages(buf); const int first_page_part = min_t(u32, PAGE_SIZE, fs_info->nodesize); SHASH_DESC_ON_STACK(shash, fs_info->csum_shash); char *kaddr; --- I'll send a patch, it'll appear in for-next soonish.
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)
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. Link: https://lore.kernel.org/lkml/20210623083901.1d49d19d@canb.auug.org.au/ 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(-)