Date: Mon, 22 Jan 2024 19:16:54 +0100
From: Erick Archer <erick.archer@....com>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: Erick Archer <erick.archer@....com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Franziska Naepelt <franziska.naepelt@...glemail.com>,
	Hans de Goede <hdegoede@...hat.com>,
	Johannes Berg <johannes.berg@...el.com>,
	Jeff Johnson <quic_jjohnson@...cinc.com>,
	Aloka Dixit <quic_alokad@...cinc.com>,
	"Gustavo A. R. Silva" <gustavoars@...nel.org>,
	linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org,
	linux-hardening@...r.kernel.org
Subject: Re: [PATCH] staging: rtl8723bs: Use kcalloc() instead of kzalloc()

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

Powered by blists - more mailing lists