linux-hardening - Re: linux-next: Tree for Sep 12 (bcachefs)

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 for Android: free password hash cracker in your pocket

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

Date:   Thu, 14 Sep 2023 15:38:07 -0400
From:   Kent Overstreet <kent.overstreet@...ux.dev>
To:     Kees Cook <keescook@...omium.org>
Cc:     Stephen Rothwell <sfr@...b.auug.org.au>,
        Linux Next Mailing List <linux-next@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-hardening@...r.kernel.org
Subject: Re: linux-next: Tree for Sep 12 (bcachefs)

On Wed, Sep 13, 2023 at 06:17:00PM -0700, Kees Cook wrote:
> On Tue, Sep 12, 2023 at 03:26:45PM +1000, Stephen Rothwell wrote:
> > New tree: bcachefs
> 
> Thanks for going through and fixing all the fake flexible array members.
> It looks much nicer. :)
> 
> I have some questions about the remaining "markers", for example:
> 
> $ git grep -A8 '\bkey_start\b' -- fs/bcachefs
> fs/bcachefs/bcachefs_format.h:  __u8            key_start[0];
> ...
> fs/bcachefs/bcachefs_format.h-  __u8            pad[sizeof(struct bkey) - 3];
> --
> fs/bcachefs/bkey.c:     u8 *l = k->key_start;
> 
> Why isn't this just:
> 
> 			u8 *l = k->pad
> 
> and you can drop the marker?

In this case, it's documentation. &k->pad tells us nothing; why is pad
significant? k->key_start documents the intent better.

> And some seem entirely unused, like all of "struct bch_reflink_v".

No, those aren't unused :)

bcachefs does the "list of variable size items" a lot - see vstructs.h.
start[] is the type of the item being stored, _data is what we use for
pointer arithmetic - because we always store sizes in units of u64s, for
alignment.

> 
> And some are going to fail at runtime, since they're still zero-sized
> and being used as an actual array:
> 
> struct bch_sb_field_journal_seq_blacklist {
>         struct bch_sb_field     field;
> 
>         struct journal_seq_blacklist_entry start[0];
>         __u64                   _data[];
> };
> ...
>                 memmove(&bl->start[i],
>                         &bl->start[i + 1],
>                         sizeof(bl->start[0]) * (nr - i));
> 
> It looks like you just want a type union for the flexible array.
> This can be done like this:
> 
> struct bch_sb_field_journal_seq_blacklist {
>         struct bch_sb_field     field;
> 
> 	union {
> 		DECLARE_FLEX_ARRAY(struct journal_seq_blacklist_entry, start);
> 		DECLARE_FLEX_ARRAY(__u64, _data);
> 	};
> };

Eesh, why though?

Honestly, I'm not a fan of the change to get rid of zero size arrays,
this seems to be adding a whole lot of macro layering and indirection
for nothing.

The only thing a zero size array could possibly be is a flexible array
member or a marker, why couldn't we have just kept treating zero size
arrays like flexible array members?