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

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

[Kees Cook]

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?

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

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);
	};
};

Hopefully that helps!

-Kees

-- 
Kees Cook

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

[Kent Overstreet]

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?

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

[Gustavo A. R. Silva]

On 9/14/23 13:38, Kent Overstreet wrote:
> 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?

Because zero-length arrays, when used as fake flexible arrays, make
things like -Warray-bounds (we've been trying to enable this compiler
option, globally) trip; among other things like being prone to result in
undefined behavior bugs when people introduce new members that make the
array end up in the middle of its containing structure.

With C99 flexible-array members, the compiler emits a warning when the
arrays are not at the end of the structure.

The DECLARE_FLEX_ARRAY() (in a union) helper allows for multiple C99
flexible-array members together at the end of a struct.

--
Gustavo

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

[Kees Cook]

On Thu, Sep 14, 2023 at 03:38:07PM -0400, Kent Overstreet wrote:
> On Wed, Sep 13, 2023 at 06:17:00PM -0700, Kees Cook wrote:
> > 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 C standard doesn't help us in that regard, that's true. But we've
been working to get it fixed. For example, there's discussion happening
next week at GNU Cauldron about flexible arrays in unions. It's already
possible, so better to just fix the standard -- real world code needs it
and uses it, as the bcachefs code illustrates. :)

> 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?

Because they're ambiguous and then the compiler can't do appropriate
bounds checking, compile-time diagnostics, etc. Maybe it's actually zero
sized, maybe it's not. Nothing stops them from being in the middle of
the structure so if someone accidentally tries to put members after it
(which has happened before), we end up with bizarre corruptions, etc,
etc. Flexible arrays are unambiguous, and that's why we committed to
converting all the fake flex arrays. The compiler does not have to guess
(or as has been the case: give up on) figuring out what was intended.

Regardless, I'm just trying to help make sure folks that run with
CONFIG_UBSAN_BOUNDS=y (as done in Android, Ubuntu, etc) will be able to
use bcachefs without runtime warnings, etc. Indexing through a 0-sized
array is going to trip the diagnostic either at runtime or when building
with -Warray-bounds.

-Kees

-- 
Kees Cook

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

[Kent Overstreet]

On Thu, Sep 14, 2023 at 05:20:41PM -0700, Kees Cook wrote:
> Because they're ambiguous and then the compiler can't do appropriate
> bounds checking, compile-time diagnostics, etc. Maybe it's actually zero
> sized, maybe it's not. Nothing stops them from being in the middle of
> the structure so if someone accidentally tries to put members after it
> (which has happened before), we end up with bizarre corruptions, etc,
> etc. Flexible arrays are unambiguous, and that's why we committed to
> converting all the fake flex arrays. The compiler does not have to guess
> (or as has been the case: give up on) figuring out what was intended.

So it does seem like we need to be able to distinguish between normal
flex arrays that go at the end of a struct vs. - what should we call
them, markers? that go in the middle.

> Regardless, I'm just trying to help make sure folks that run with
> CONFIG_UBSAN_BOUNDS=y (as done in Android, Ubuntu, etc) will be able to
> use bcachefs without runtime warnings, etc. Indexing through a 0-sized
> array is going to trip the diagnostic either at runtime or when building
> with -Warray-bounds.

I do have CONFIG_UBSAN_BOUNDS=y testing in my own CI, so all the runtime
errors should be fixed now (some of them with casts, but the casts are
in helpers that know what they're doing, not scattered around at
random).

So I think we're good for now - I'm going to hold off on more cleanup
for now unless reports of actual ubsan splats turn up, since I'm getting
a bit bombarded at the moment :)

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

[Kees Cook]

On Tue, Sep 19, 2023 at 05:23:18PM -0400, Kent Overstreet wrote:
> On Thu, Sep 14, 2023 at 05:20:41PM -0700, Kees Cook wrote:
> > Because they're ambiguous and then the compiler can't do appropriate
> > bounds checking, compile-time diagnostics, etc. Maybe it's actually zero
> > sized, maybe it's not. Nothing stops them from being in the middle of
> > the structure so if someone accidentally tries to put members after it
> > (which has happened before), we end up with bizarre corruptions, etc,
> > etc. Flexible arrays are unambiguous, and that's why we committed to
> > converting all the fake flex arrays. The compiler does not have to guess
> > (or as has been the case: give up on) figuring out what was intended.
> 
> So it does seem like we need to be able to distinguish between normal
> flex arrays that go at the end of a struct vs. - what should we call
> them, markers? that go in the middle.

As long as markers are just treated as address offsets in an struct, I
don't see a problem with them being 0-length arrays. I personally find
them confusing since whatever follows the marker is usually what I'm
trying to address, so the marker serves no purpose.

In the case of finding the offset to a subset of struct members, we
moved all of those in the kernel to using struct_group() instead. But
again, this was just for removing ambiguity for the compiler's ability
to enforce bounds checking (in this case on the memcpy()-family of
functions).

> 
> > Regardless, I'm just trying to help make sure folks that run with
> > CONFIG_UBSAN_BOUNDS=y (as done in Android, Ubuntu, etc) will be able to
> > use bcachefs without runtime warnings, etc. Indexing through a 0-sized
> > array is going to trip the diagnostic either at runtime or when building
> > with -Warray-bounds.
> 
> I do have CONFIG_UBSAN_BOUNDS=y testing in my own CI, so all the runtime
> errors should be fixed now (some of them with casts, but the casts are
> in helpers that know what they're doing, not scattered around at
> random).

Great! Thank you for chasing them all down. If you also have
CONFIG_FORTIFY_SOURCE=y then that should also be checking all the
strcpy()/memcpy() families too. The only thing that may be a problem in
the future is our effort to enable -Warray-bounds at build time. GCC
still has one false positive[1] remaining, but once that's fixed
(hopefully for GCC 14) the rest of the kernel is (was?) warning-free
(in our local testing where CONFIG_CC_NO_ARRAY_BOUNDS has been disabled).

> 
> So I think we're good for now - I'm going to hold off on more cleanup
> for now unless reports of actual ubsan splats turn up, since I'm getting
> a bit bombarded at the moment :)

Understood! :)

-Kees

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109071

-- 
Kees Cook