On Fri, 2016-08-26 at 15:57 -0400, Julia Lawall wrote:
> On Fri, 26 Aug 2016, SF Markus Elfring wrote:
> > From: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
> > Date: Fri, 26 Aug 2016 18:32:53 +0200
> >
> > * A multiplication for the size determination of a memory allocation
> > indicated that an array data structure should be processed.
> > Thus use the corresponding function "kmalloc_array".
> >
> > This issue was detected by using the Coccinelle software.
> >
> > * Replace the specification of data structures by pointer dereferences
> > to make the corresponding size determination a bit safer according to
> > the Linux coding style convention.
> >
> > Signed-off-by: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
> > ---
> > arch/ia64/sn/kernel/irq.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/ia64/sn/kernel/irq.c b/arch/ia64/sn/kernel/irq.c
[]
> > @@ -474,12 +474,12 @@ void __init sn_irq_lh_init(void)
> > {
> > int i;
> >
> > - sn_irq_lh = kmalloc(sizeof(struct list_head *) * NR_IRQS, GFP_KERNEL);
> > + sn_irq_lh = kmalloc_array(NR_IRQS, sizeof(*sn_irq_lh), GFP_KERNEL);
> > if (!sn_irq_lh)
> > panic("SN PCI INIT: Failed to allocate memory for PCI init\n");
> >
> > for (i = 0; i < NR_IRQS; i++) {
> > - sn_irq_lh[i] = kmalloc(sizeof(struct list_head), GFP_KERNEL);
> > + sn_irq_lh[i] = kmalloc(*sn_irq_lh[i], GFP_KERNEL);
>
> Did a sizeof get lost here?
Yes, thanks Julia.
This is why adding the generating spatch code is always good.
And Markus, please always compile test your code using the
appropriate cross-compilers available here:
https://www.kernel.org/pub/tools/crosstool/
And btw: using sizeof(*pp[i]) or sizeof(**pp) is not always
clearer or better than using sizeof(type)
If you _really wanted to clear up this code and make it more
robust/better, it'd probably be nicer to convert the
struct list_head **sn_irq_lh to a single struct list_head *
and do a single
struct list_head *sn_irq_la = malloc_array(nr_irqs, sizeof(struct list_head);
instead of an malloc_array of the *sn_irq_lh and the
multiple individual struct list_head malloc entries
and change the INIT_LIST_HEAD and indexing code.
That would be less data space overall given the alignment
waste of the individual allocs.
It also appears that sn_irq_lh is extern in
arch/ia64/include/asm/sn/intr.h but could be made static
to arch/ia64/sn/kernel/irq.c.
But really, the conversion isn't worthwhile as NR_IRQS is
limited to much less than the maximum allocation / sizeof(ptr).
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
