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 for Android: free password hash cracker in your pocket

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

Date:   Sat, 27 Aug 2016 09:02:49 +0200
From:   SF Markus Elfring <elfring@...rs.sourceforge.net>
To:     Joe Perches <joe@...ches.com>
Cc:     Julia Lawall <julia.lawall@...6.fr>, 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()

>>> @@ -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.

Unfortunately, another copy mistake happened during a bit of
source code editing.

> This is why adding the generating spatch code is always good.

I find that this broken update suggestion can point a few details out
for further considerations.

I dared to combine some software aspects once more in this use case.
Such a combination (join point) shows interesting challenges,
doesn't it?

> And Markus, please always compile test your code using the
> appropriate cross-compilers available here:
> https://www.kernel.org/pub/tools/crosstool/

Thanks for your link.

> And btw: using sizeof(*pp[i]) or sizeof(**pp) is not always
> clearer or better than using sizeof(type)

Do you express a target conflict between your expectations
and the evolving Linux coding style documentation here?

Would any software developers insist to see the corresponding
data type directly instead of "evaluating" a pointer expression?

> 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 *
…
> That would be less data space overall given the alignment
> waste of the individual allocs.

Does this suggestion mean that I should drop my proposal
around the software components "IRQ" and "TLB" for the system
architecture "IA64" in such a questionable patch series?

Regards,
Markus