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[];