[PATCH next 0/2] btrfs: Fix build warnings for arm

[PATCH next 0/2] btrfs: Fix build warnings for arm

[Andreas Färber]

Hello Wenruo and Dave,

This mini-series fixes two build warnings found while cross-compiling for arm,
using openSUSE's cross-arm-binutils and cross-arm-none-gcc9 packages.

Replacing BUG_ON() with btrfs_crit() error handling is noble work, but please
be careful not to hardcode format specifiers for x86_64's size_t.

https://www.kernel.org/doc/Documentation/printk-formats.txt

In one case it could've been noticed during review, in another it was hidden
through a macro and would've only been found through compile-testing.
Probably a 32-bit i386 build would do; otherwise ARCH=arm multi_v7_defconfig
plus CONFIG_BTRFS_FS should reproduce.

It's around for maybe three weeks, so I wonder why kbuild bot didn't catch it.

Cheers,
Andreas

Andreas Färber (2):
  btrfs: tree-checker: Fix error format string
  btrfs: extent-tree: Fix error format string

 fs/btrfs/extent-tree.c  | 2 +-
 fs/btrfs/tree-checker.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.16.4

[PATCH next 1/2] btrfs: tree-checker: Fix error format string

[Andreas Färber]

From: Andreas Färber <afaerber@suse.com>

Argument BTRFS_FILE_EXTENT_INLINE_DATA_START is defined as offsetof(),
which returns type size_t, so we need %zu instead of %lu.

This fixes a build warning on 32-bit arm:

  ../fs/btrfs/tree-checker.c: In function 'check_extent_data_item':
  ../fs/btrfs/tree-checker.c:230:43: warning: format '%lu' expects argument of type 'long unsigned int', but argument 5 has type 'unsigned int' [-Wformat=]
    230 |     "invalid item size, have %u expect [%lu, %u)",
        |                                         ~~^
        |                                           |
        |                                           long unsigned int
        |                                         %u

Fixes: a31ccb4b7ba2 ("btrfs: tree-checker: Check item size before reading file extent type")
Cc: Qu Wenruo <wqu@suse.com>
Cc: David Sterba <dsterba@suse.com>
Signed-off-by: Andreas Färber <afaerber@suse.com>
---
 fs/btrfs/tree-checker.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index 493d4d9e0f79..092b8ece36d7 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -227,7 +227,7 @@ static int check_extent_data_item(struct extent_buffer *leaf,
 	 */
 	if (item_size < BTRFS_FILE_EXTENT_INLINE_DATA_START) {
 		file_extent_err(leaf, slot,
-				"invalid item size, have %u expect [%lu, %u)",
+				"invalid item size, have %u expect [%zu, %u)",
 				item_size, BTRFS_FILE_EXTENT_INLINE_DATA_START,
 				SZ_4K);
 		return -EUCLEAN;
-- 
2.16.4

[PATCH next 2/2] btrfs: extent-tree: Fix error format string

[Andreas Färber]

From: Andreas Färber <afaerber@suse.com>

sizeof() returns type size_t, thus we need %zu instead of %lu.

This fixes the following build warning on 32-bit arm:

  In file included from ../include/linux/printk.h:7,
                   from ../include/linux/kernel.h:15,
                   from ../include/asm-generic/bug.h:19,
                   from ../arch/arm/include/asm/bug.h:60,
                   from ../include/linux/bug.h:5,
                   from ../include/linux/thread_info.h:12,
                   from ../include/asm-generic/current.h:5,
                   from ./arch/arm/include/generated/asm/current.h:1,
                   from ../include/linux/sched.h:12,
                   from ../fs/btrfs/extent-tree.c:6:
  ../fs/btrfs/extent-tree.c: In function '__btrfs_free_extent':
  ../include/linux/kern_levels.h:5:18: warning: format '%lu' expects argument of type 'long unsigned int', but argument 8 has type 'unsigned int' [-Wformat=]
      5 | #define KERN_SOH "\001"  /* ASCII Start Of Header */
        |                  ^~~~~~
  ../include/linux/kern_levels.h:10:19: note: in expansion of macro 'KERN_SOH'
     10 | #define KERN_CRIT KERN_SOH "2" /* critical conditions */
        |                   ^~~~~~~~
  ../fs/btrfs/ctree.h:2986:24: note: in expansion of macro 'KERN_CRIT'
   2986 |  btrfs_printk(fs_info, KERN_CRIT fmt, ##args)
        |                        ^~~~~~~~~
  ../fs/btrfs/extent-tree.c:3207:4: note: in expansion of macro 'btrfs_crit'
   3207 |    btrfs_crit(info,
        |    ^~~~~~~~~~
  ../fs/btrfs/extent-tree.c:3208:83: note: format string is defined here
   3208 | "invalid extent item size for key (%llu, %u, %llu) owner %llu, has %u expect >= %lu",
        |                                                                                 ~~^
        |                                                                                   |
        |                                                                                   long unsigned int
        |                                                                                 %u

Fixes: 0c171e9095e4 ("btrfs: extent-tree: Kill BUG_ON() in __btrfs_free_extent() and do better comment")
Cc: Qu Wenruo <wqu@suse.com>
Cc: David Sterba <dsterba@suse.com>
Signed-off-by: Andreas Färber <afaerber@suse.com>
---
 fs/btrfs/extent-tree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 7c7a3e30e917..631c9743ddc7 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -3205,7 +3205,7 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
 		struct btrfs_tree_block_info *bi;
 		if (unlikely(item_size < sizeof(*ei) + sizeof(*bi))) {
 			btrfs_crit(info,
-"invalid extent item size for key (%llu, %u, %llu) owner %llu, has %u expect >= %lu",
+"invalid extent item size for key (%llu, %u, %llu) owner %llu, has %u expect >= %zu",
 				   key.objectid, key.type, key.offset,
 				   owner_objectid, item_size,
 				   sizeof(*ei) + sizeof(*bi));
-- 
2.16.4

Re: [PATCH next 1/2] btrfs: tree-checker: Fix error format string

[David Sterba]

On Fri, Nov 08, 2019 at 10:38:52PM +0100, Andreas Färber wrote:
> From: Andreas Färber <afaerber@suse.com>
> 
> Argument BTRFS_FILE_EXTENT_INLINE_DATA_START is defined as offsetof(),
> which returns type size_t, so we need %zu instead of %lu.
> 
> This fixes a build warning on 32-bit arm:
> 
>   ../fs/btrfs/tree-checker.c: In function 'check_extent_data_item':
>   ../fs/btrfs/tree-checker.c:230:43: warning: format '%lu' expects argument of type 'long unsigned int', but argument 5 has type 'unsigned int' [-Wformat=]
>     230 |     "invalid item size, have %u expect [%lu, %u)",
>         |                                         ~~^
>         |                                           |
>         |                                           long unsigned int
>         |                                         %u

Is there a gcc warning option that can catch that on 64bit too?
-Wformat=2 does not and I don't see any other of the option family to do
that. We've had fixups of the size_t printk formats and I'd like to
catch that when the patches are added to the devel branches. I can't run
32bit build check each time but this seems to be the only way so far.

> Fixes: a31ccb4b7ba2 ("btrfs: tree-checker: Check item size before reading file extent type")

As the patch is still in the devel branch, the commit id is not stable
and I'll fold the change to to the patch. Thanks.

Re: [PATCH next 1/2] btrfs: tree-checker: Fix error format string

[Geert Uytterhoeven]

Hi David,

On Mon, Nov 11, 2019 at 7:32 PM David Sterba <dsterba@suse.cz> wrote:
> On Fri, Nov 08, 2019 at 10:38:52PM +0100, Andreas Färber wrote:
> > From: Andreas Färber <afaerber@suse.com>
> >
> > Argument BTRFS_FILE_EXTENT_INLINE_DATA_START is defined as offsetof(),
> > which returns type size_t, so we need %zu instead of %lu.
> >
> > This fixes a build warning on 32-bit arm:
> >
> >   ../fs/btrfs/tree-checker.c: In function 'check_extent_data_item':
> >   ../fs/btrfs/tree-checker.c:230:43: warning: format '%lu' expects argument of type 'long unsigned int', but argument 5 has type 'unsigned int' [-Wformat=]
> >     230 |     "invalid item size, have %u expect [%lu, %u)",
> >         |                                         ~~^
> >         |                                           |
> >         |                                           long unsigned int
> >         |                                         %u
>
> Is there a gcc warning option that can catch that on 64bit too?
> -Wformat=2 does not and I don't see any other of the option family to do
> that. We've had fixups of the size_t printk formats and I'd like to
> catch that when the patches are added to the devel branches. I can't run
> 32bit build check each time but this seems to be the only way so far.

Yep. On 64-bit, size_t _is_ unsigned long.
So you have to compile for both 32-bit and 64-bit.

Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>

> > Fixes: a31ccb4b7ba2 ("btrfs: tree-checker: Check item size before reading file extent type")
>
> As the patch is still in the devel branch, the commit id is not stable

It indeed is not:
Fixes: 153a6d299956983d ("btrfs: tree-checker: Check item size before
reading file extent type")

> and I'll fold the change to to the patch. Thanks.

Apparently that was forgotten, and now the issue is part of Linus' tree.

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH next 1/2] btrfs: tree-checker: Fix error format string

[David Sterba]

On Tue, Nov 26, 2019 at 11:36:58AM +0100, Geert Uytterhoeven wrote:
> > > Fixes: a31ccb4b7ba2 ("btrfs: tree-checker: Check item size before reading file extent type")
> >
> > As the patch is still in the devel branch, the commit id is not stable
> 
> It indeed is not:
> Fixes: 153a6d299956983d ("btrfs: tree-checker: Check item size before
> reading file extent type")
> 
> > and I'll fold the change to to the patch. Thanks.
> 
> Apparently that was forgotten, and now the issue is part of Linus' tree.

Mistake on my side so I'll apply the full fixup patch, thanks for
noticing.