Re: [RFC net-next 1/2] overflow: add DECLARE_FLEX() for on-stack allocs

From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Date: Fri, 4 Aug 2023 12:59:08 +0200

On 8/2/23 00:31, Kees Cook wrote:

quoted

On Tue, Aug 01, 2023 at 01:19:22PM +0200, Przemek Kitszel wrote:

quoted

Add DECLARE_FLEX() macro for on-stack allocations of structs with
flexible array member.

I like this idea!

One terminology nit: I think this should be called "DEFINE_...", since
it's a specific instantiation. Other macros in the kernel seem to confuse
this a lot, though. Yay naming.

Thanks, makes sense!

quoted

quoted

Using underlying array for on-stack storage lets us to declare known
on compile-time structures without kzalloc().

Hmpf, this appears to immediately trip over any (future) use of
__counted_by()[1] for these (since the counted-by member would be
initialized to zero), but I think I have a solution. (See below.)

quoted

Actual usage for ice driver is in next patch of the series.

Note that "struct" kw and "*" char is moved to the caller, to both:
have shorter macro name, and have more natural type specification
in the driver code (IOW not hiding an actual type of var).

Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
  include/linux/overflow.h | 14 ++++++++++++++
  1 file changed, 14 insertions(+)

diff --git a/include/linux/overflow.h b/include/linux/overflow.h
index f9b60313eaea..403b7ec120a2 100644
--- a/include/linux/overflow.h
+++ b/include/linux/overflow.h

@@ -309,4 +309,18 @@ static inline size_t __must_check

size_sub(size_t minuend, size_t subtrahend)
  #define struct_size_t(type, member, count)                    \
      struct_size((type *)NULL, member, count)
  +/**
+ * DECLARE_FLEX() - Declare an on-stack instance of structure with
trailing
+ * flexible array.
+ * @type: Pointer to structure type, including "struct" keyword and
"*" char.
+ * @name: Name for a (pointer) variable to create.
+ * @member: Name of the array member.
+ * @count: Number of elements in the array; must be compile-time const.
+ *
+ * Declare an instance of structure *@type with trailing flexible
array.
+ */
+#define DECLARE_FLEX(type, name, member, count)                    \
+    u8 name##_buf[struct_size((type)NULL, member, count)]
__aligned(8) = {};\
+    type name = (type)&name##_buf
+
  #endif /* __LINUX_OVERFLOW_H */

I was disappointed to discover that only global (static) initializers
would work for a flex array member. :(

i.e. this works:

struct foo {
     unsigned long flags;
     unsigned char count;

So bad that in the ice driver (perhaps others too), we have cases that
there is no counter or it has different meaning.
(potentially "complicated" meaning - ice' struct
ice_aqc_alloc_free_res_elem has "__le16 num_elems", so could not be used
verbatim, and it's not actual counter either :/ (I was fooled by such

Speaking of __le16 (we already discussed it 1:1): it's not a rare case
to define Endianness-sensitive structures with a flex array at the end,
so for those with __{be,le}* we could be adding __counted_by() attribute
only when the host Endianness matches the structure's to have at least
some coverage. By "some" I mean actually a lot when it comes to LE
structures, which usually is the case :)

assumption here [2]). Perhaps recent series by Olek [3] is also good
illustration of hard cases for __counted_by()

quoted

     int array[] __counted_by(count);
};

Thanks,
Olek