Messages in this thread | | | Subject | Re: [PATCH] x86/uv/time: Replace one-element array and save heap space | From | Joe Perches <> | Date | Wed, 20 May 2020 12:01:24 -0700 |
| |
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
| |