Re: [PATCH] platform/x86: thinkpad_acpi: Prefer struct_size over open coded arithmetic

[Len Baker <len.baker@gmx.com>]

Hi,

On Tue, Sep 21, 2021 at 03:46:23PM +0200, Hans de Goede wrote:
> On 9/20/21 7:58 AM, Kees Cook wrote:
> > On Sat, Sep 18, 2021 at 05:05:00PM +0200, Len Baker wrote:
> >>
> >>  static struct attribute_set *create_attr_set(unsigned int max_members,
> >> @@ -1020,13 +1020,11 @@ static struct attribute_set *create_attr_set(unsigned int max_members,
> >>  		return NULL;
> >>
> >>  	/* Allocates space for implicit NULL at the end too */
> >> -	sobj = kzalloc(sizeof(struct attribute_set_obj) +
> >> -		    max_members * sizeof(struct attribute *),
> >> -		    GFP_KERNEL);
> >> +	sobj = kzalloc(struct_size(sobj, a, max_members + 1), GFP_KERNEL);
> >
> > Whoa, this needs a lot more detail in the changelog if this is actually
> > correct. The original code doesn't seem to match the comment? (Where is
> > the +1?) So is this also a bug-fix?
>
> Kees, at first I thought you were spot-on with this comment, but the
> truth is more subtle. struct attribute_set_obj was:
>
> struct attribute_set_obj {
>         struct attribute_set s;
>         struct attribute *a;
> } __attribute__((packed));
>
> Another way of looking at this, which makes things more clear is as:
>
> struct attribute_set_obj {
>         struct attribute_set s;
>         struct attribute *a[1];
> } __attribute__((packed));
>
> So the sizeof(struct attribute_set_obj) in the original kzalloc call
> included room for 1 "extra" pointer which is reserved for the terminating
> NULL pointer.
>
> Changing the struct to:
>
> struct attribute_set_obj {
>         struct attribute_set s;
>         struct attribute *a[];
> } __attribute__((packed));
>
> Is equivalent to changing it to:
>
> struct attribute_set_obj {
>         struct attribute_set s;
>         struct attribute *a[0];
> } __attribute__((packed));
>
> So the change in the struct declaration reduces the sizeof(struct attribute_set_obj)
> by the size of 1 pointer, making the +1 necessary.
>
> So AFAICT there is actually no functional change here.

Hans, thanks for the explanation. Yes, this is the reason I added the "plus 1".
Not only based on the comment :)

Regards,
Len