Re: [PATCH net-next V2 1/2] net/mlx5e: Batch NAPI RCU grace periods on channel close

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] net/mlx5e: missing grace period after page_pool_destroy() in
  channel close
- [Low] net/mlx5e: redundant pre/post_sync open-coding and duplicated
  queue teardown
--

commit 2f06177c661abed7fd45e14fc9a588083535c8b2
Author: Carolina Jubran <cjubran@nvidia.com>

net/mlx5e: Batch NAPI RCU grace periods on channel close

This commit splits channel deactivation and teardown into pre_sync and
post_sync phases so that synchronize_net() grace periods are batched
across all channels during channel close.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index c3c2c5720c6f3..73b504ba8c8c6 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c

[ ... ]

> @@ -2749,6 +2785,27 @@ static void mlx5e_close_queues(struct mlx5e_channel *c)
>  	mlx5e_close_cq(&c->icosq.cq);
>  }
>  
> +static void mlx5e_close_queues_post_sync(struct mlx5e_channel *c)
> +{
> +	if (c->xdp)
> +		mlx5e_close_xdpsq_post_sync(&c->rq_xdpsq);
> +	/* The same ICOSQ is used for UMRs for both RQ and XSKRQ. */
> +	cancel_work_sync(&c->icosq.recover_work);
> +	mlx5e_close_rq(&c->rq);
> +	mlx5e_close_sqs(c);
> +	mlx5e_close_icosq(&c->icosq);
> +	mutex_destroy(&c->icosq_recovery_lock);
> +	if (c->async_icosq)
> +		mlx5e_close_async_icosq(c->async_icosq);
> +	if (c->xdp)
> +		mlx5e_close_cq(&c->rq_xdpsq.cq);
> +	mlx5e_close_cq(&c->rq.cq);
> +	if (c->xdpsq)
> +		mlx5e_close_xdpredirect_sq_post_sync(c->xdpsq);
> +	mlx5e_close_tx_cqs(c);
> +	mlx5e_close_cq(&c->icosq.cq);
> +}

[ ... ]

> @@ -3060,32 +3144,67 @@ static int mlx5e_wait_channels_min_rx_wqes(struct mlx5e_channels *chs)

[ ... ]

> +static void mlx5e_close_channels_pre_sync(struct mlx5e_channels *chs)
> +{
> +	int i;
> +
> +	if (chs->ptp)
> +		__netif_napi_del_locked(&chs->ptp->napi);
> +
> +	for (i = 0; i < chs->num; i++) {
> +		mlx5e_close_channel_pre_sync(chs->c[i]);
> +		__netif_napi_del_locked(&chs->c[i]->napi);
> +	}
> +}
> +
> +static void mlx5e_close_channels_post_sync(struct mlx5e_channels *chs)
> +{
> +	int i;
> +
> +	if (chs->ptp) {
> +		mlx5e_ptp_close_queues(chs->ptp);
> +		kvfree(chs->ptp);
> +		chs->ptp = NULL;
> +	}
> +
> +	for (i = 0; i < chs->num; i++) {
> +		mlx5e_close_channel_post_sync(chs->c[i]);
> +		kvfree(chs->c[i]);
> +	}
>  
>  	kfree(chs->c);
>  	chs->num = 0;
>  }
>  
> +void mlx5e_close_channels(struct mlx5e_channels *chs)
> +{
> +	mlx5e_close_channels_pre_sync(chs);
> +	synchronize_net();
> +	mlx5e_close_channels_post_sync(chs);
> +}

[Severity: High]
Can freeing chs->ptp and chs->c[i] in mlx5e_close_channels_post_sync()
cause a use-after-free on &c->napi (or &ptp->napi) in
page_pool_napi_local()?

rq->page_pool is created with pool->p.napi pointing to the NAPI struct
embedded in struct mlx5e_channel or struct mlx5e_ptp.  That pointer is
only cleared when mlx5e_close_rq() -> mlx5e_free_rq() ->
xdp_rxq_info_unreg() calls page_pool_destroy():

net/core/page_pool.c:page_pool_disable_direct_recycling() {
	...
	WRITE_ONCE(pool->p.napi, NULL);
}

Before this patch, mlx5e_close_queues() and mlx5e_ptp_close_queues() ran
before netif_napi_del_locked(), so synchronize_net() waited for any
concurrent softirq running page_pool_napi_local() to finish before
kvfree() freed the channel.  Moving queue teardown into
mlx5e_close_channels_post_sync() places page_pool_destroy() after
synchronize_net(), and page_pool_scrub() skips its own synchronize_net()
when dma_dev_need_sync() is false:

CPU0                                  CPU1 (<softirq>)
mlx5e_close_channels()
  mlx5e_close_channels_pre_sync()
  synchronize_net()
  mlx5e_close_channels_post_sync()
    mlx5e_close_channel_post_sync()
      ...
                                      napi_consume_skb()
                                        ...
                                          page_pool_napi_local()
                                            napi = READ_ONCE(pool->p.napi);
        page_pool_destroy()
          WRITE_ONCE(pool->p.napi, NULL);
    kvfree(chs->c[i])
                                            READ_ONCE(napi->list_owner)
                                              <-- use-after-free

Closing the queues before synchronize_net() (or waiting for a grace period
after page_pool_destroy() before kvfree()) would avoid freeing the channel
while a softirq still holds the NAPI pointer.

[ ... ]

> @@ -3610,8 +3742,14 @@ int mlx5e_close_locked(struct net_device *netdev)
>  	clear_bit(MLX5E_STATE_OPENED, &priv->state);
>  
>  	netif_carrier_off(priv->netdev);
> -	mlx5e_deactivate_priv_channels(priv);
> -	mlx5e_close_channels(&priv->channels);
> +
> +	mlx5e_deactivate_priv_channels_pre_sync(priv);
> +	synchronize_net();
> +	mlx5e_deactivate_priv_channels_post_sync(priv);
> +
> +	mlx5e_close_channels_pre_sync(&priv->channels);
> +	synchronize_net();
> +	mlx5e_close_channels_post_sync(&priv->channels);
>  
>  	return 0;
>  }

[Severity: Low]
This isn't a bug, but mlx5e_close_locked() open-codes the exact bodies of
mlx5e_deactivate_priv_channels() and mlx5e_close_channels() without
combining their synchronize_net() calls, so it could keep calling those
two helpers directly.  Similarly, mlx5e_close_queues_post_sync()
duplicates mlx5e_close_queues(); mlx5e_close_queues() could run the
pre_sync steps, call synchronize_net(), and call
mlx5e_close_queues_post_sync() like mlx5e_close_channel() does.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110185005.2932568-1-tariqt@nvidia.com?part=1