lkml.org 
[lkml]   [2021]   [Jul]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 04/64] stddef: Introduce struct_group() helper macro
    On Tue, Jul 27, 2021 at 01:57:55PM -0700, Kees Cook wrote:
    > Kernel code has a regular need to describe groups of members within a
    > structure usually when they need to be copied or initialized separately
    > from the rest of the surrounding structure. The generally accepted design
    > pattern in C is to use a named sub-struct:
    >
    > struct foo {
    > int one;
    > struct {
    > int two;
    > int three;
    > } thing;
    > int four;
    > };
    >
    > This would allow for traditional references and sizing:
    >
    > memcpy(&dst.thing, &src.thing, sizeof(dst.thing));
    >
    > However, doing this would mean that referencing struct members enclosed
    > by such named structs would always require including the sub-struct name
    > in identifiers:
    >
    > do_something(dst.thing.three);
    >
    > This has tended to be quite inflexible, especially when such groupings
    > need to be added to established code which causes huge naming churn.
    > Three workarounds exist in the kernel for this problem, and each have
    > other negative properties.
    >
    > To avoid the naming churn, there is a design pattern of adding macro
    > aliases for the named struct:
    >
    > #define f_three thing.three
    >
    > This ends up polluting the global namespace, and makes it difficult to
    > search for identifiers.
    >
    > Another common work-around in kernel code avoids the pollution by avoiding
    > the named struct entirely, instead identifying the group's boundaries using
    > either a pair of empty anonymous structs of a pair of zero-element arrays:
    >
    > struct foo {
    > int one;
    > struct { } start;
    > int two;
    > int three;
    > struct { } finish;
    > int four;
    > };
    >
    > struct foo {
    > int one;
    > int start[0];
    > int two;
    > int three;
    > int finish[0];
    > int four;
    > };
    >
    > This allows code to avoid needing to use a sub-struct name for member
    > references within the surrounding structure, but loses the benefits of
    > being able to actually use such a struct, making it rather fragile. Using
    > these requires open-coded calculation of sizes and offsets. The efforts
    > made to avoid common mistakes include lots of comments, or adding various
    > BUILD_BUG_ON()s. Such code is left with no way for the compiler to reason
    > about the boundaries (e.g. the "start" object looks like it's 0 bytes
    > in length and is not structurally associated with "finish"), making bounds
    > checking depend on open-coded calculations:
    >
    > if (length > offsetof(struct foo, finish) -
    > offsetof(struct foo, start))
    > return -EINVAL;
    > memcpy(&dst.start, &src.start, length);
    >
    > However, the vast majority of places in the kernel that operate on
    > groups of members do so without any identification of the grouping,
    > relying either on comments or implicit knowledge of the struct contents,
    > which is even harder for the compiler to reason about, and results in
    > even more fragile manual sizing, usually depending on member locations
    > outside of the region (e.g. to copy "two" and "three", use the start of
    > "four" to find the size):
    >
    > BUILD_BUG_ON((offsetof(struct foo, four) <
    > offsetof(struct foo, two)) ||
    > (offsetof(struct foo, four) <
    > offsetof(struct foo, three));
    > if (length > offsetof(struct foo, four) -
    > offsetof(struct foo, two))
    > return -EINVAL;
    > memcpy(&dst.two, &src.two, length);
    >
    > And both of the prior two idioms additionally appear to write beyond the
    > end of the referenced struct member, forcing the compiler to ignore any
    > attempt to perform bounds checking.
    >
    > In order to have a regular programmatic way to describe a struct
    > region that can be used for references and sizing, can be examined for
    > bounds checking, avoids forcing the use of intermediate identifiers,
    > and avoids polluting the global namespace, introduce the struct_group()
    > macro. This macro wraps the member declarations to create an anonymous
    > union of an anonymous struct (no intermediate name) and a named struct
    > (for references and sizing):
    >
    > struct foo {
    > int one;
    > struct_group(thing,
    > int two,
    > int three,
    > );
    > int four;
    > };
    >
    > if (length > sizeof(src.thing))
    > return -EINVAL;
    > memcpy(&dst.thing, &src.thing, length);
    > do_something(dst.three);
    >
    > There are some rare cases where the resulting struct_group() needs
    > attributes added, so struct_group_attr() is also introduced to allow
    > for specifying struct attributes (e.g. __align(x) or __packed).
    >
    > Co-developed-by: Keith Packard <keithpac@amazon.com>
    > Signed-off-by: Keith Packard <keithpac@amazon.com>
    > Signed-off-by: Kees Cook <keescook@chromium.org>

    Acked-by: Gustavo A. R. Silva <gustavoars@kernel.org>

    Love it! :)

    Thanks
    --
    Gustavo

    > ---
    > include/linux/stddef.h | 34 ++++++++++++++++++++++++++++++++++
    > 1 file changed, 34 insertions(+)
    >
    > diff --git a/include/linux/stddef.h b/include/linux/stddef.h
    > index 998a4ba28eba..cf7f866944f9 100644
    > --- a/include/linux/stddef.h
    > +++ b/include/linux/stddef.h
    > @@ -36,4 +36,38 @@ enum {
    > #define offsetofend(TYPE, MEMBER) \
    > (offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER))
    >
    > +/**
    > + * struct_group_attr(NAME, ATTRS, MEMBERS)
    > + *
    > + * Used to create an anonymous union of two structs with identical
    > + * layout and size: one anonymous and one named. The former can be
    > + * used normally without sub-struct naming, and the latter can be
    > + * used to reason about the start, end, and size of the group of
    > + * struct members. Includes structure attributes argument.
    > + *
    > + * @NAME: The name of the mirrored sub-struct
    > + * @ATTRS: Any struct attributes (normally empty)
    > + * @MEMBERS: The member declarations for the mirrored structs
    > + */
    > +#define struct_group_attr(NAME, ATTRS, MEMBERS) \
    > + union { \
    > + struct { MEMBERS } ATTRS; \
    > + struct { MEMBERS } ATTRS NAME; \
    > + }
    > +
    > +/**
    > + * struct_group(NAME, MEMBERS)
    > + *
    > + * Used to create an anonymous union of two structs with identical
    > + * layout and size: one anonymous and one named. The former can be
    > + * used normally without sub-struct naming, and the latter can be
    > + * used to reason about the start, end, and size of the group of
    > + * struct members.
    > + *
    > + * @NAME: The name of the mirrored sub-struct
    > + * @MEMBERS: The member declarations for the mirrored structs
    > + */
    > +#define struct_group(NAME, MEMBERS) \
    > + struct_group_attr(NAME, /* no attrs */, MEMBERS)
    > +
    > #endif
    > --
    > 2.30.2
    >

    \
     
     \ /
      Last update: 2021-07-28 04:30    [W:4.101 / U:0.072 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site