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

[Johannes Berg <johannes@xxxxxxxxxxxxxxxx>]

On Wed, 2022-05-04 at 08:38 -0700, Kees Cook wrote:
> 
> It seemed like requiring a structure be rearranged to take advantage of
> the "automatic layout introspection" wasn't very friendly. On the other
> hand, looking at the examples, most of them are already neighboring
> members. Hmmm.

A lot of them are, and many could be, though not all.

> > 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).
> 
> The names aren't great, but I wanted to distinguish "elements" as the
> array not the count. Yay naming.

:-)

> However, perhaps the solution is to have _both_. i.e using
> BOUNDED_FLEX_ARRAY(count_type, count_name, array_type, array_name) for
> the "neighboring" case, and the DECLARE...{ELEMENTS,COUNT} for the
> "split" case.

Seems reasonable to me.

> And DECLARE_FLEX_ARRAY_ELEMENTS could actually be expanded to include
> the count_name too, so both methods could be "forward portable" to a
> future where C grew the syntax for bounded flex arrays.

I guess I don't see that happening :)

> > This seems rather awkward, having to set it to NULL, then checking rc
> > (and possibly needing a separate variable for it), etc.
> 
> I think the errno return is completely required. I had an earlier version
> of this that was much more like a drop-in replacement for memcpy that
> would just truncate or panic, 
> 

Oh, I didn't mean to imply it should truncate or panic or such - but if
it returns a pointer it can still be an ERR_PTR() or NULL instead of
having this separate indication, which even often confuses static type
checkers since they don't always see the "errno == 0 <=> ptr != NULL"
relation.

So not saying you shouldn't have any error return - clearly you need
that, just saying that I'm not sure that having the two separated is
great.


> Requiring instance to be NULL is debatable, but I feel pretty strongly
> about it because it does handle a class of mistakes (resource leaks),
> and it's not much of a burden to require a known-good starting state.

Yeah, dunno, I guess I'm slightly more on the side of not requiring it,
since we don't do the same for kmalloc() etc. and probably really
wouldn't want to add kmalloc_s() that does it ;-)

I mean, you _could_ go there:

int kmalloc_s(void **ptr, size_t size, gfp_t gfp)
{
  void *ret;

  if (*ptr)
    return -EINVAL;

  ret = kmalloc(size, gfp);
  if (!ret)
    return -ENOMEM;
  *ptr = ret;
  return 0;  
}

right? But we don't really do that, and I'm not sure it'd be a win if
done over the whole code base.

So I'm not really sure why this aspect here should need to be different,
except of course that you already need the input argument for the magic.

But we could still have (this prototype is theoretical, of course, it
cannot be implemented in C):

void *mem_to_flex_dup(void *ptr, const void *data, size_t elements,
                      gfp_t gfp);


which isn't really that much better though.

And btw, while I was writing it down I was looking to see if it should
be "size_t elements" or "size_t len" (like memcpy), it took me some time
to figure out, and I was looking at the examples:

 1) most of them actually use __u8 or some variant thereof, so you
    could probably add an even simpler macro like
       BOUNDED_FLEX_DATA(int, bytes, data)
    which has the u8 type internally.

 2) Unless I'm confusing myself, you got the firewire change wrong,
    because __mem_to_flex_dup takes the "elements_count", but the
    memcpy() there wasn't multiplied by the sizeof(element)? Or maybe
    the fact that it was declared as __u32 header[0] is wrong, and it
    should be __u8, but it's all very confusing, and I'm really not
    sure about this at all.



One "perhaps you'll laugh me out of the room" suggestion might be to
actually be able to initialize the whole thing too?


mydata = flex_struct_alloc(mydata, GFP_KERNEL,
                           variable_data, variable_len,
                           .member = 1,
                           .another = 2);

(the ordering can't really be otherwise since you have to use
__VA_ARGS__).

That might reduce some more code too, though I guess it's quite some
additional magic ... :)


> > 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".
> 
> Sure, and I have tried to get it down as small as possible. The earlier
> "just put all the member names in every call" version was horrid. :P

:-D

> I
> realize it's more work to check errno, but the memcpy() API we've all
> been trained to use is just plain dangerous. I don't think it's
> unreasonable to ask people to retrain themselves to avoid it. All that
> said, yes, I want it to be as friendly as possible.
> 
> > Maybe there should also be a realloc() version of it?
> 
> Sure! Seems reasonable. I'd like to see the code pattern for this
> though. Do you have any examples?

I was going to point to struct cfg80211_bss_ies, but I realize now
they're RCU-managed, so we never resize them anyway ... So maybe it's
less common than I thought it might be.

I suppose you know better since you converted a lot of stuff already :-)

johannes