Re: [PATCH] perf/x86/rapl: Prefer struct_size over open coded arithmetic

[Gustavo A. R. Silva]

On 18/03/24 17:40, Kees Cook wrote:
> On Sun, Mar 17, 2024 at 05:44:42PM +0100, Erick Archer wrote:
> > This is an effort to get rid of all multiplications from allocation
> > functions in order to prevent integer overflows [1][2].
> >
> > As the "rapl_pmus" variable is a pointer to "struct rapl_pmus" and
> > this structure ends in a flexible array:
> >
> > struct rapl_pmus {
> > 	[...]
> > 	struct rapl_pmu *pmus[] __counted_by(maxdie);
> > };
> >
> > the preferred way in the kernel is to use the struct_size() helper to
> > do the arithmetic instead of the calculation "size + count * size" in
> > the kzalloc() function.
> >
> > This way, the code is more readable and safer.
> >
> > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [1]
> > Link: https://github.com/KSPP/linux/issues/160 [2]
> > Signed-off-by: Erick Archer <erick.archer@xxxxxxx>
>
> Thanks!
>
> Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
>
> I was inspired to come up with a Coccinelle script to find this pattern.
> This seems to do it, though it also removes the blank line. I'm not sure
> how to stop it from doing that. I'm running this treewide to see if I
> can find others...
>
> // Options: --no-includes --include-headers

with --no-includes this one is missed in arch/x86/events/:

diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
index 258e2cdf28fa..213fe48f9391 100644
--- a/arch/x86/events/intel/uncore.c
+++ b/arch/x86/events/intel/uncore.c
@@ -350,12 +350,11 @@ static void uncore_pmu_init_hrtimer(struct intel_uncore_box *box)
 static struct intel_uncore_box *uncore_alloc_box(struct intel_uncore_type *type,
                                                 int node)
 {
-       int i, size, numshared = type->num_shared_regs ;
+       int i, numshared = type->num_shared_regs ;
        struct intel_uncore_box *box;

-       size = sizeof(*box) + numshared * sizeof(struct intel_uncore_extra_reg);
-
-       box = kzalloc_node(size, GFP_KERNEL, node);
+       box = kzalloc_node(struct_size(box, shared_regs, numshared),
+                          GFP_KERNEL, node);
        if (!box)
                return NULL;

Funny thing is that in this case it's quite convenient that the script removes
the blank like. :P

--
Gustavo