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
