Re: [PATCH 02/32] Introduce flexible array struct memcpy() helpers

[Johannes Berg <johannes@xxxxxxxxxxxxxxxx>]

On Tue, 2022-05-03 at 18:44 -0700, Kees Cook wrote:
> 
> For example, using the most complicated helper, mem_to_flex_dup():
> 
>     /* Flexible array struct with members identified. */
>     struct something {
>         int mode;
>         DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(int, how_many);
>         unsigned long flags;
>         DECLARE_FLEX_ARRAY_ELEMENTS(u32, value);

In many cases, the order of the elements doesn't really matter, so maybe
it'd be nicer to be able to write it as something like

DECLARE_FLEX_STRUCT(something,
        int mode;
        unsigned long flags;
        ,
        int, how_many,
        u32, value);

perhaps? OK, that doesn't seem so nice either.

Maybe

struct something {
        int mode;
        unsigned long flags;
        FLEX_ARRAY(
                int, how_many,
                u32, value
        );
};

or so? The long and duplicated DECLARE_FLEX_ARRAY_ELEMENTS_COUNT and
DECLARE_FLEX_ARRAY_ELEMENTS seems a bit tedious to me, at least in cases
where the struct layout is not the most important thing (or it's already
at the end anyway).


>     struct something *instance = NULL;
>     int rc;
> 
>     rc = mem_to_flex_dup(&instance, byte_array, count, GFP_KERNEL);
>     if (rc)
>         return rc;

This seems rather awkward, having to set it to NULL, then checking rc
(and possibly needing a separate variable for it), etc.

But I can understand how you arrived at this:
 - need to pass instance or &instance or such for typeof()
   or offsetof() or such
 - instance = mem_to_flex_dup(instance, ...)
   looks too much like it would actually dup 'instance', rather than
   'byte_array'
 - if you pass &instance anyway, checking for NULL is simple and adds a
   bit of safety

but still, honestly, I don't like it. As APIs go, it feels a bit
cumbersome and awkward to use, and you really need everyone to use this,
and not say "uh what, I'll memcpy() instead".

Maybe there should also be a realloc() version of it?


> +/** __fas_bytes - Calculate potential size of flexible array structure

I think you forgot "\n *" in many cases here after "/**".

johannes