Thread (45 messages) 45 messages, 10 authors, 2016-09-28

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

From: SF Markus Elfring <hidden>
Date: 2016-09-28 16:42:16
Also in: kernel-janitors, lkml

quoted
* 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?
   2016-07-26
   mm: faster kmalloc_array(), kcalloc()
   https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=91c6a05f72a996bee5133e76374ab3ad7d3b9b72

I'm not going to change code just because some checker suggests to do so.
The script "checkpatch.pl" can point information out like the following.

WARNING: Prefer kmalloc_array over kmalloc with multiply

quoted
  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?
https://github.com/coccinelle/coccinelle/issues/81

Would you like to experiment any further with an excerpt?


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

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

quoted
* 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.

quoted
-	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?

Regards,
Markus
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help