Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Date:   Thu, 14 Sep 2023 15:38:07 -0400
From:   Kent Overstreet <kent.overstreet@linux.dev>
To:     Kees Cook <keescook@chromium.org>
Cc:     Stephen Rothwell <sfr@canb.auug.org.au>,
        Linux Next Mailing List <linux-next@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-hardening@vger.kernel.org
Subject: Re: linux-next: Tree for Sep 12 (bcachefs)
Message-ID: <20230914193807.ozcmylp6n6dsqkbi@moria.home.lan>
References: <20230912152645.0868a96a@canb.auug.org.au>
 <202309131803.6A3C1D05A@keescook>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <202309131803.6A3C1D05A@keescook>
Precedence: bulk

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?