linux-kernel - Re: [PATCH] bcachefs: rename version -> bversion for big endian builds

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fo6bvxt5o5veelshcig3zrqyktwvpxzxpvz4bb3n6gyk2vwejk@fx7opeolkbvj>
Date: Mon, 30 Sep 2024 08:31:02 -0400
From: Kent Overstreet <kent.overstreet@...ux.dev>
To: Geert Uytterhoeven <geert@...ux-m68k.org>
Cc: Guenter Roeck <linux@...ck-us.net>, linux-bcachefs@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH] bcachefs: rename version -> bversion for big endian
 builds

On Mon, Sep 30, 2024 at 02:08:25PM GMT, Geert Uytterhoeven wrote:
> Hi Kent,
> 
> On Mon, Sep 30, 2024 at 12:11 PM Kent Overstreet
> <kent.overstreet@...ux.dev> wrote:
> > On Mon, Sep 30, 2024 at 12:04:42PM GMT, Geert Uytterhoeven wrote:
> > > On Mon, Sep 30, 2024 at 2:39 AM Guenter Roeck <linux@...ck-us.net> wrote:
> > > > Builds on big endian systems fail as follows.
> > > >
> > > > fs/bcachefs/bkey.h: In function 'bch2_bkey_format_add_key':
> > > > fs/bcachefs/bkey.h:557:41: error:
> > > >         'const struct bkey' has no member named 'bversion'
> > > >
> > > > The original commit only renamed the variable for little endian builds.
> > > > Rename it for big endian builds as well to fix the problem.
> > > >
> > > > Fixes: cf49f8a8c277 ("bcachefs: rename version -> bversion")
> > >
> > > Which is (again) not found on any mailing list, and has never been in
> > > linux-next before it hit upstream...
> > >
> > > > Cc: Kent Overstreet <kent.overstreet@...ux.dev>
> > > > Signed-off-by: Guenter Roeck <linux@...ck-us.net>
> > >
> > > > --- a/fs/bcachefs/bcachefs_format.h
> > > > +++ b/fs/bcachefs/bcachefs_format.h
> > > > @@ -223,7 +223,7 @@ struct bkey {
> > > >  #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> > > >         struct bpos     p;
> > > >         __u32           size;           /* extent size, in sectors */
> > > > -       struct bversion version;
> > > > +       struct bversion bversion;
> > > >
> > > >         __u8            pad[1];
> > > >  #endif
> > >
> > > BTW, how does this work when accessing a non-native file system?
> > > Didn't we stop doing bi-endian file systems in v2.1.10, when ext2 was
> > > converted from a bi-endian to a little-endian file system?
> >
> > we byte swab if necessary
> 
> So you have to test 4 combinations instead of 2 (which you don't do,
> obviously ;-)
> 
> Ext2 was converted from a bi-endian to a little-endian file system
> because it turned out the conditional byte-swapping was more
> expensive than unconditional (not) byte-swapping. Given all the
> bcache structures are already tagged with __packed anyway, I guess
> this is even more true for bcachefs.
> 
> The proper way established +25y ago was to settle on one endianness
> layout for all on-disk data. That way you do not have to duplicate
> data and code for little vs. big endian, keep both paths in sync, and
> you can annotate everything with __[bl]eXX attributes to let sparse
> help you catch bugs.
> 
> Which endianness to pick is up to you. Ext2 settled on little-endian,
> XFS on big-endian.

If you peruse that code even slightly, you'll see that what we're doing
is treating the key as a multi word integer, so word order has to match
machine byte order in order for various things in the btree lookup code
to work.

But sure, try and tell me there's something about filesystems I don't
already know...