linux-kernel - Re: [PATCH 1/5] IA64-IRQ: Use kmalloc_array() in sn_irq_lh

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1472245341.4914.79.camel@perches.com>
Date:   Fri, 26 Aug 2016 14:02:21 -0700
From:   Joe Perches <joe@...ches.com>
To:     Julia Lawall <julia.lawall@...6.fr>,
        SF Markus Elfring <elfring@...rs.sourceforge.net>
Cc:     linux-ia64@...r.kernel.org, Fenghua Yu <fenghua.yu@...el.com>,
        Tony Luck <tony.luck@...el.com>,
        LKML <linux-kernel@...r.kernel.org>,
        kernel-janitors@...r.kernel.org,
        Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [PATCH 1/5] IA64-IRQ: Use kmalloc_array() in sn_irq_lh_init()

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@...rs.sourceforge.net>
> > 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@...rs.sourceforge.net>
> > ---
> >  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).