Re: [PATCH] x86/uv/time: Replace one-element array and save heap space

[Joe Perches <joe@perches.com>]

On Wed, 2020-05-20 at 10:19 -0700, Kees Cook wrote:
> On Mon, May 18, 2020 at 12:09:16PM -0700, Joe Perches wrote:
> > On Mon, 2020-05-18 at 14:01 -0500, Gustavo A. R. Silva wrote:
> > > The current codebase makes use of one-element arrays in the following
> > > form:
> > > 
> > > struct something {
> > >     int length;
> > >     u8 data[1];
> > > };
> > []
> > > This issue has been out there since 2009.
> > > This issue was found with the help of Coccinelle and fixed _manually_.
> > []
> > > diff --git a/arch/x86/platform/uv/uv_time.c b/arch/x86/platform/uv/uv_time.c
[]
> > > @@ -156,9 +156,8 @@ static __init int uv_rtc_allocate_timers(void)
> > >  		struct uv_rtc_timer_head *head = blade_info[bid];
> > >  
> > >  		if (!head) {
> > > -			head = kmalloc_node(sizeof(struct uv_rtc_timer_head) +
> > > -				(uv_blade_nr_possible_cpus(bid) *
> > > -					2 * sizeof(u64)),
> > > +			head = kmalloc_node(struct_size(head, cpu,
> > > +				uv_blade_nr_possible_cpus(bid)),
> > 
> > It's probably safer to use kzalloc_node here as well.
> 
> Hm, I think it's not actually needed here.

Right. Turns out it's not needed.

> All three members are
> immediately initialized and it doesn't look to ever be copied to
> userspace.

It's more that the reader doesn't have to lookup the
struct to know all the members are initialized.

It's also not a fast path so any extra time to zero
is not significant.

> > >  				GFP_KERNEL, nid);
> > >  			if (!head) {
> > >  				uv_rtc_deallocate_timers();
> 
> FWIW, I think this change is good as-is. Always nice to get back a
> little memory. ;)

Saving space, 0 to 4 bytes at a time,
depending on the heap sizes...

Using the struct_size is clearer though, so that's good too.

cheers, Joe