Re: [PATCH -next, v2] sched: Use struct_size() helper in task_numa_group()

[Kees Cook <keescook@chromium.org>]

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 <peterz@infradead.org> 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