Received-SPF: pass (google.com: domain of linux-kernel+bounces-33955-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 22 Jan 2024 19:16:54 +0100
From: Erick Archer <erick.archer@gmx.com>
To: Dan Carpenter <dan.carpenter@linaro.org>
Cc: Erick Archer <erick.archer@gmx.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Franziska Naepelt <franziska.naepelt@googlemail.com>,
	Hans de Goede <hdegoede@redhat.com>,
	Johannes Berg <johannes.berg@intel.com>,
	Jeff Johnson <quic_jjohnson@quicinc.com>,
	Aloka Dixit <quic_alokad@quicinc.com>,
	"Gustavo A. R. Silva" <gustavoars@kernel.org>,
	linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org,
	linux-hardening@vger.kernel.org
Subject: Re: [PATCH] staging: rtl8723bs: Use kcalloc() instead of kzalloc()
Message-ID: <20240122181654.GA2834@titan>
References: <20240119173900.11035-1-erick.archer@gmx.com>
 <a5d8a5f1-432d-46f0-84fe-7b5b22ff5f32@moroto.mountain>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <a5d8a5f1-432d-46f0-84fe-7b5b22ff5f32@moroto.mountain>
UI-OutboundReport: notjunk:1;M01:P0:UsolengSshc=;iH0T6UGqXMqk89ahnVw5eMaTrWO
 Yr8qNqI5zZBV9KGn7C30KszUZthd3jRUXDgx/Yk0Ft28ZxlmF0LPJ9zZBPiKbjl/eFWnFSiTY
 pDf6DYxOYaDHVmeAy14wYthaFg0hmS5NoAXjaLwZkiiGf5gz0ynGqYPnSxaUTa2lNGa5AJrKF
 apOd5cRSM3aljlqXBK944RpUAkfkahXL5Fvt/NWnYuZxYCE0VcLpOZaxFjD/yQM05fI8iEhTr
 UieHEXvzpWM9PV/0eaEbfidyIMYCydIx4kXvyDtb5L8iK0H1oLCWp9CUpuNrXFq9O5BcSdI3y
 Z/WW+jYH0YWaDtJugFFkq3vl2x0boNky4ovdMh76SY5UB3rF5EDHxRt9Zji2aR28iO1d+iBJH
 jNnlP1TSn2RRopQ2HGvtKaza0/Js3b2kiOVx/+YudnwrUbO+sYjk5g3LB/H1uPVsbI2PrgLoK
 MIKhIiG9Qv8zAaKbrZYf1gToUMdEdQwA0v338Hosxx3mPvsPPKm7aQBkXKLCsNxnizttK8YDr
 O/gC5EHKGu8WVsU4P4IAMwpEZ/lJ+Wq7cGHFBC2tTBEICoaLMgSyBW51MDGi3GHUZ7X9G97ju
 LLZYQZwU74N5ZnLXwvuQ1CisU2dW3VjSSxjVGDwdIEjueT/EQ/MTtG5QTtNMEgDt67q34OXc2
 v98+a/M/cXjKk8ScPfLv3WWegKkcU5j/rovtyvQcf8vTr5qMRkmqAK0eyslxVAkmci/LwK5kx
 6LCHU9P3iewRwGdjj4U26yL4vI8WVY0JpQI0JyoAahF20whb3SygHxHog5IPO1Mjj0Q5z1NaW
 jgtongx7dlOXwWBc4ptHU8VvNGVF6RrKpAxKOdrVZiIhgIjcu0kWgDzfuv3AJ8H5JBe08OONb
 l9joLlKR9+0zPVs9FG/yGbMHstpBPPZa0s4J02U+3PHjp8kyxSTd4HGC3Ti+Ueo2ule/3mBvM
 8cVJZ9qfdGfWIhDO6DSux0ph4SA=
Content-Transfer-Encoding: quoted-printable

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 simila=
r)
> > function arguments due to the risk of them overflowing. This could lea=
d
> > 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 argumen=
t
> > 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 th=
e
> > former (unlike the latter).
> >
> > Link: https://www.kernel.org/doc/html/next/process/deprecated.html#ope=
n-coded-arithmetic-in-allocator-arguments [1]
> > Link: https://github.com/KSPP/linux/issues/162
> > Signed-off-by: Erick Archer <erick.archer@gmx.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
>