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

[Tariq Toukan <tariqt@nvidia.com>]

On 12/10/2026 16:48, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider.
> 
> Critical: 1 · High: 0 · Medium: 1 · Low: 2
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Critical] Use-after-free in mlx5e_close_locked() (the ndo_stop path),
>    introduced by this patch.
> - [Medium] The commit message does not match the close-phase code, and
>    the close path's safety now rests on an undocumented, unenforced…
> - [Low] mlx5e_close_locked() now calls mlx5e_close_channels_pre_sync()
>    and mlx5e_close_channels_post_sync() directly instead of…
> - [Low] The patch changes how mlx5e_close_channel_post_sync()
>    (en_main.c:3032) decides whether to tear down a channel's XSK queues,…
> 
> --
> 
> net/mlx5e: Batch NAPI RCU grace periods on channel close
> 
> This splits mlx5e channel deactivate and close into pre_sync and post_sync
> phases, so one synchronize_net() covers all channels instead of one per
> queue. NAPI instances are unlinked with __netif_napi_del_locked(), and a
> single grace period is taken before the channels are freed. Single-queue
> paths are meant to keep their own synchronize_net() calls.
> 
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> index f90b5a34e097..fe4e5d0b2e9a 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> 
> [ ... ]
> 
>> @@ -2141,18 +2160,28 @@ int mlx5e_open_xdpsq(struct mlx5e_channel *c, struct mlx5e_params *params,
>>   	return err;
>>   }
>>   
>> -void mlx5e_close_xdpsq(struct mlx5e_xdpsq *sq)
>> +static void mlx5e_destroy_xdpsq(struct mlx5e_xdpsq *sq)
>>   {
>>   	struct mlx5e_channel *c = sq->channel;
>>   
>> -	clear_bit(MLX5E_SQ_STATE_ENABLED, &sq->state);
>> -	synchronize_net(); /* Sync with NAPI. */
>> -
>>   	mlx5e_destroy_sq(c->mdev, sq->sqn);
>>   	mlx5e_free_xdpsq_descs(sq);
>>   	mlx5e_free_xdpsq(sq);
>>   }
>>   
>> +void mlx5e_close_xdpsq_post_sync(struct mlx5e_xdpsq *sq)
>> +{
>> +	clear_bit(MLX5E_SQ_STATE_ENABLED, &sq->state);
>> +	mlx5e_destroy_xdpsq(sq);
>> +}
>> +
>> +void mlx5e_close_xdpsq(struct mlx5e_xdpsq *sq)
>> +{
>> +	clear_bit(MLX5E_SQ_STATE_ENABLED, &sq->state);
>> +	synchronize_net(); /* Sync with NAPI. */
>> +	mlx5e_destroy_xdpsq(sq);
>> +}
>> +
> 
> [Severity: Medium]
> The commit message says:
> 
>    First clear the ENABLED state on all queues, then wait for a single
>    grace period before continuing with the teardown.
> 
> and:
> 
>    Keep the existing per object synchronize_net() calls for single queue
>    paths.
> 
> Does that match the close phase?
> 
> mlx5e_close_xdpsq_post_sync() clears MLX5E_SQ_STATE_ENABLED after the
> shared grace period and then destroys the SQ right away. So there is no
> pre_sync clear for rq_xdpsq, the XDP_REDIRECT xdpsq or xsksq, and nothing
> waits after this clear_bit(). mlx5e_close_channel_pre_sync() only clears
> MLX5E_CHANNEL_STATE_XSK.
> 
> The other close-phase waits follow the same pattern:
> 
> - mlx5e_qos_close_queues_post_sync() removes the synchronize_rcu()
>    between unpublishing and freeing the array instead of batching it.
> 
> - The single-queue path mlx5e_close_xsk() goes from two synchronize_net()
>    calls to one. That particular reduction looks safe on its own, because
>    NAPI only touches xsksq under the xsk_open snapshot taken in the same
>    RCU section.
> 
> So these waits look removed rather than batched, on the assumption that
> NAPI is already disabled. That precondition only appears in a comment in
> mlx5e_qos_close_queues_post_sync():
> 
> 	/* Caller guarantees that NAPI is stopped or was never enabled. */
> 
> mlx5e_close_xdpsq_post_sync() is exported in en.h and also called from
> xsk/setup.c, but nothing documents or enforces that requirement.
> 
> Could the commit message describe this? Could the precondition also be
> documented on the exported post_sync helpers? As noted below for
> mlx5e_close_locked(), NAPI being disabled may not be enough on its own.
> 
> [ ... ]
> 
>> @@ -2936,35 +2993,56 @@ static void mlx5e_activate_channel(struct mlx5e_channel *c)
> 
> [ ... ]
> 
>> -static void mlx5e_close_channel(struct mlx5e_channel *c)
>> +static void mlx5e_close_channel_pre_sync(struct mlx5e_channel *c)
>>   {
>>   	if (test_bit(MLX5E_CHANNEL_STATE_XSK, c->state))
>> -		mlx5e_close_xsk(c);
>> -	mlx5e_close_queues(c);
>> -	mlx5e_qos_close_queues(c);
>> -	netif_napi_del_locked(&c->napi);
>> +		mlx5e_close_xsk_pre_sync(c);
>> +}
>> +
>> +static void mlx5e_close_channel_post_sync(struct mlx5e_channel *c)
>> +{
>> +	if (c->xskrq.xsk_pool)
>> +		mlx5e_close_xsk_post_sync(c);
> 
> [Severity: Low]
> This isn't a bug, but the test for tearing down the XSK queues changes
> here from test_bit(MLX5E_CHANNEL_STATE_XSK, c->state) to
> c->xskrq.xsk_pool. It is only correct because of the new error-path reset
> in mlx5e_open_xsk() in xsk/setup.c:
> 
> err_close_rx_cq:
> 	mlx5e_close_cq(&c->xskrq.cq);
> 
> 	c->xskrq.xsk_pool = NULL;
> 	c->xsksq.xsk_pool = NULL;
> 
> Without that reset, a failed mlx5e_xsk_enable_locked() on a live channel
> would leave the pool pointer set. Close would then tear down xskrq and
> xsksq a second time.
> 
> The two hunks depend on each other, but the commit message mentions
> neither. Could this be noted in the commit message, so a partial backport
> or a later edit to the error path doesn't bring back a double teardown?
> 
>> +	mlx5e_close_queues_post_sync(c);
>> +	mlx5e_qos_close_queues_post_sync(c);
>> +}
> 
> [ ... ]
> 
>> @@ -3060,33 +3138,72 @@ static int mlx5e_wait_channels_min_rx_wqes(struct mlx5e_channels *chs)
> 
> [ ... ]
> 
>> +static void mlx5e_close_channels_post_sync(struct mlx5e_channels *chs)
>>   {
>>   	int i;
>>   
>> -	ASSERT_RTNL();
>>   	if (chs->ptp) {
>> -		mlx5e_ptp_close(chs->ptp);
>> +		mlx5e_ptp_close_queues(chs->ptp);
>> +		__netif_napi_del_locked(&chs->ptp->napi);
>> +	}
>> +
>> +	for (i = 0; i < chs->num; i++) {
>> +		mlx5e_close_channel_post_sync(chs->c[i]);
>> +		__netif_napi_del_locked(&chs->c[i]->napi);
>> +	}
>> +
>> +	synchronize_net();
>> +
>> +	if (chs->ptp) {
>> +		kvfree(chs->ptp);
>>   		chs->ptp = NULL;
>>   	}
>> +
>>   	for (i = 0; i < chs->num; i++)
>> -		mlx5e_close_channel(chs->c[i]);
>> +		kvfree(chs->c[i]);
>>   
>>   	kfree(chs->c);
>>   	chs->num = 0;
>>   }
>>   
>> +void mlx5e_close_channels(struct mlx5e_channels *chs)
>> +{
>> +	ASSERT_RTNL();
>> +	mlx5e_close_channels_pre_sync(chs);
>> +	synchronize_net();
>> +	mlx5e_close_channels_post_sync(chs);
>> +}
> 
> [Severity: Low]
> mlx5e_close_locked() now calls mlx5e_close_channels_pre_sync() and
> mlx5e_close_channels_post_sync() directly. That means the ASSERT_RTNL(),
> which now lives only in this wrapper, no longer runs on the ndo_stop path.
> 
> This isn't a functional bug, since mlx5e_close() and mlx5e_rep_close()
> both run under RTNL. Would it be worth keeping the assertion in
> mlx5e_close_locked() or in the split helpers?
> 
> [ ... ]
> 
>> @@ -3611,8 +3741,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);
>> +	mlx5e_close_channels_pre_sync(&priv->channels);
>> +
>> +	synchronize_net();
>> +
>> +	mlx5e_deactivate_priv_channels_post_sync(priv);
>> +	mlx5e_close_channels_post_sync(&priv->channels);
> 
> [Severity: Critical]
> In mlx5e_close_locked(), the only grace period now runs before
> napi_disable_locked(). What waits for a NAPI poll that is still running
> between napi_disable_locked() and the frees in
> mlx5e_close_channels_post_sync()?
> 
> The sequence becomes:
> 
> mlx5e_close_locked()
>      synchronize_net()
>      mlx5e_deactivate_priv_channels_post_sync()
>          mlx5e_deactivate_channel_post_sync()
>              napi_disable_locked(&c->napi)
>      mlx5e_close_channels_post_sync()
>          mlx5e_close_channel_post_sync()
>              mlx5e_close_queues_post_sync()
>                  frees sq->dim, rq->dim, c->async_icosq, c->xdpsq
>              mlx5e_qos_close_queues_post_sync()
>                  kfree(sq), kvfree(qos_sqs), no grace period
>          synchronize_net()
>          kvfree(chs->c[i])
> 
> napi_disable_locked() only waits for NAPIF_STATE_SCHED to be released.
> Documentation/networking/napi.rst notes that napi_disable() does not wait
> for the poll method to exit.
> 
> mlx5e_napi_poll() keeps using queue memory after napi_complete_done(),
> inside rcu_read_lock():
> 
> mlx5e_napi_poll() {
>      ...
> 	qos_sqs = rcu_dereference(c->qos_sqs);
>      ...
> 	if (unlikely(!napi_complete_done(napi, work_done)))
> 		goto out;
>      ...
> 	for (i = 0; i < c->num_tc; i++) {
> 		mlx5e_handle_tx_dim(&c->sq[i]);
> 		mlx5e_cq_arm(&c->sq[i].cq);
>      ...
> 			if (sq) {
> 				mlx5e_handle_tx_dim(sq);
> 				mlx5e_cq_arm(&sq->cq);
>      ...
> 	mlx5e_handle_rx_dim(rq);
>      ...
> }
> 
> The NAPI CPU could be delayed after napi_complete_done(), for example a
> preempted threaded NAPI on PREEMPT_RT, or vCPU preemption. In that case,
> can this tail touch the QoS SQs, the qos_sqs array, the DIM structs,
> async_icosq and xdpsq after they have been freed? net_dim() could also
> schedule dim->work on freed memory after cancel_work_sync() has already
> run.
> 
> Before this patch, mlx5e_qos_close_queues() did synchronize_rcu() between
> rcu_replace_pointer() and the frees. The per-object synchronize_net()
> calls also gave a grace period after each earlier channel's
> napi_disable_locked().
> 
> The synchronize_net() in mlx5e_close_channels_post_sync() runs after the
> queues are already freed, so it only covers the mlx5e_channel and
> mlx5e_ptp structs.
> 
> The other paths call mlx5e_deactivate_priv_channels() and then
> mlx5e_close_channels(), so they still get one grace period after
> napi_disable_locked(). Is merging the two into a single wait safe for
> mlx5e_close_locked()?
> 

We'll fix.

pw-bot: cr

>>   
>>   	return 0;
>>   }
>