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

[Kees Cook <keescook@chromium.org>]

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