| Message ID | AS8PR02MB7237F4D39BF6AA0FF40E91638B392@AS8PR02MB7237.eurprd02.prod.outlook.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf/x86/intel/uncore: Prefer struct_size over open coded arithmetic | expand |
On Sat, Mar 30, 2024 at 03:32:59PM +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 "box" variable is a pointer to "struct intel_uncore_box" and > this structure ends in a flexible array: > > struct intel_uncore_box { > [...] > struct intel_uncore_extra_reg shared_regs[]; > }; > > 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_node() function. > > This way, the code is more readable and safer. > > This code was detected with the help of Coccinelle, and audited and > modified manually. > > 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@outlook.com> Friendly ping. Any comments? Regards, Erick
On Sat, Mar 30, 2024 at 03:32:59PM +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 "box" variable is a pointer to "struct intel_uncore_box" and > this structure ends in a flexible array: > > struct intel_uncore_box { > [...] > struct intel_uncore_extra_reg shared_regs[]; > }; > > 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_node() function. > > This way, the code is more readable and safer. > > This code was detected with the help of Coccinelle, and audited and > modified manually. > > 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@outlook.com> > --- > Hi, > > The Coccinelle script used to detect this code pattern is the following: > > virtual report > > @rule1@ > type t1; > type t2; > identifier i0; > identifier i1; > identifier i2; > identifier ALLOC =~ "kmalloc|kzalloc|kmalloc_node|kzalloc_node|vmalloc|vzalloc|kvmalloc|kvzalloc"; > position p1; > @@ > > i0 = sizeof(t1) + sizeof(t2) * i1; > ... > i2 = ALLOC@p1(..., i0, ...); > > @script:python depends on report@ > p1 << rule1.p1; > @@ > > msg = "WARNING: verify allocation on line %s" % (p1[0].line) > coccilib.report.print_report(p1[0],msg) > > Regards, > Erick > --- > arch/x86/events/intel/uncore.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c > index 258e2cdf28fa..ce756d24c370 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; Thanks, yes, this looks correct to me. Reviewed-by: Kees Cook <keescook@chromium.org> Peter and Ingo, you seem to traditionally take these changes (via -tip)? Can you please pick this up?
On Mon, Apr 29, 2024 at 10:18:03AM -0700, Kees Cook wrote: > Peter and Ingo, you seem to traditionally take these changes (via -tip)? > Can you please pick this up? I have been explicitly not taking these things for perf and sched for a while now. As I wrote in that other mail, I detest struct_size(), it obfuscates code for no real benefit afaict.
Hi everyone, On Sat, Mar 30, 2024 at 03:32:59PM +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 "box" variable is a pointer to "struct intel_uncore_box" and > this structure ends in a flexible array: > > struct intel_uncore_box { > [...] > struct intel_uncore_extra_reg shared_regs[]; > }; > > 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_node() function. > > This way, the code is more readable and safer. > > This code was detected with the help of Coccinelle, and audited and > modified manually. > > 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@outlook.com> How could this patch be accepted? Thanks, Erick
diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c index 258e2cdf28fa..ce756d24c370 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;
This is an effort to get rid of all multiplications from allocation functions in order to prevent integer overflows [1][2]. As the "box" variable is a pointer to "struct intel_uncore_box" and this structure ends in a flexible array: struct intel_uncore_box { [...] struct intel_uncore_extra_reg shared_regs[]; }; 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_node() function. This way, the code is more readable and safer. This code was detected with the help of Coccinelle, and audited and modified manually. 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@outlook.com> --- Hi, The Coccinelle script used to detect this code pattern is the following: virtual report @rule1@ type t1; type t2; identifier i0; identifier i1; identifier i2; identifier ALLOC =~ "kmalloc|kzalloc|kmalloc_node|kzalloc_node|vmalloc|vzalloc|kvmalloc|kvzalloc"; position p1; @@ i0 = sizeof(t1) + sizeof(t2) * i1; ... i2 = ALLOC@p1(..., i0, ...); @script:python depends on report@ p1 << rule1.p1; @@ msg = "WARNING: verify allocation on line %s" % (p1[0].line) coccilib.report.print_report(p1[0],msg) Regards, Erick --- arch/x86/events/intel/uncore.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)