Re: [PATCH net V3 1/3] net/mlx5e: Fix HV VHCA stats zero-sized buffer allocation

[Tariq Toukan <tariqt@nvidia.com>]

On 23/06/2026 13:46, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev
> 
> Simon says: I think this issue can be addressed by reversing the order
> that patches 1/3 and 2/3 appear in this patch-set.
> 
> ---
> net/mlx5e: Fix HV VHCA stats zero-sized buffer allocation
> 
> This commit fixes a zero-sized buffer allocation by using max_nch instead of
> stats_nch when allocating the buffer for HV VHCA stats.
> 
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/hv_vhca_stats.c b/drivers/net/ethernet/mellanox/mlx5/core/en/hv_vhca_stats.c
>> index 195863b2c013e..06cbd49d4e98e 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/hv_vhca_stats.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/hv_vhca_stats.c
>> @@ -122,7 +128,7 @@ static void mlx5e_hv_vhca_stats_cleanup(struct mlx5_hv_vhca_agent *agent)
>>   
>>   void mlx5e_hv_vhca_stats_create(struct mlx5e_priv *priv)
>>   {
>> -	int buf_len = mlx5e_hv_vhca_stats_buf_size(priv);
>> +	int buf_len = mlx5e_hv_vhca_stats_buf_max_size(priv);
>>   	struct mlx5_hv_vhca_agent *agent;
>>   
>>   	priv->stats_agent.buf = kvzalloc(buf_len, GFP_KERNEL);
> 
> [Severity: High]
> Does this change introduce a potential double-free regression on memory
> allocation failure?
> 
> When mlx5e_hv_vhca_stats_destroy() cleans up the stats agent, it frees the
> agent but does not set priv->stats_agent.agent to NULL.
> 

Good catch on the double-free, thank you.

Swapping the patches has its own problem: patch 2/3 also modifies 
mlx5e_hv_vhca_stats_create(), so applying the race fix first would leave 
an intermediate tree that still has the zero-sized buffer bug, which is 
exactly the condition the race fix builds on top of.

Instead, I think we'll make patch 1/3 self-contained by including the 
NULL assignments in mlx5e_hv_vhca_stats_destroy():

       mlx5_hv_vhca_agent_destroy(priv->stats_agent.agent);
       priv->stats_agent.agent = NULL;
       kvfree(priv->stats_agent.buf);
       priv->stats_agent.buf = NULL;

This closes the double-free window in the same patch that opens it, with 
no ordering dependency on patch 2/3.

> Before this patch, kvzalloc() was called with 0 and returned ZERO_SIZE_PTR,
> so it never returned NULL. It always proceeded to call
> mlx5_hv_vhca_agent_create() which overwrote priv->stats_agent.agent.
> 
> Now that the buffer size is non-zero, if kvzalloc() fails it will return
> NULL and take the early return path in mlx5e_hv_vhca_stats_create():
> 
> 	priv->stats_agent.buf = kvzalloc(buf_len, GFP_KERNEL);
> 	if (!priv->stats_agent.buf)
> 		return;
> 
> This early return bypasses overwriting the dangling priv->stats_agent.agent.
> If a subsequent device detach occurs, mlx5e_hv_vhca_stats_destroy() could
> dereference and free this stale pointer again.
> 
> I noticed this was later addressed upstream in commit e600849cc1e0
> ("net/mlx5e: Fix HV VHCA stats agent registration race").
>