* [PATCH] x86/uv/time: Replace one-element array and save heap space
@ 2020-05-18 19:01 Gustavo A. R. Silva
2020-05-18 19:09 ` Joe Perches
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Gustavo A. R. Silva @ 2020-05-18 19:01 UTC (permalink / raw)
To: Darren Hart, Andy Shevchenko, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, x86@kernel.org H. Peter Anvin
Cc: platform-driver-x86, linux-kernel, Gustavo A. R. Silva, Kees Cook
The current codebase makes use of one-element arrays in the following
form:
struct something {
int length;
u8 data[1];
};
struct something *instance;
instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL);
instance->length = size;
memcpy(instance->data, source, size);
but the preferred mechanism to declare variable-length types such as
these ones is a flexible array member[1][2], introduced in C99:
struct foo {
int stuff;
struct boo array[];
};
By making use of the mechanism above, we will get a compiler warning
in case the flexible array does not occur last in the structure, which
will help us prevent some kind of undefined behavior bugs from being
inadvertently introduced[3] to the codebase from now on. So, replace
the one-element array with a flexible-array member.
Also, make use of the new struct_size() helper to properly calculate the
total size needed to allocate dynamic memory for struct uv_rtc_timer_head.
Notice that, due to the use of a one-element array, space for an extra
struct cpu:
struct {
int lcpu; /* systemwide logical cpu number */
u64 expires; /* next timer expiration for this cpu */
} cpu[1]
was being allocated at the moment of applying the sizeof operator to
struct uv_rtc_timer_head in the call to kmalloc_node() at line 159:
159 head = kmalloc_node(sizeof(struct uv_rtc_timer_head) +
160 (uv_blade_nr_possible_cpus(bid) *
161 2 * sizeof(u64)),
162 GFP_KERNEL, nid);
but that extra cpu[] was never actually being accessed due to the
following piece of code at line 168:
168 head->ncpus = uv_blade_nr_possible_cpus(bid);
and the piece of code at line 187:
187 for (c = 0; c < head->ncpus; c++) {
188 u64 exp = head->cpu[c].expires;
189 if (exp < lowest) {
190 bcpu = c;
191 lowest = exp;
192 }
193 }
so heap space was being wasted.
Another thing important to notice is that through the use of the
struct_size() helper, code at line 161:
161 2 * sizeof(u64)),
is changed to now be the actual size of struct cpu; see
sizeof(*(p)->member) at include/linux/overflow.h:314:
314 #define struct_size(p, member, n) \
315 __ab_c_size(n, \
316 sizeof(*(p)->member) + __must_be_array((p)->member),\
317 sizeof(*(p)))
As a side note, the original developer could have implemented code at line
161: 2 * sizeof(64) as follows:
sizeof(*head->cpu)
This issue has been out there since 2009.
This issue was found with the help of Coccinelle and fixed _manually_.
[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
arch/x86/platform/uv/uv_time.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/arch/x86/platform/uv/uv_time.c b/arch/x86/platform/uv/uv_time.c
index 7af31b245636..993a8ae6fdfb 100644
--- a/arch/x86/platform/uv/uv_time.c
+++ b/arch/x86/platform/uv/uv_time.c
@@ -52,7 +52,7 @@ struct uv_rtc_timer_head {
struct {
int lcpu; /* systemwide logical cpu number */
u64 expires; /* next timer expiration for this cpu */
- } cpu[1];
+ } cpu[];
};
/*
@@ -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)),
GFP_KERNEL, nid);
if (!head) {
uv_rtc_deallocate_timers();
--
2.26.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] x86/uv/time: Replace one-element array and save heap space
2020-05-18 19:01 [PATCH] x86/uv/time: Replace one-element array and save heap space Gustavo A. R. Silva
@ 2020-05-18 19:09 ` Joe Perches
2020-05-18 22:32 ` Gustavo A. R. Silva
2020-05-20 17:19 ` Kees Cook
2020-05-21 23:24 ` Gustavo A. R. Silva
2020-09-30 6:52 ` Thomas Gleixner
2 siblings, 2 replies; 8+ messages in thread
From: Joe Perches @ 2020-05-18 19:09 UTC (permalink / raw)
To: Gustavo A. R. Silva, Darren Hart, Andy Shevchenko,
Thomas Gleixner, Ingo Molnar, Borislav Petkov,
x86@kernel.org H. Peter Anvin
Cc: platform-driver-x86, linux-kernel, Gustavo A. R. Silva, Kees Cook
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
> index 7af31b245636..993a8ae6fdfb 100644
> --- a/arch/x86/platform/uv/uv_time.c
> +++ b/arch/x86/platform/uv/uv_time.c
> @@ -52,7 +52,7 @@ struct uv_rtc_timer_head {
> struct {
> int lcpu; /* systemwide logical cpu number */
> u64 expires; /* next timer expiration for this cpu */
> - } cpu[1];
> + } cpu[];
> };
>
> /*
> @@ -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.
> GFP_KERNEL, nid);
> if (!head) {
> uv_rtc_deallocate_timers();
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86/uv/time: Replace one-element array and save heap space
2020-05-18 19:09 ` Joe Perches
@ 2020-05-18 22:32 ` Gustavo A. R. Silva
2020-05-20 17:19 ` Kees Cook
1 sibling, 0 replies; 8+ messages in thread
From: Gustavo A. R. Silva @ 2020-05-18 22:32 UTC (permalink / raw)
To: Joe Perches
Cc: Darren Hart, Andy Shevchenko, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, x86@kernel.org H. Peter Anvin,
platform-driver-x86, linux-kernel, Gustavo A. R. Silva,
Kees Cook
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
> > index 7af31b245636..993a8ae6fdfb 100644
> > --- a/arch/x86/platform/uv/uv_time.c
> > +++ b/arch/x86/platform/uv/uv_time.c
> > @@ -52,7 +52,7 @@ struct uv_rtc_timer_head {
> > struct {
> > int lcpu; /* systemwide logical cpu number */
> > u64 expires; /* next timer expiration for this cpu */
> > - } cpu[1];
> > + } cpu[];
> > };
> >
> > /*
> > @@ -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.
>
Thanks for your feedback, Joe. I'll wait for comments from the
maintainers on this and the rest of the patch.
Thanks
--
Gustavo
> > GFP_KERNEL, nid);
> > if (!head) {
> > uv_rtc_deallocate_timers();
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86/uv/time: Replace one-element array and save heap space
2020-05-18 19:09 ` Joe Perches
2020-05-18 22:32 ` Gustavo A. R. Silva
@ 2020-05-20 17:19 ` Kees Cook
2020-05-20 19:01 ` Joe Perches
1 sibling, 1 reply; 8+ messages in thread
From: Kees Cook @ 2020-05-20 17:19 UTC (permalink / raw)
To: Joe Perches
Cc: Gustavo A. R. Silva, Darren Hart, Andy Shevchenko,
Thomas Gleixner, Ingo Molnar, Borislav Petkov,
x86@kernel.org H. Peter Anvin, platform-driver-x86, linux-kernel,
Gustavo A. R. Silva
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
> > index 7af31b245636..993a8ae6fdfb 100644
> > --- a/arch/x86/platform/uv/uv_time.c
> > +++ b/arch/x86/platform/uv/uv_time.c
> > @@ -52,7 +52,7 @@ struct uv_rtc_timer_head {
> > struct {
> > int lcpu; /* systemwide logical cpu number */
> > u64 expires; /* next timer expiration for this cpu */
> > - } cpu[1];
> > + } cpu[];
> > };
> >
> > /*
> > @@ -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. All three members are
immediately initialized and it doesn't look to ever be copied to
userspace.
>
> > 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. ;)
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86/uv/time: Replace one-element array and save heap space
2020-05-20 17:19 ` Kees Cook
@ 2020-05-20 19:01 ` Joe Perches
0 siblings, 0 replies; 8+ messages in thread
From: Joe Perches @ 2020-05-20 19:01 UTC (permalink / raw)
To: Kees Cook
Cc: Gustavo A. R. Silva, Darren Hart, Andy Shevchenko,
Thomas Gleixner, Ingo Molnar, Borislav Petkov,
x86@kernel.org H. Peter Anvin, platform-driver-x86, linux-kernel,
Gustavo A. R. Silva
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86/uv/time: Replace one-element array and save heap space
2020-05-18 19:01 [PATCH] x86/uv/time: Replace one-element array and save heap space Gustavo A. R. Silva
2020-05-18 19:09 ` Joe Perches
@ 2020-05-21 23:24 ` Gustavo A. R. Silva
2020-09-29 23:14 ` Gustavo A. R. Silva
2020-09-30 6:52 ` Thomas Gleixner
2 siblings, 1 reply; 8+ messages in thread
From: Gustavo A. R. Silva @ 2020-05-21 23:24 UTC (permalink / raw)
To: Darren Hart, Andy Shevchenko, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, x86@kernel.org H. Peter Anvin, John Stultz
Cc: platform-driver-x86, linux-kernel, Gustavo A. R. Silva, Kees Cook
[+CC John Stultz <john.stultz@linaro.org> and +Kees' Reviewed-by tag]
Reviewed-by: Kees Cook <keescook@chromium.org>
On Mon, May 18, 2020 at 02:01:14PM -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];
> };
>
> struct something *instance;
>
> instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL);
> instance->length = size;
> memcpy(instance->data, source, size);
>
> but the preferred mechanism to declare variable-length types such as
> these ones is a flexible array member[1][2], introduced in C99:
>
> struct foo {
> int stuff;
> struct boo array[];
> };
>
> By making use of the mechanism above, we will get a compiler warning
> in case the flexible array does not occur last in the structure, which
> will help us prevent some kind of undefined behavior bugs from being
> inadvertently introduced[3] to the codebase from now on. So, replace
> the one-element array with a flexible-array member.
>
> Also, make use of the new struct_size() helper to properly calculate the
> total size needed to allocate dynamic memory for struct uv_rtc_timer_head.
> Notice that, due to the use of a one-element array, space for an extra
> struct cpu:
>
> struct {
> int lcpu; /* systemwide logical cpu number */
> u64 expires; /* next timer expiration for this cpu */
> } cpu[1]
>
> was being allocated at the moment of applying the sizeof operator to
> struct uv_rtc_timer_head in the call to kmalloc_node() at line 159:
>
> 159 head = kmalloc_node(sizeof(struct uv_rtc_timer_head) +
> 160 (uv_blade_nr_possible_cpus(bid) *
> 161 2 * sizeof(u64)),
> 162 GFP_KERNEL, nid);
>
> but that extra cpu[] was never actually being accessed due to the
> following piece of code at line 168:
>
> 168 head->ncpus = uv_blade_nr_possible_cpus(bid);
>
> and the piece of code at line 187:
>
> 187 for (c = 0; c < head->ncpus; c++) {
> 188 u64 exp = head->cpu[c].expires;
> 189 if (exp < lowest) {
> 190 bcpu = c;
> 191 lowest = exp;
> 192 }
> 193 }
>
> so heap space was being wasted.
>
> Another thing important to notice is that through the use of the
> struct_size() helper, code at line 161:
>
> 161 2 * sizeof(u64)),
>
> is changed to now be the actual size of struct cpu; see
> sizeof(*(p)->member) at include/linux/overflow.h:314:
>
> 314 #define struct_size(p, member, n) \
> 315 __ab_c_size(n, \
> 316 sizeof(*(p)->member) + __must_be_array((p)->member),\
> 317 sizeof(*(p)))
>
> As a side note, the original developer could have implemented code at line
> 161: 2 * sizeof(64) as follows:
>
> sizeof(*head->cpu)
>
> This issue has been out there since 2009.
>
> This issue was found with the help of Coccinelle and fixed _manually_.
>
> [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> [2] https://github.com/KSPP/linux/issues/21
> [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
>
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
> arch/x86/platform/uv/uv_time.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/platform/uv/uv_time.c b/arch/x86/platform/uv/uv_time.c
> index 7af31b245636..993a8ae6fdfb 100644
> --- a/arch/x86/platform/uv/uv_time.c
> +++ b/arch/x86/platform/uv/uv_time.c
> @@ -52,7 +52,7 @@ struct uv_rtc_timer_head {
> struct {
> int lcpu; /* systemwide logical cpu number */
> u64 expires; /* next timer expiration for this cpu */
> - } cpu[1];
> + } cpu[];
> };
>
> /*
> @@ -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)),
> GFP_KERNEL, nid);
> if (!head) {
> uv_rtc_deallocate_timers();
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86/uv/time: Replace one-element array and save heap space
2020-05-21 23:24 ` Gustavo A. R. Silva
@ 2020-09-29 23:14 ` Gustavo A. R. Silva
0 siblings, 0 replies; 8+ messages in thread
From: Gustavo A. R. Silva @ 2020-09-29 23:14 UTC (permalink / raw)
To: Gustavo A. R. Silva, Darren Hart, Andy Shevchenko,
Thomas Gleixner, Ingo Molnar, Borislav Petkov,
x86@kernel.org H. Peter Anvin, John Stultz
Cc: platform-driver-x86, linux-kernel, Kees Cook, linux-hardening
Hi all,
Who can take this?
Thanks
--
Gustavo
On 5/21/20 18:24, Gustavo A. R. Silva wrote:
> [+CC John Stultz <john.stultz@linaro.org> and +Kees' Reviewed-by tag]
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> On Mon, May 18, 2020 at 02:01:14PM -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];
>> };
>>
>> struct something *instance;
>>
>> instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL);
>> instance->length = size;
>> memcpy(instance->data, source, size);
>>
>> but the preferred mechanism to declare variable-length types such as
>> these ones is a flexible array member[1][2], introduced in C99:
>>
>> struct foo {
>> int stuff;
>> struct boo array[];
>> };
>>
>> By making use of the mechanism above, we will get a compiler warning
>> in case the flexible array does not occur last in the structure, which
>> will help us prevent some kind of undefined behavior bugs from being
>> inadvertently introduced[3] to the codebase from now on. So, replace
>> the one-element array with a flexible-array member.
>>
>> Also, make use of the new struct_size() helper to properly calculate the
>> total size needed to allocate dynamic memory for struct uv_rtc_timer_head.
>> Notice that, due to the use of a one-element array, space for an extra
>> struct cpu:
>>
>> struct {
>> int lcpu; /* systemwide logical cpu number */
>> u64 expires; /* next timer expiration for this cpu */
>> } cpu[1]
>>
>> was being allocated at the moment of applying the sizeof operator to
>> struct uv_rtc_timer_head in the call to kmalloc_node() at line 159:
>>
>> 159 head = kmalloc_node(sizeof(struct uv_rtc_timer_head) +
>> 160 (uv_blade_nr_possible_cpus(bid) *
>> 161 2 * sizeof(u64)),
>> 162 GFP_KERNEL, nid);
>>
>> but that extra cpu[] was never actually being accessed due to the
>> following piece of code at line 168:
>>
>> 168 head->ncpus = uv_blade_nr_possible_cpus(bid);
>>
>> and the piece of code at line 187:
>>
>> 187 for (c = 0; c < head->ncpus; c++) {
>> 188 u64 exp = head->cpu[c].expires;
>> 189 if (exp < lowest) {
>> 190 bcpu = c;
>> 191 lowest = exp;
>> 192 }
>> 193 }
>>
>> so heap space was being wasted.
>>
>> Another thing important to notice is that through the use of the
>> struct_size() helper, code at line 161:
>>
>> 161 2 * sizeof(u64)),
>>
>> is changed to now be the actual size of struct cpu; see
>> sizeof(*(p)->member) at include/linux/overflow.h:314:
>>
>> 314 #define struct_size(p, member, n) \
>> 315 __ab_c_size(n, \
>> 316 sizeof(*(p)->member) + __must_be_array((p)->member),\
>> 317 sizeof(*(p)))
>>
>> As a side note, the original developer could have implemented code at line
>> 161: 2 * sizeof(64) as follows:
>>
>> sizeof(*head->cpu)
>>
>> This issue has been out there since 2009.
>>
>> This issue was found with the help of Coccinelle and fixed _manually_.
>>
>> [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
>> [2] https://github.com/KSPP/linux/issues/21
>> [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
>>
>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>> ---
>> arch/x86/platform/uv/uv_time.c | 7 +++----
>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/platform/uv/uv_time.c b/arch/x86/platform/uv/uv_time.c
>> index 7af31b245636..993a8ae6fdfb 100644
>> --- a/arch/x86/platform/uv/uv_time.c
>> +++ b/arch/x86/platform/uv/uv_time.c
>> @@ -52,7 +52,7 @@ struct uv_rtc_timer_head {
>> struct {
>> int lcpu; /* systemwide logical cpu number */
>> u64 expires; /* next timer expiration for this cpu */
>> - } cpu[1];
>> + } cpu[];
>> };
>>
>> /*
>> @@ -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)),
>> GFP_KERNEL, nid);
>> if (!head) {
>> uv_rtc_deallocate_timers();
>> --
>> 2.26.2
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86/uv/time: Replace one-element array and save heap space
2020-05-18 19:01 [PATCH] x86/uv/time: Replace one-element array and save heap space Gustavo A. R. Silva
2020-05-18 19:09 ` Joe Perches
2020-05-21 23:24 ` Gustavo A. R. Silva
@ 2020-09-30 6:52 ` Thomas Gleixner
2 siblings, 0 replies; 8+ messages in thread
From: Thomas Gleixner @ 2020-09-30 6:52 UTC (permalink / raw)
To: Gustavo A. R. Silva, Darren Hart, Andy Shevchenko, Ingo Molnar,
Borislav Petkov, x86@kernel.org H. Peter Anvin
Cc: platform-driver-x86, linux-kernel, Gustavo A. R. Silva, Kees Cook
On Mon, May 18 2020 at 14:01, 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];
> };
>
> struct something *instance;
>
> instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL);
> instance->length = size;
> memcpy(instance->data, source, size);
>
> but the preferred mechanism to declare variable-length types such as
> these ones is a flexible array member[1][2], introduced in C99:
>
> struct foo {
> int stuff;
> struct boo array[];
> };
>
> By making use of the mechanism above, we will get a compiler warning
> in case the flexible array does not occur last in the structure, which
> will help us prevent some kind of undefined behavior bugs from being
> inadvertently introduced[3] to the codebase from now on. So, replace
> the one-element array with a flexible-array member.
>
> Also, make use of the new struct_size() helper to properly calculate the
> total size needed to allocate dynamic memory for struct uv_rtc_timer_head.
> Notice that, due to the use of a one-element array, space for an extra
> struct cpu:
>
> struct {
> int lcpu; /* systemwide logical cpu number */
> u64 expires; /* next timer expiration for this cpu */
> } cpu[1]
>
> was being allocated at the moment of applying the sizeof operator to
> struct uv_rtc_timer_head in the call to kmalloc_node() at line 159:
>
> 159 head = kmalloc_node(sizeof(struct uv_rtc_timer_head) +
> 160 (uv_blade_nr_possible_cpus(bid) *
> 161 2 * sizeof(u64)),
> 162 GFP_KERNEL, nid);
>
> but that extra cpu[] was never actually being accessed due to the
> following piece of code at line 168:
>
> 168 head->ncpus = uv_blade_nr_possible_cpus(bid);
>
> and the piece of code at line 187:
>
> 187 for (c = 0; c < head->ncpus; c++) {
> 188 u64 exp = head->cpu[c].expires;
> 189 if (exp < lowest) {
> 190 bcpu = c;
> 191 lowest = exp;
> 192 }
> 193 }
>
> so heap space was being wasted.
>
> Another thing important to notice is that through the use of the
> struct_size() helper, code at line 161:
>
> 161 2 * sizeof(u64)),
>
> is changed to now be the actual size of struct cpu; see
> sizeof(*(p)->member) at include/linux/overflow.h:314:
>
> 314 #define struct_size(p, member, n) \
> 315 __ab_c_size(n, \
> 316 sizeof(*(p)->member) + __must_be_array((p)->member),\
> 317 sizeof(*(p)))
>
> As a side note, the original developer could have implemented code at line
> 161: 2 * sizeof(64) as follows:
>
> sizeof(*head->cpu)
This changelog is an unparseable pile of word salad wasting brain heap
space.
The gist is:
Variable sized arrays at the end of a struct should be defined as
unsized array foo[] not as foo[1].
struct uv_rtc_timer_head contains a sized array cpu[1].
Switch it to an unsized array and use the struct_size() helper to
calculate the allocation size.
Thanks,
tglx
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-09-30 6:53 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-18 19:01 [PATCH] x86/uv/time: Replace one-element array and save heap space Gustavo A. R. Silva
2020-05-18 19:09 ` Joe Perches
2020-05-18 22:32 ` Gustavo A. R. Silva
2020-05-20 17:19 ` Kees Cook
2020-05-20 19:01 ` Joe Perches
2020-05-21 23:24 ` Gustavo A. R. Silva
2020-09-29 23:14 ` Gustavo A. R. Silva
2020-09-30 6:52 ` Thomas Gleixner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).