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

Johannes Berg
2022-05-04T07:28:53Z

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