| Message ID | 20240119173900.11035-1-erick.archer@gmx.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 58dc02750fcca1b16b7b1f3d305ae34ebeafebfd |
| Headers | show |
| Series | staging: rtl8723bs: Use kcalloc() instead of kzalloc() | expand |
On 1/19/24 11:39, 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@gmx.com> Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org> Thanks!
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@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. 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." regards, dan carpenter
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@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 >
On Mon, Jan 22, 2024 at 07:16:54PM +0100, Erick Archer wrote: > 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@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. > Yeah. It's not a problem, we all started as newbies. I guess what I was trying to say is that if you're going to be sending a lot of patches then it's worth explaining this but if you're only sending one then I wouldn't bother. I don't really expect people to figure out all the code, it's just that if you can see the effect of a patch right away then, please, include that in the patch. Even when it's like in this case, "This patch should have no effect on runtime." That sort of information is still useful. Or if you can't figure out the implications that's also useful information to me as a reviewer. Just put those I don't know comments under the --- cut off. regards, dan carpenter
diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c index 1ff763c10064..65a450fcdce7 100644 --- a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c +++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c @@ -1259,8 +1259,7 @@ static int cfg80211_rtw_scan(struct wiphy *wiphy goto check_need_indicate_scan_done; } - ssid = kzalloc(RTW_SSID_SCAN_AMOUNT * sizeof(struct ndis_802_11_ssid), - GFP_KERNEL); + ssid = kcalloc(RTW_SSID_SCAN_AMOUNT, sizeof(*ssid), GFP_KERNEL); if (!ssid) { ret = -ENOMEM; goto check_need_indicate_scan_done;
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@gmx.com> --- drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) -- 2.25.1