Re: [PATCH 1/5] ISDN-Gigaset: Use kmalloc_array() in two functions

From: SF Markus Elfring
Date: Wed Sep 28 2016 - 12:42:43 EST

>> * Multiplications for the size determination of memory allocations
>> indicated that array data structures should be processed.
>> Thus use the corresponding function "kmalloc_array".
> Was the current code incorrect?

I suggest to use a safer interface for array allocations.

> What makes kmalloc_array() better?

1. How do you think about the safety checks that this function provides?

2. Will you be also affected by further software evolution here?
mm: faster kmalloc_array(), kcalloc()

> I'm not going to change code just because some checker suggests to do so.

The script "" can point information out like the following.

WARNING: Prefer kmalloc_array over kmalloc with multiply

>> This issue was detected by using the Coccinelle software.
> So? And which coccinelle script was actually used?

How do you think about to look into related information sources?

Would you like to experiment any further with an excerpt?

expression count, target;
type T;
target =
- kmalloc(sizeof(T) * (count)
+ kmalloc_array(count, sizeof(T)
, ...);

expression count, pointer, target;
target =
- kmalloc(sizeof(*pointer) * (count)
+ kmalloc_array(count, sizeof(*pointer)
, ...);

> I couldn't spot a coccinelle script doing that in the current tree.

This is true for such a software update opportunity.

>> * Replace the specification of a data structure by a pointer dereference
>> to make the corresponding size determination a bit safer according to
>> the Linux coding style convention.
> I'm not happy with you mixing this with the above, less trivial, change.

I find that it is a useful combination. - A parameter is adjusted together
with a special function name.

>> - drv->cs = kmalloc(minors * sizeof *drv->cs, GFP_KERNEL);
>> + drv->cs = kmalloc_array(minors, sizeof(*drv->cs), GFP_KERNEL);
> For "minors" the same holds as for "channels", above.
> And you snuck in a parentheses change. That should have probably been
> merged with 5/5.

Would you prefer to add them in another update step?