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

[Tariq Toukan]

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").