netdev - Re: [PATCH] sfc: use kmalloc_array() instead of kmalloc()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <7bfb811e-72cf-43c4-81e1-6e63338f6a29@gmail.com>
Date: Tue, 26 Aug 2025 20:15:37 +0100
From: Edward Cree <ecree.xilinx@...il.com>
To: Qianfeng Rong <rongqianfeng@...o.com>, linux-net-drivers@....com,
 Andy Moreton <andy.moreton@....com>
Cc: Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] sfc: use kmalloc_array() instead of kmalloc()

On 25/08/2025 16:12, Qianfeng Rong wrote:
> As noted in the kernel documentation [1], open-coded multiplication in
> allocator arguments is discouraged because it can lead to integer overflow.
> 
> Use kmalloc_array() to gain built-in overflow protection, making memory
> allocation safer when calculating allocation size compared to explicit
> multiplication.
> 
> Link: https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments #1
> Signed-off-by: Qianfeng Rong <rongqianfeng@...o.com>

This is fine on its own terms, but I would prefer the use of array_size()
 (as in the existing ef100 code) rather than kmalloc_array(), because the
 new X4 devices report a buffer size rather than a number of stats to the
 host, meaning that the common code will need to work in terms of size
 rather than num when support for stats on X4 is added.
Looping in AndyM who's been working on said support and can hopefully
 correct me if I've misstated anything.

-ed

> ---
>  drivers/net/ethernet/sfc/ef10.c      | 4 ++--
>  drivers/net/ethernet/sfc/ef100_nic.c | 3 ++-
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
> index fcec81f862ec..311df5467c4a 100644
> --- a/drivers/net/ethernet/sfc/ef10.c
> +++ b/drivers/net/ethernet/sfc/ef10.c
> @@ -1326,8 +1326,8 @@ static int efx_ef10_init_nic(struct efx_nic *efx)
>  		efx->must_realloc_vis = false;
>  	}
>  
> -	nic_data->mc_stats = kmalloc(efx->num_mac_stats * sizeof(__le64),
> -				     GFP_KERNEL);
> +	nic_data->mc_stats = kmalloc_array(efx->num_mac_stats, sizeof(__le64),
> +					   GFP_KERNEL);
>  	if (!nic_data->mc_stats)
>  		return -ENOMEM;
>  
> diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c
> index 3ad95a4c8af2..f4b74381831f 100644
> --- a/drivers/net/ethernet/sfc/ef100_nic.c
> +++ b/drivers/net/ethernet/sfc/ef100_nic.c
> @@ -640,7 +640,8 @@ static size_t ef100_update_stats(struct efx_nic *efx,
>  				 u64 *full_stats,
>  				 struct rtnl_link_stats64 *core_stats)
>  {
> -	__le64 *mc_stats = kmalloc(array_size(efx->num_mac_stats, sizeof(__le64)), GFP_ATOMIC);
> +	__le64 *mc_stats = kmalloc_array(efx->num_mac_stats, sizeof(__le64),
> +					 GFP_ATOMIC);
>  	struct ef100_nic_data *nic_data = efx->nic_data;
>  	DECLARE_BITMAP(mask, EF100_STAT_COUNT) = {};
>  	u64 *stats = nic_data->stats;