[PATCH] btrfs-progs: fix accessors for big endian systems

[PATCH] btrfs-progs: fix accessors for big endian systems

[Qu Wenruo]

[BUG]
There is a bug report from the github issues, that on s390x big endian
systems, mkfs.btrfs just fails:

 $ mkfs.btrfs  -f ~/test.img
 btrfs-progs v6.3.1
 Invalid mapping for 1081344-1097728, got 17592186044416-17592190238720
 Couldn't map the block 1081344
 ERROR: cannot read chunk root
 ERROR: open ctree failed

[CAUSE]
The error is caused by wrong endian conversion.

The system where Fedora guys reported errors are using big endian:

 $ lscpu
 Byte Order:            Big Endian

While checking the offending @disk_key and @key inside
btrfs_read_sys_array(), we got:

 2301		while (cur_offset < array_size) {
 (gdb)
 2304			if (cur_offset + len > array_size)
 (gdb)
 2307			btrfs_disk_key_to_cpu(&key, disk_key);
 (gdb)
 2310			sb_array_offset += len;
 (gdb) print *disk_key
 $2 = {objectid = 281474976710656, type = 228 '\344', offset = 17592186044416}
 (gdb) print key
 $3 = {objectid = 281474976710656, type = 228 '\344', offset = 17592186044416}
 (gdb)

Now we can see, @disk_key is indeed in the little endian, but @key is
not converted to the CPU native endian.

Furthermore, if we step into the help btrfs_disk_key_to_cpu(), it shows
we're using little endian version:

 (gdb) step
 btrfs_disk_key_to_cpu (disk_key=0x109fcdb, cpu_key=0x3ffffff847f)
     at ./kernel-shared/accessors.h:592
 592		memcpy(cpu_key, disk_key, sizeof(struct btrfs_key));

[FIX]
The kernel accessors.h checks if __LITTLE_ENDIAN is defined or not, but
that only works inside kernel.

In user space, __LITTLE_ENDIAN and __BIG_ENDIAN are both defined inside
<bit/endian.h> header.

Instead we should check __BYTE_ORDER against __LITTLE_ENDIAN to
determine our endianness.

With this change, s390x build works as expected now.

Issue: #639
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 kernel-shared/accessors.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel-shared/accessors.h b/kernel-shared/accessors.h
index 625acfbe8ca7..06ab6e7e9f12 100644
--- a/kernel-shared/accessors.h
+++ b/kernel-shared/accessors.h
@@ -7,6 +7,8 @@
 #define _static_assert(expr)   _Static_assert(expr, #expr)
 #endif
 
+#include <bits/endian.h>
+
 struct btrfs_map_token {
 	struct extent_buffer *eb;
 	char *kaddr;
@@ -579,7 +581,7 @@ BTRFS_SETGET_STACK_FUNCS(disk_key_objectid, struct btrfs_disk_key, objectid, 64)
 BTRFS_SETGET_STACK_FUNCS(disk_key_offset, struct btrfs_disk_key, offset, 64);
 BTRFS_SETGET_STACK_FUNCS(disk_key_type, struct btrfs_disk_key, type, 8);
 
-#ifdef __LITTLE_ENDIAN
+#if __BYTE_ORDER == __LITTLE_ENDIAN
 
 /*
  * Optimized helpers for little-endian architectures where CPU and on-disk
-- 
2.41.0

Re: [PATCH] btrfs-progs: fix accessors for big endian systems

[David Sterba]

On Wed, Jun 14, 2023 at 06:23:43PM +0800, Qu Wenruo wrote:
> [BUG]
> There is a bug report from the github issues, that on s390x big endian
> systems, mkfs.btrfs just fails:
> 
>  $ mkfs.btrfs  -f ~/test.img
>  btrfs-progs v6.3.1
>  Invalid mapping for 1081344-1097728, got 17592186044416-17592190238720
>  Couldn't map the block 1081344
>  ERROR: cannot read chunk root
>  ERROR: open ctree failed
> 
> [CAUSE]
> The error is caused by wrong endian conversion.
> 
> The system where Fedora guys reported errors are using big endian:
> 
>  $ lscpu
>  Byte Order:            Big Endian
> 
> While checking the offending @disk_key and @key inside
> btrfs_read_sys_array(), we got:
> 
>  2301		while (cur_offset < array_size) {
>  (gdb)
>  2304			if (cur_offset + len > array_size)
>  (gdb)
>  2307			btrfs_disk_key_to_cpu(&key, disk_key);
>  (gdb)
>  2310			sb_array_offset += len;
>  (gdb) print *disk_key
>  $2 = {objectid = 281474976710656, type = 228 '\344', offset = 17592186044416}
>  (gdb) print key
>  $3 = {objectid = 281474976710656, type = 228 '\344', offset = 17592186044416}
>  (gdb)
> 
> Now we can see, @disk_key is indeed in the little endian, but @key is
> not converted to the CPU native endian.
> 
> Furthermore, if we step into the help btrfs_disk_key_to_cpu(), it shows
> we're using little endian version:
> 
>  (gdb) step
>  btrfs_disk_key_to_cpu (disk_key=0x109fcdb, cpu_key=0x3ffffff847f)
>      at ./kernel-shared/accessors.h:592
>  592		memcpy(cpu_key, disk_key, sizeof(struct btrfs_key));
> 
> [FIX]
> The kernel accessors.h checks if __LITTLE_ENDIAN is defined or not, but
> that only works inside kernel.
> 
> In user space, __LITTLE_ENDIAN and __BIG_ENDIAN are both defined inside
> <bit/endian.h> header.
> 
> Instead we should check __BYTE_ORDER against __LITTLE_ENDIAN to
> determine our endianness.
> 
> With this change, s390x build works as expected now.
> 
> Issue: #639
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Added to devel, thanks.

Re: [PATCH] btrfs-progs: fix accessors for big endian systems

[Johannes Thumshirn]

On 14.06.23 12:26, Qu Wenruo wrote:
> ---
>  kernel-shared/accessors.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel-shared/accessors.h b/kernel-shared/accessors.h
> index 625acfbe8ca7..06ab6e7e9f12 100644
> --- a/kernel-shared/accessors.h
> +++ b/kernel-shared/accessors.h
> @@ -7,6 +7,8 @@
>  #define _static_assert(expr)   _Static_assert(expr, #expr)
>  #endif
>  
> +#include <bits/endian.h>
> +
>  struct btrfs_map_token {
>  	struct extent_buffer *eb;
>  	char *kaddr;
> @@ -579,7 +581,7 @@ BTRFS_SETGET_STACK_FUNCS(disk_key_objectid, struct btrfs_disk_key, objectid, 64)
>  BTRFS_SETGET_STACK_FUNCS(disk_key_offset, struct btrfs_disk_key, offset, 64);
>  BTRFS_SETGET_STACK_FUNCS(disk_key_type, struct btrfs_disk_key, type, 8);
>  
> -#ifdef __LITTLE_ENDIAN
> +#if __BYTE_ORDER == __LITTLE_ENDIAN
>  
>  /*
>   * Optimized helpers for little-endian architectures where CPU and on-disk

Hmm but with a change like that we can't just copy over the
kernel files into user-space.

Just something we need to keep in mind.

Re: [PATCH] btrfs-progs: fix accessors for big endian systems

[David Sterba]

On Wed, Jun 14, 2023 at 05:50:32PM +0000, Johannes Thumshirn wrote:
> On 14.06.23 12:26, Qu Wenruo wrote:
> > ---
> >  kernel-shared/accessors.h | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel-shared/accessors.h b/kernel-shared/accessors.h
> > index 625acfbe8ca7..06ab6e7e9f12 100644
> > --- a/kernel-shared/accessors.h
> > +++ b/kernel-shared/accessors.h
> > @@ -7,6 +7,8 @@
> >  #define _static_assert(expr)   _Static_assert(expr, #expr)
> >  #endif
> >  
> > +#include <bits/endian.h>
> > +
> >  struct btrfs_map_token {
> >  	struct extent_buffer *eb;
> >  	char *kaddr;
> > @@ -579,7 +581,7 @@ BTRFS_SETGET_STACK_FUNCS(disk_key_objectid, struct btrfs_disk_key, objectid, 64)
> >  BTRFS_SETGET_STACK_FUNCS(disk_key_offset, struct btrfs_disk_key, offset, 64);
> >  BTRFS_SETGET_STACK_FUNCS(disk_key_type, struct btrfs_disk_key, type, 8);
> >  
> > -#ifdef __LITTLE_ENDIAN
> > +#if __BYTE_ORDER == __LITTLE_ENDIAN
> >  
> >  /*
> >   * Optimized helpers for little-endian architectures where CPU and on-disk
> 
> Hmm but with a change like that we can't just copy over the
> kernel files into user-space.
> 
> Just something we need to keep in mind.

There are more changes in all synced files, it can't be a direct copy
yet.

Re: [PATCH] btrfs-progs: fix accessors for big endian systems

[David Sterba]

On Wed, Jun 14, 2023 at 06:23:43PM +0800, Qu Wenruo wrote:
> --- a/kernel-shared/accessors.h
> +++ b/kernel-shared/accessors.h
> @@ -7,6 +7,8 @@
>  #define _static_assert(expr)   _Static_assert(expr, #expr)
>  #endif
>  
> +#include <bits/endian.h>

Files from bits/ should not be included directly and it's
glibc-specific, also breaks build on musl. Fixed.

I'm going to enable quick build tests for pull request, right now the
tests are ran once I push devel which usually happens after I reply with
the 'applied' mail. The test results can be found at
https://github.com/kdave/btrfs-progs/actions .

Re: [PATCH] btrfs-progs: fix accessors for big endian systems

[Qu Wenruo]

On 2023/6/15 17:28, David Sterba wrote:
> On Wed, Jun 14, 2023 at 06:23:43PM +0800, Qu Wenruo wrote:
>> --- a/kernel-shared/accessors.h
>> +++ b/kernel-shared/accessors.h
>> @@ -7,6 +7,8 @@
>>   #define _static_assert(expr)   _Static_assert(expr, #expr)
>>   #endif
>>
>> +#include <bits/endian.h>
>
> Files from bits/ should not be included directly and it's
> glibc-specific, also breaks build on musl. Fixed.

Weird, as for those things which should not be included, normally they
have something like this from endianness.h:

#ifndef _BITS_ENDIAN_H
# error "Never use XXXXX directly; include <whatever.h> instead."
#endif

Another concern removing this line is, we're relying on the final .c
file to include needed headers.
For this particular case it's not a problem at all as standard library
headers would be included anyway.

But I'm still not sure what should be the proper way to go.

>
> I'm going to enable quick build tests for pull request, right now the
> tests are ran once I push devel which usually happens after I reply with
> the 'applied' mail. The test results can be found at
> https://github.com/kdave/btrfs-progs/actions .

That would be very appreciated.

However this is better with github pull hooks, I'm not sure if this
means it's better to use github pull as the primary method, and just
keep the emails as backup methods for old school guys like me.

Thanks,
Qu

Re: [PATCH] btrfs-progs: fix accessors for big endian systems

[David Sterba]

On Thu, Jun 15, 2023 at 05:56:31PM +0800, Qu Wenruo wrote:
> 
> 
> On 2023/6/15 17:28, David Sterba wrote:
> > On Wed, Jun 14, 2023 at 06:23:43PM +0800, Qu Wenruo wrote:
> >> --- a/kernel-shared/accessors.h
> >> +++ b/kernel-shared/accessors.h
> >> @@ -7,6 +7,8 @@
> >>   #define _static_assert(expr)   _Static_assert(expr, #expr)
> >>   #endif
> >>
> >> +#include <bits/endian.h>
> >
> > Files from bits/ should not be included directly and it's
> > glibc-specific, also breaks build on musl. Fixed.
> 
> Weird, as for those things which should not be included, normally they
> have something like this from endianness.h:
> 
> #ifndef _BITS_ENDIAN_H
> # error "Never use XXXXX directly; include <whatever.h> instead."
> #endif

Yeah not all of them have it, they probably should.

> Another concern removing this line is, we're relying on the final .c
> file to include needed headers.
> For this particular case it's not a problem at all as standard library
> headers would be included anyway.
> 
> But I'm still not sure what should be the proper way to go.

Actually we have the proper way, to include kerncompat.h from all
kernel-shared files. There's endian.h included and there are also
__BYTE_ORDER checks done the right way. The accessors.h currently has
no includes at all.

Ideally all includes are self contained when compile tested, ie. all
types/macros/... are either properly defined by other includes or have a
forward definition. I had done a pass with include-what-you-use tool but
that was before the kernel source sync. Doing compile tests of headers
is simpler, using only compiler. Planned to be done eventually.

> > I'm going to enable quick build tests for pull request, right now the
> > tests are ran once I push devel which usually happens after I reply with
> > the 'applied' mail. The test results can be found at
> > https://github.com/kdave/btrfs-progs/actions .
> 
> That would be very appreciated.
> 
> However this is better with github pull hooks, I'm not sure if this
> means it's better to use github pull as the primary method, and just
> keep the emails as backup methods for old school guys like me.

Currently we do both and you can open a pull request with the patches
you send as mails to get the build tests. Or, you can do local build
test when you set up docker (how to do that is described in
ci/README.md), then it's one script away.