Hi Dan, On Mon, Jan 22, 2024 at 09:55:11AM +0300, Dan Carpenter wrote: > On Fri, Jan 19, 2024 at 06:39:00PM +0100, Erick Archer wrote: > > As noted in the "Deprecated Interfaces, Language Features, Attributes, > > and Conventions" documentation [1], size calculations (especially > > multiplication) should not be performed in memory allocator (or similar) > > function arguments due to the risk of them overflowing. This could lead > > to values wrapping around and a smaller allocation being made than the > > caller was expecting. Using those allocations could lead to linear > > overflows of heap memory and other misbehaviors. > > > > So, use the purpose specific kcalloc() function instead of the argument > > count * size in the kzalloc() function. > > > > Also, it is preferred to use sizeof(*pointer) instead of sizeof(type) > > due to the type of the variable can change and one needs not change the > > former (unlike the latter). > > > > Link: https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [1] > > Link: https://github.com/KSPP/linux/issues/162 > > Signed-off-by: Erick Archer <erick.archer@xxxxxxx> > > I quite often write responses to patches and then never send them. I > wrote this response and debated sending it but in the end I decided to > send it because you have sent multiple patches. If you had only sent > one patch then I wouldn't have bothered. My intention is not to bother anyone. I'm a linux kernel developer newbie and I try to do my best. > Generally, commit messages should say what the user visible effects of > a patch are. Sometimes with these sorts of commits, it's hard to > determine the effect. For example, Kees went through and changed dozens > or hundreds of these allocations to use safer constructs and we don't > necessarily expect him to audit all the code. They should already have > been fine, but it's better to be safe. > > However in this case obviously the patch is small and just by glancing > at it we can see that it has no effect on rutime. > > But if someone is reviewing patches with "git log" instead of > "git log -p" they aren't going to see the patch. I can almost always > figure out what a commit does without looking at the commit message, > that doesn't mean that the commit messages are unnecessary. > > So I really prefer if commit message say, "This commit is just to make > static checkers happy and to make the code more readable. It has no > effect on runtime." The commit message you wrote is way more scary than > is warranted. Here is my proposed commit message: > > "We are trying to get rid of all multiplications from allocation > functions to prevent integer overflows. Here the multiplication is > obviously safe, but using kcalloc() is more appropriate and improves > readability. This patch has no effect on runtime behavior." > Understood. Thank you very much for your guidance and advices. I will change the commit message and I will send a more appropiate v2 patch. Best regards, Erick > regards, > dan carpenter >