Re: [PATCH net 5/7] net/mlx5e: macsec: Delete remaining SecYs during cleanup

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 2 (1) · Medium: 0 (1) · Low: 0

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

- [High] mlx5e_macsec_cleanup() frees the MACsec context and sets
  `priv->macsec = NULL` (macsec.c:1826-1827) without ever withdrawing…
- [High] mlx5e_macsec_cleanup() takes macsec->lock only for the new SecY-
  deletion sweep (macsec.c:1814-1819), then drops it and destroys/frees…

Pre-existing issues:
- [High] macsec_del_rxsc_ctx() (macsec.c:820-849), which this patch newly
  invokes from the bulk cleanup path, releases the RX-SC's xarray…
- [Medium] Suspend-time cleanup now deletes every programmed SecY, SA,
  encryption key, MACsec HW object and steering rule…

--

net/mlx5e: macsec: Delete remaining SecYs during cleanup

The patch factors the SecY teardown out of mlx5e_macsec_del_secy() into a
new macsec_del_secy() helper, and calls it for every remaining entry of
macsec->macsec_device_list_head from mlx5e_macsec_cleanup().  It also
adds xa_destroy(), clears mdev->macsec_fs and clears priv->macsec at the
end of cleanup.

A few questions below about the teardown ordering and about the callbacks
that remain registered after cleanup has run.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> index 20eec345b36cd..84ce388d3846e 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c

[ ... ]

> @@ -1272,7 +1260,7 @@ static int mlx5e_macsec_del_secy(struct macsec_context *ctx)
>  
>  	list = &macsec_device->macsec_rx_sc_list_head;
>  	list_for_each_entry_safe(rx_sc, tmp, list, rx_sc_list_element)
> -		macsec_del_rxsc_ctx(macsec, rx_sc, ctx->secy->netdev);
> +		macsec_del_rxsc_ctx(macsec, rx_sc, netdev);

[Severity: High]

This isn't a bug introduced by this patch, but since the cleanup sweep is
now a second caller of macsec_del_rxsc_ctx() it seems worth asking about
the lifetime of rx_sc->sc_xarray_element there:

drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c:macsec_del_rxsc_ctx() {
	...
	list_del_rcu(&rx_sc->rx_sc_list_element);
	xa_erase(&macsec->sc_xarray, rx_sc->sc_xarray_element->fs_id);
	dst_release(&rx_sc->md_dst->dst);
	kfree(rx_sc->sc_xarray_element);
	kfree_rcu_mightsleep(rx_sc);
}

The RX datapath loads that element under RCU:

drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c:mlx5e_macsec_offload_handle_rx_skb() {
	...
	rcu_read_lock();
	sc_xarray_element = xa_load(&macsec->sc_xarray, fs_id);
	rx_sc = sc_xarray_element ? sc_xarray_element->rx_sc : NULL;
	if (rx_sc && rx_sc->md_dst) {
	...
}

Only rx_sc is freed via kfree_rcu_mightsleep(); sc_xarray_element is freed
with plain kfree() right after xa_erase().  Can a reader that already
obtained sc_xarray_element from xa_load() then dereference freed memory for
sc_xarray_element->rx_sc, and go on to dereference the resulting garbage
pointer at rx_sc->md_dst?

The comment just above those lines says:

	/* At this point the relevant MACsec offload Rx rule already removed at
	 * mlx5e_macsec_cleanup_sa need to wait for datapath to finish current
	 * Rx related data propagating using xa_erase which uses rcu to sync,
	 * once fs_id is erased then this rx_sc is hidden from datapath.
	 */

xa_erase() stores NULL into the slot but does not wait for a grace period,
so is the stated justification for the immediate kfree() of
sc_xarray_element accurate?

>  
>  	kfree(macsec_device->dev_addr);
>  	macsec_device->dev_addr = NULL;
> @@ -1280,7 +1268,24 @@ static int mlx5e_macsec_del_secy(struct macsec_context *ctx)
>  	list_del_rcu(&macsec_device->macsec_device_list_element);
>  	--macsec->num_of_devices;
>  	kfree(macsec_device);
> +}
> +
> +static int mlx5e_macsec_del_secy(struct macsec_context *ctx)
> +{
> +	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
> +	struct mlx5e_macsec *macsec = priv->macsec;
> +	struct mlx5e_macsec_device *macsec_device;
> +	int err = 0;
>  
> +	mutex_lock(&macsec->lock);
> +	macsec_device = mlx5e_macsec_get_macsec_device_context(macsec, ctx);
> +	if (!macsec_device) {
> +		netdev_err(ctx->netdev, "MACsec offload: Failed to find device context\n");
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
> +	macsec_del_secy(macsec, macsec_device);
>  out:
>  	mutex_unlock(&macsec->lock);
>  
> @@ -1796,6 +1801,7 @@ int mlx5e_macsec_init(struct mlx5e_priv *priv)
>  
>  void mlx5e_macsec_cleanup(struct mlx5e_priv *priv)
>  {
> +	struct mlx5e_macsec_device *macsec_device, *tmp;
>  	struct mlx5e_macsec *macsec = priv->macsec;
>  	struct mlx5_core_dev *mdev = priv->mdev;
>  
> @@ -1803,9 +1809,20 @@ void mlx5e_macsec_cleanup(struct mlx5e_priv *priv)
>  		return;
>  
>  	mlx5_notifier_unregister(mdev, &macsec->nb);
> -	mlx5_macsec_fs_cleanup(mdev->macsec_fs);
>  	destroy_workqueue(macsec->wq);
> +
> +	mutex_lock(&macsec->lock);
> +	list_for_each_entry_safe(macsec_device, tmp,
> +				 &macsec->macsec_device_list_head,
> +				 macsec_device_list_element)
> +		macsec_del_secy(macsec, macsec_device);
> +	mutex_unlock(&macsec->lock);

[Severity: Medium]

This is a pre-existing restoration gap rather than something this patch
introduces, and the commit message already says fixing it is out of scope,
but it may be worth spelling out what the observable result is.

After this sweep plus mlx5_macsec_fs_cleanup(), every SecY, SA, encryption
key, MACsec object and steering rule is gone, while the MACsec upper
netdevices stay registered with their SecYs still marked offloaded.
mlx5e_macsec_init() on resume only allocates a fresh context, an empty
sc_xarray, a new ASO and a new flow-steering instance, with no replay.

So on resume, does offloaded TX end up dropping frames here?

drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c:mlx5e_macsec_handle_tx_skb() {
	...
	fs_id = mlx5_macsec_fs_get_fs_id_from_hashtable(macsec->mdev->macsec_fs,
							&md_dst->u.macsec_info.sci);
	if (!fs_id)
		goto err_out;
	...
}

and do later control operations on those retained SecYs fail with -EINVAL
from the device-context lookup in mlx5e_macsec_del_secy(), which also makes
macsec_update_offload() abort the transition?

> +
> +	xa_destroy(&macsec->sc_xarray);
> +	mlx5_macsec_fs_cleanup(mdev->macsec_fs);
> +	mdev->macsec_fs = NULL;
>  	mlx5e_macsec_aso_cleanup(&macsec->aso, mdev);
>  	mutex_destroy(&macsec->lock);
>  	kfree(macsec);

[Severity: High]

macsec->lock is released before mutex_destroy() and kfree(macsec), and
priv->macsec is cleared only after the free.  Every offload callback reads
priv->macsec locklessly and then blocks on that same mutex, for example
mlx5e_macsec_add_secy() and the rewritten mlx5e_macsec_del_secy():

	struct mlx5e_macsec *macsec = priv->macsec;
	...
	mutex_lock(&macsec->lock);

mlx5e_macsec_cleanup() runs from mlx5e_nic_disable() after rtnl_unlock(),
while the MACsec core dispatches the mdo_* ops under rtnl (macsec_newlink()
-> mdo_add_secy, macsec_upd_offload() -> macsec_update_offload()), so rtnl
does not serialize the two paths and macsec->lock is the only mutual
exclusion:

CPU0 (netlink, rtnl held)
mlx5e_macsec_add_secy()
	macsec = priv->macsec;		/* still non-NULL */
	mutex_lock(&macsec->lock);	/* blocks */

CPU1 (devlink reload / health recovery / suspend, no rtnl)
mlx5e_macsec_cleanup()
	mutex_unlock(&macsec->lock);
	...
	mutex_destroy(&macsec->lock);
	kfree(macsec);
	priv->macsec = NULL;

Can CPU0 then acquire a mutex inside freed memory, list_add a new
macsec_device into the freed macsec_device_list_head, and mutex_unlock()
freed memory?  A SecY added in that window would also be leaked, which is
the case this patch is trying to close.  Would clearing the gating state
(and withdrawing the offload registration) before dropping the lock and
freeing be a better ordering?

> +	priv->macsec = NULL;
>  }

[Severity: High]

Clearing priv->macsec here leaves the offload registration in place.
netdev->macsec_ops and NETIF_F_HW_MACSEC are set once in
mlx5e_macsec_build_netdev(), called from mlx5e_build_nic_netdev() at netdev
creation, and nothing in mlx5e_macsec_cleanup() reverses that.  The core
only checks those two fields:

drivers/net/macsec.c:macsec_check_offload() {
	...
		return macsec->real_dev->features & NETIF_F_HW_MACSEC &&
		       macsec->real_dev->macsec_ops;
	...
}

There is no netif_device_present() test there, and none of the mlx5e
callbacks NULL-check priv->macsec; they all start with
mutex_lock(&priv->macsec->lock), and the rewritten mlx5e_macsec_del_secy()
does:

	struct mlx5e_macsec *macsec = priv->macsec;
	...
	mutex_lock(&macsec->lock);

Since mlx5e_macsec_cleanup() is called from mlx5e_nic_disable() after
rtnl_unlock() and with the netdev still registered (only
netif_device_detach()'ed), can "ip link del macsec0" reach
macsec_common_dellink() -> mdo_del_secy -> mlx5e_macsec_del_secy() with
priv->macsec == NULL and oops on the mutex_lock()?

The same applies indefinitely if mlx5e_macsec_init() fails on resume, since
mlx5e_nic_enable() only logs the error:

drivers/net/ethernet/mellanox/mlx5/core/en_main.c:mlx5e_nic_enable() {
	...
	err = mlx5e_macsec_init(priv);
	if (err)
		mlx5_core_err(mdev, "MACsec initialization failed, %d\n", err);
	...
}

so priv->macsec stays NULL while the ops remain advertised.  The TX helpers
reached from en_accel.h have the same exposure, for instance
mlx5e_macsec_handle_tx_skb() dereferencing macsec->mdev->macsec_fs with no
NULL check on macsec.  Should cleanup also drop macsec_ops and
NETIF_F_HW_MACSEC, or should the callbacks check priv->macsec?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917175433.4090878-1-tariqt%40nvidia.com