Make use of struct_size() helper instead of an open-coded calculation.
There is no functional change in this patch.
Link: https://github.com/KSPP/linux/issues/160
Signed-off-by: Xiu Jianfeng <[email protected]>
---
kernel/sched/fair.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 095b0aa378df..af933a7f9e5d 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -2437,9 +2437,8 @@ static void task_numa_group(struct task_struct *p, int cpupid, int flags,
int i;
if (unlikely(!deref_curr_numa_group(p))) {
- unsigned int size = sizeof(struct numa_group) +
- NR_NUMA_HINT_FAULT_STATS *
- nr_node_ids * sizeof(unsigned long);
+ unsigned int size = struct_size(grp, faults,
+ NR_NUMA_HINT_FAULT_STATS * nr_node_ids);
grp = kzalloc(size, GFP_KERNEL | __GFP_NOWARN);
if (!grp)
--
2.17.1
On Mon, 10 Jan 2022 09:23:54 +0800
Xiu Jianfeng <[email protected]> wrote:
> Make use of struct_size() helper instead of an open-coded calculation.
> There is no functional change in this patch.
Reviewed-by: Steven Rostedt <[email protected]>
-- Steve
>
> Link: https://github.com/KSPP/linux/issues/160
> Signed-off-by: Xiu Jianfeng <[email protected]>
> ---
> kernel/sched/fair.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 095b0aa378df..af933a7f9e5d 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -2437,9 +2437,8 @@ static void task_numa_group(struct task_struct *p, int cpupid, int flags,
> int i;
>
> if (unlikely(!deref_curr_numa_group(p))) {
> - unsigned int size = sizeof(struct numa_group) +
> - NR_NUMA_HINT_FAULT_STATS *
> - nr_node_ids * sizeof(unsigned long);
> + unsigned int size = struct_size(grp, faults,
> + NR_NUMA_HINT_FAULT_STATS * nr_node_ids);
>
> grp = kzalloc(size, GFP_KERNEL | __GFP_NOWARN);
> if (!grp)
On Mon, Jan 10, 2022 at 09:23:54AM +0800, Xiu Jianfeng wrote:
> Make use of struct_size() helper instead of an open-coded calculation.
> There is no functional change in this patch.
>
> Link: https://github.com/KSPP/linux/issues/160
> Signed-off-by: Xiu Jianfeng <[email protected]>
> ---
> kernel/sched/fair.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 095b0aa378df..af933a7f9e5d 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -2437,9 +2437,8 @@ static void task_numa_group(struct task_struct *p, int cpupid, int flags,
> int i;
>
> if (unlikely(!deref_curr_numa_group(p))) {
> - unsigned int size = sizeof(struct numa_group) +
> - NR_NUMA_HINT_FAULT_STATS *
> - nr_node_ids * sizeof(unsigned long);
> + unsigned int size = struct_size(grp, faults,
> + NR_NUMA_HINT_FAULT_STATS * nr_node_ids);
Again, why?! The old code was perfectly readable, this, not so much.
On Mon, 10 Jan 2022 23:46:15 +0100
Peter Zijlstra <[email protected]> wrote:
> On Mon, Jan 10, 2022 at 09:23:54AM +0800, Xiu Jianfeng wrote:
> > Make use of struct_size() helper instead of an open-coded calculation.
> > There is no functional change in this patch.
> >
> > Link: https://github.com/KSPP/linux/issues/160
> > Signed-off-by: Xiu Jianfeng <[email protected]>
> > ---
> > kernel/sched/fair.c | 5 ++---
> > 1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 095b0aa378df..af933a7f9e5d 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -2437,9 +2437,8 @@ static void task_numa_group(struct task_struct *p, int cpupid, int flags,
> > int i;
> >
> > if (unlikely(!deref_curr_numa_group(p))) {
> > - unsigned int size = sizeof(struct numa_group) +
> > - NR_NUMA_HINT_FAULT_STATS *
> > - nr_node_ids * sizeof(unsigned long);
> > + unsigned int size = struct_size(grp, faults,
> > + NR_NUMA_HINT_FAULT_STATS * nr_node_ids);
>
> Again, why?! The old code was perfectly readable, this, not so much.
Because it is unsafe, and there is an effort to get rid of all open coded
struct_size() code. Linus has told me to do the same with my code.
https://lore.kernel.org/all/CAHk-=wiGWjxs7EVUpccZEi6esvjpHJdgHQ=vtUeJ5crL62hx9A@mail.gmail.com/
And to be honest, the new change is a lot easier to read than the original
code.
struct_size() lets you know the field "faults" and the number of elements.
You don't need to know the size of "faults". Whereas the original code,
how is that readable? From that code, how do you know what the
sizeof(unsigned long) is for?
-- Steve
On Mon, Jan 10, 2022 at 07:31:58PM -0500, Steven Rostedt wrote:
> On Mon, 10 Jan 2022 23:46:15 +0100
> Peter Zijlstra <[email protected]> wrote:
>
> > On Mon, Jan 10, 2022 at 09:23:54AM +0800, Xiu Jianfeng wrote:
> > > Make use of struct_size() helper instead of an open-coded calculation.
> > > There is no functional change in this patch.
> > >
> > > Link: https://github.com/KSPP/linux/issues/160
> > > Signed-off-by: Xiu Jianfeng <[email protected]>
> > > ---
> > > kernel/sched/fair.c | 5 ++---
> > > 1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > > index 095b0aa378df..af933a7f9e5d 100644
> > > --- a/kernel/sched/fair.c
> > > +++ b/kernel/sched/fair.c
> > > @@ -2437,9 +2437,8 @@ static void task_numa_group(struct task_struct *p, int cpupid, int flags,
> > > int i;
> > >
> > > if (unlikely(!deref_curr_numa_group(p))) {
> > > - unsigned int size = sizeof(struct numa_group) +
> > > - NR_NUMA_HINT_FAULT_STATS *
> > > - nr_node_ids * sizeof(unsigned long);
> > > + unsigned int size = struct_size(grp, faults,
> > > + NR_NUMA_HINT_FAULT_STATS * nr_node_ids);
> >
> > Again, why?! The old code was perfectly readable, this, not so much.
>
> Because it is unsafe, and there is an effort to get rid of all open coded
> struct_size() code. Linus has told me to do the same with my code.
>
> https://lore.kernel.org/all/CAHk-=wiGWjxs7EVUpccZEi6esvjpHJdgHQ=vtUeJ5crL62hx9A@mail.gmail.com/
>
> And to be honest, the new change is a lot easier to read than the original
> code.
I agree.
Also, I was taking a look at the thread above and noticed the sparse
warning doesn't go away. However, the change is correct. :)
gustavo@beefy:~/git/linux$ grep 'using sizeof on a flexible structure' next-20220110.out | grep kernel/trace/trace.c
kernel/trace/trace.c:1009:17: warning: using sizeof on a flexible structure
kernel/trace/trace.c:2660:17: warning: using sizeof on a flexible structure
kernel/trace/trace.c:2770:51: warning: using sizeof on a flexible structure
kernel/trace/trace.c:3358:16: warning: using sizeof on a flexible structure
kernel/trace/trace.c:3418:16: warning: using sizeof on a flexible structure
kernel/trace/trace.c:7082:16: warning: using sizeof on a flexible structure
kernel/trace/trace.c:7160:16: warning: using sizeof on a flexible structure
gustavo@beefy:~/git/linux$ grep -nw struct_size kernel/trace/trace.c
2770: int max_len = PAGE_SIZE - struct_size(entry, array, 1);
Thanks
--
Gustavo
>
> struct_size() lets you know the field "faults" and the number of elements.
> You don't need to know the size of "faults". Whereas the original code,
> how is that readable? From that code, how do you know what the
> sizeof(unsigned long) is for?
>
> -- Steve
On Mon, Jan 10, 2022 at 07:31:58PM -0500, Steven Rostedt wrote:
> On Mon, 10 Jan 2022 23:46:15 +0100
> Peter Zijlstra <[email protected]> wrote:
>
> > On Mon, Jan 10, 2022 at 09:23:54AM +0800, Xiu Jianfeng wrote:
> > > Make use of struct_size() helper instead of an open-coded calculation.
> > > There is no functional change in this patch.
> > >
> > > Link: https://github.com/KSPP/linux/issues/160
> > > Signed-off-by: Xiu Jianfeng <[email protected]>
> > > ---
> > > kernel/sched/fair.c | 5 ++---
> > > 1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > > index 095b0aa378df..af933a7f9e5d 100644
> > > --- a/kernel/sched/fair.c
> > > +++ b/kernel/sched/fair.c
> > > @@ -2437,9 +2437,8 @@ static void task_numa_group(struct task_struct *p, int cpupid, int flags,
> > > int i;
> > >
> > > if (unlikely(!deref_curr_numa_group(p))) {
> > > - unsigned int size = sizeof(struct numa_group) +
> > > - NR_NUMA_HINT_FAULT_STATS *
> > > - nr_node_ids * sizeof(unsigned long);
> > > + unsigned int size = struct_size(grp, faults,
> > > + NR_NUMA_HINT_FAULT_STATS * nr_node_ids);
> >
> > Again, why?! The old code was perfectly readable, this, not so much.
>
> Because it is unsafe,
Unsafe how? Changelog doesn't mention anything, nor do you. In fact,
Changelog says there is no functional change, which makes me hate the
thing for obscuring something that was simple.
> And to be honest, the new change is a lot easier to read than the original
> code.
I find it the other way around, because now I need to find and untangle
the unholy mess that is struct_size(), whereas currently it is trivial
C.
On Tue, 11 Jan 2022 12:30:42 +0100
Peter Zijlstra <[email protected]> wrote:
> > > > if (unlikely(!deref_curr_numa_group(p))) {
> > > > - unsigned int size = sizeof(struct numa_group) +
> > > > - NR_NUMA_HINT_FAULT_STATS *
> > > > - nr_node_ids * sizeof(unsigned long);
> > > > + unsigned int size = struct_size(grp, faults,
> > > > + NR_NUMA_HINT_FAULT_STATS * nr_node_ids);
> > >
> > > Again, why?! The old code was perfectly readable, this, not so much.
> >
> > Because it is unsafe,
>
> Unsafe how? Changelog doesn't mention anything, nor do you. In fact,
> Changelog says there is no functional change, which makes me hate the
> thing for obscuring something that was simple.
If for some reason faults changes in size, the original code must be
updated whereas the new code is robust enough to not need changing.
>
> > And to be honest, the new change is a lot easier to read than the original
> > code.
>
> I find it the other way around, because now I need to find and untangle
> the unholy mess that is struct_size(), whereas currently it is trivial
> C.
It's a C hack and far from trivial. Maybe to you as you are use to
these hacks. But seriously, this is not something the average C coder
is use to, as variable length structures are rather unique to the
kernel.
Note that struct_size() is commonly used in the kernel. Better start
getting use to it ;-)
-- Steve
On Tue, Jan 11, 2022 at 10:14:25AM -0500, Steven Rostedt wrote:
> On Tue, 11 Jan 2022 12:30:42 +0100
> Peter Zijlstra <[email protected]> wrote:
>
> > > > > if (unlikely(!deref_curr_numa_group(p))) {
> > > > > - unsigned int size = sizeof(struct numa_group) +
> > > > > - NR_NUMA_HINT_FAULT_STATS *
> > > > > - nr_node_ids * sizeof(unsigned long);
> > > > > + unsigned int size = struct_size(grp, faults,
> > > > > + NR_NUMA_HINT_FAULT_STATS * nr_node_ids);
> > > >
> > > > Again, why?! The old code was perfectly readable, this, not so much.
> > >
> > > Because it is unsafe,
> >
> > Unsafe how? Changelog doesn't mention anything, nor do you. In fact,
> > Changelog says there is no functional change, which makes me hate the
> > thing for obscuring something that was simple.
>
> If for some reason faults changes in size, the original code must be
> updated whereas the new code is robust enough to not need changing.
Then I would still much prefer something like:
unsigned int size = sizeof(*grp) +
NR_NUMA_HINT_FAULT_STATS * numa_node_ids * sizeof(gfp->faults);
Which is still far more readable than some obscure macro. But again, the
Changelog doesn't mention any actual benefit of the patch and makes the
code less clear.
> It's a C hack and far from trivial. Maybe to you as you are use to
> these hacks. But seriously, this is not something the average C coder
> is use to, as variable length structures are rather unique to the
> kernel.
That's just not true, I've used them in userspace too (even before I
started tinkering with the kernel). I've even used this pattern in other
languages.
It is a fairly useful and common pattern to have a small structure and
an array in the same memory allocation.
Think hash-tables, the structure contains the size of the table and some
other things, like for example a seed for the hash function or a lock,
and then the table itself as an array.
I can't, nor do I want to, remember all these stupid little macros. Esp.
not for trivial things like this.
On Thu, Jan 13, 2022 at 10:18:57AM +0100, Peter Zijlstra wrote:
> On Tue, Jan 11, 2022 at 10:14:25AM -0500, Steven Rostedt wrote:
> > On Tue, 11 Jan 2022 12:30:42 +0100
> > Peter Zijlstra <[email protected]> wrote:
> >
> > > > > > if (unlikely(!deref_curr_numa_group(p))) {
> > > > > > - unsigned int size = sizeof(struct numa_group) +
> > > > > > - NR_NUMA_HINT_FAULT_STATS *
> > > > > > - nr_node_ids * sizeof(unsigned long);
> > > > > > + unsigned int size = struct_size(grp, faults,
> > > > > > + NR_NUMA_HINT_FAULT_STATS * nr_node_ids);
> > > > >
> > > > > Again, why?! The old code was perfectly readable, this, not so much.
> > > >
> > > > Because it is unsafe,
> > >
> > > Unsafe how? Changelog doesn't mention anything, nor do you. In fact,
> > > Changelog says there is no functional change, which makes me hate the
> > > thing for obscuring something that was simple.
> >
> > If for some reason faults changes in size, the original code must be
> > updated whereas the new code is robust enough to not need changing.
I think this alone is reason enough. :)
> Then I would still much prefer something like:
>
> unsigned int size = sizeof(*grp) +
> NR_NUMA_HINT_FAULT_STATS * numa_node_ids * sizeof(gfp->faults);
>
> Which is still far more readable than some obscure macro. But again, the
I'm not sure it's _obscure_, but it is relatively new. It's even
documented. ;)
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
That said, the original patch is incomplete: it should be using size_t
for "size".
> It is a fairly useful and common pattern to have a small structure and
> an array in the same memory allocation.
>
> Think hash-tables, the structure contains the size of the table and some
> other things, like for example a seed for the hash function or a lock,
> and then the table itself as an array.
Right, the use of flexible arrays is very common in the kernel. So much
so that we've spent years fixing all the ancient "fake flexible arrays"
scattered around the kernel messing up all kinds of compile-time and
run-time flaw mitigations. Flexible array manipulations are notoriously
prone to mistakes (overflows in allocation, mismatched bounds storage
sizes, array index overflows, etc). These helpers (with more to come)
help remove some of the foot-guns that C would normally impart to them.
> I can't, nor do I want to, remember all these stupid little macros. Esp.
> not for trivial things like this.
Well, the good news is that other folks will (and are) fixing them for
you. :) Even if you never make mistakes with flexible arrays, other
people do, and so we need to take on some improvements to the robustness
of the kernel source tree-wide.
-Kees
--
Kees Cook
?? 2022/1/15 11:50, Kees Cook ะด??:
> On Thu, Jan 13, 2022 at 10:18:57AM +0100, Peter Zijlstra wrote:
>> On Tue, Jan 11, 2022 at 10:14:25AM -0500, Steven Rostedt wrote:
>>> On Tue, 11 Jan 2022 12:30:42 +0100
>>> Peter Zijlstra <[email protected]> wrote:
>>>
>>>>>>> if (unlikely(!deref_curr_numa_group(p))) {
>>>>>>> - unsigned int size = sizeof(struct numa_group) +
>>>>>>> - NR_NUMA_HINT_FAULT_STATS *
>>>>>>> - nr_node_ids * sizeof(unsigned long);
>>>>>>> + unsigned int size = struct_size(grp, faults,
>>>>>>> + NR_NUMA_HINT_FAULT_STATS * nr_node_ids);
>>>>>> Again, why?! The old code was perfectly readable, this, not so much.
>>>>> Because it is unsafe,
>>>> Unsafe how? Changelog doesn't mention anything, nor do you. In fact,
>>>> Changelog says there is no functional change, which makes me hate the
>>>> thing for obscuring something that was simple.
>>> If for some reason faults changes in size, the original code must be
>>> updated whereas the new code is robust enough to not need changing.
> I think this alone is reason enough. :)
>
>> Then I would still much prefer something like:
>>
>> unsigned int size = sizeof(*grp) +
>> NR_NUMA_HINT_FAULT_STATS * numa_node_ids * sizeof(gfp->faults);
>>
>> Which is still far more readable than some obscure macro. But again, the
> I'm not sure it's _obscure_, but it is relatively new. It's even
> documented. ;)
> https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
>
> That said, the original patch is incomplete: it should be using size_t
> for "size".
thanks, I will send a v3 patch with this change and more detailed commit
message.
>> It is a fairly useful and common pattern to have a small structure and
>> an array in the same memory allocation.
>>
>> Think hash-tables, the structure contains the size of the table and some
>> other things, like for example a seed for the hash function or a lock,
>> and then the table itself as an array.
> Right, the use of flexible arrays is very common in the kernel. So much
> so that we've spent years fixing all the ancient "fake flexible arrays"
> scattered around the kernel messing up all kinds of compile-time and
> run-time flaw mitigations. Flexible array manipulations are notoriously
> prone to mistakes (overflows in allocation, mismatched bounds storage
> sizes, array index overflows, etc). These helpers (with more to come)
> help remove some of the foot-guns that C would normally impart to them.
>
>> I can't, nor do I want to, remember all these stupid little macros. Esp.
>> not for trivial things like this.
> Well, the good news is that other folks will (and are) fixing them for
> you. :) Even if you never make mistakes with flexible arrays, other
> people do, and so we need to take on some improvements to the robustness
> of the kernel source tree-wide.
>
> -Kees
>
On Fri, Jan 14, 2022 at 07:50:47PM -0800, Kees Cook wrote:
> On Thu, Jan 13, 2022 at 10:18:57AM +0100, Peter Zijlstra wrote:
> > Then I would still much prefer something like:
> >
> > unsigned int size = sizeof(*grp) +
> > NR_NUMA_HINT_FAULT_STATS * numa_node_ids * sizeof(gfp->faults);
> >
> > Which is still far more readable than some obscure macro. But again, the
>
> I'm not sure it's _obscure_, but it is relatively new. It's even
> documented. ;)
> https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
I'm one of those people who doesn't read documentation, I read code.
I also flat out refuse to read any documentation that isn't plain text.
> > I can't, nor do I want to, remember all these stupid little macros. Esp.
> > not for trivial things like this.
>
> Well, the good news is that other folks will (and are) fixing them for
> you. :) Even if you never make mistakes with flexible arrays, other
> people do, and so we need to take on some improvements to the robustness
> of the kernel source tree-wide.
But nobody helps me read the code when I trip over crap like this :/ Why
do we have to have endless silly helpers for things that can be
trivially expressed in regular C? I appreciate things like
container_of() because if you write that out it's a mess, but this, very
much not so.
struct_size(grp, faults, NR_NUMA_HINT_FAULTS_STATS * numa_node_ids);
vs
sizeof(*gfp) + sizeof(grp->faults) * NR_NUMA_HINT_FAULT_STATS * nr_node_ids;
The latter wins hands down, instantly obvious what it does while with
the former I'd have to look up the macro.
On Tue, Jan 18, 2022 at 09:57:45AM +0100, Peter Zijlstra wrote:
> On Fri, Jan 14, 2022 at 07:50:47PM -0800, Kees Cook wrote:
> > On Thu, Jan 13, 2022 at 10:18:57AM +0100, Peter Zijlstra wrote:
>
> > > Then I would still much prefer something like:
> > >
> > > unsigned int size = sizeof(*grp) +
> > > NR_NUMA_HINT_FAULT_STATS * numa_node_ids * sizeof(gfp->faults);
> > >
> > > Which is still far more readable than some obscure macro. But again, the
> >
> > I'm not sure it's _obscure_, but it is relatively new. It's even
> > documented. ;)
> > https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
>
> I'm one of those people who doesn't read documentation, I read code.
>
> I also flat out refuse to read any documentation that isn't plain text.
Sure, which is why it's in the tree:
Documentation/process/deprecated.rst
>
> > > I can't, nor do I want to, remember all these stupid little macros. Esp.
> > > not for trivial things like this.
> >
> > Well, the good news is that other folks will (and are) fixing them for
> > you. :) Even if you never make mistakes with flexible arrays, other
> > people do, and so we need to take on some improvements to the robustness
> > of the kernel source tree-wide.
>
> But nobody helps me read the code when I trip over crap like this :/ Why
> do we have to have endless silly helpers for things that can be
> trivially expressed in regular C? I appreciate things like
> container_of() because if you write that out it's a mess, but this, very
> much not so.
>
> struct_size(grp, faults, NR_NUMA_HINT_FAULTS_STATS * numa_node_ids);
>
> vs
>
> sizeof(*gfp) + sizeof(grp->faults) * NR_NUMA_HINT_FAULT_STATS * nr_node_ids;
>
> The latter wins hands down, instantly obvious what it does while with
> the former I'd have to look up the macro.
One of the drivers is general robustness. The open-coded version can
have bugs slowly drift in, especially with the sizeof() ends up naming
a structs like:
sizeof(struct object) + sizeof(struct element) * NR_NUMA_HINT_FAULT_STATS * nr_node_ids;
One of the points of struct_size() is to include the semantic sanity
checking that the compiler can do (i.e. making sure "faults" is a member
of "grp", correctly sizing them both, avoiding overflows, etc, etc).
I know what you mean about not liking looking up new macros, but given
C's fragility in these areas, it's been important for us to swap stuff
out shift the burdens to the compiler as much as possible.
-Kees
--
Kees Cook