Re: [PATCH net-next V4 04/14] net/mlx5e: macsec: Block TC offload when MACsec is enabled

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 1

- [High] Unbalanced TC-block reference across the MACsec teardown path:
  mlx5e_macsec_init_sa_fs() now takes one mdev->offload_block.num_tc…
- [Low] Commit-message scope vs. implementation: the message states the
  patch makes "sure that TC and MACsec cannot be both active at the same…

--

net/mlx5e: macsec: Block TC offload when MACsec is enabled

The patch takes a reference on the mlx5 TC/accel mutual exclusion counter
whenever a MACsec RX (decrypt) steering rule is installed, and drops it when
that rule is removed. It also holds a best-effort block across
macsec_upd_secy_hw_address() so the drain/re-add loops are not disturbed by a
concurrent TC filter add.

> 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 daff53ba7d09f..a15a0aff292ff 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c

[ ... ]

> @@ -324,6 +325,8 @@ static void mlx5e_macsec_cleanup_sa_fs(struct mlx5e_macsec *macsec,
>  	mlx5_macsec_fs_del_rule(macsec->mdev->macsec_fs, sa->macsec_rule, action, netdev,
>  				fs_id);
>  	sa->macsec_rule = NULL;
> +	if (!is_tx)
> +		mlx5e_accel_unblock_tc_offload(macsec->mdev);
>  }

[Severity: High]
This is the only place that gives back the count taken in
mlx5e_macsec_init_sa_fs(). Is mlx5e_macsec_cleanup() a second teardown path
that destroys the same RX rules without passing through here?

drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c:mlx5e_macsec_cleanup() {
	mlx5_notifier_unregister(mdev, &macsec->nb);
	mlx5_macsec_fs_cleanup(mdev->macsec_fs);
	destroy_workqueue(macsec->wq);
	mlx5e_macsec_aso_cleanup(&macsec->aso, mdev);
	mutex_destroy(&macsec->lock);
	kfree(macsec);
}

It never walks macsec_device_list_head / rx_sc->rx_sa[] and never calls
mlx5e_accel_unblock_tc_offload(), so wouldn't every RX SA rule still installed
at that moment leave mdev->offload_block.num_tc permanently incremented?

The path looks reachable without any mdo_del_rxsa/mdo_del_secy first:

mlx5_unload_one()/mlx5e_suspend() -> _mlx5e_suspend() ->
mlx5e_detach_netdev() -> mlx5e_nic_profile.disable == mlx5e_nic_disable() ->
mlx5e_macsec_cleanup()

In those flows (system suspend, devlink reload, health recovery) the lower
netdev is only netif_device_detach()ed, not unregistered, so the stacked
MACsec devices and their offloaded RX SAs are still live. mdev->offload_block
is initialized once in mlx5_mdev_init() and is not reset by unload/load.

After such an event, does every later TC flower add on that device fail
forever?

drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:mlx5e_tc_block_accel_offload() {
	mutex_lock(&mdev->offload_block.lock);
	if (mdev->offload_block.num_tc)
		ret = -EBUSY;
	...
}

The WARN_ON_ONCE() in mlx5e_accel_unblock_tc_offload() would not fire here
because the leak is a missing decrement, so there is no diagnostic either.
Would it be more robust to drop the reference in mlx5e_macsec_cleanup(), or to
hold it per MACsec device in add_secy/del_secy instead of per RX rule?

>  
>  static void mlx5e_macsec_cleanup_sa(struct mlx5e_macsec *macsec,
> @@ -343,6 +346,7 @@ static int mlx5e_macsec_init_sa_fs(struct macsec_context *ctx,
>  	const struct macsec_tx_sc *tx_sc = &ctx->secy->tx_sc;
>  	struct mlx5_macsec_rule_attrs rule_attrs;
>  	union mlx5_macsec_rule *macsec_rule;
> +	int err = 0;
>  
>  	if (is_tx && tx_sc->encoding_sa != sa->assoc_num)
>  		return 0;
> @@ -353,13 +357,26 @@ static int mlx5e_macsec_init_sa_fs(struct macsec_context *ctx,
>  	rule_attrs.action = (is_tx) ? MLX5_ACCEL_MACSEC_ACTION_ENCRYPT :
>  				      MLX5_ACCEL_MACSEC_ACTION_DECRYPT;
>  
> +	if (!is_tx) {
> +		err = mlx5e_accel_block_tc_offload(priv->mdev);
> +		if (err)
> +			return err;
> +	}
> +

[Severity: Low]
This isn't a bug, but the commit message says the patch makes

    "sure that TC and MACsec cannot be both active at the same time"

while the block here is taken only for RX rules (!is_tx), and only for as long
as an RX rule exists. A device configured with just an active TX SA goes
through mlx5e_macsec_add_txsa() -> mlx5e_macsec_init_sa() ->
mlx5e_macsec_init_sa_fs() with is_tx=true and never touches
mdev->offload_block.num_tc, so TC flower filters are still accepted while
MACsec offload is active.

That scope looks functionally sufficient, since the follow-up patch
"net/mlx5e: macsec: Move RX marker from ft_metadata to flow_tag" only sets
FLOW_CONTEXT_HAS_TAG/flow_tag on the RX crypto rules in
macsec_fs_rx_add_rule(), the TX path keeps using MLX5_MACSEC_TX_METADATA, and
tc_act_can_offload_mark() limits TC marks to 16 bits so a TC filter cannot
forge the proto bits. Could the commit message state the RX-only scope
explicitly, the way the sibling IPsec patch does?

Related to the same narrow scope: because the block is per RX rule, a TC filter
can be installed during any window with no RX rule present (all RX SAs
inactive, or after a partially failed macsec_upd_secy_hw_address()). The next
RX SA activation then returns a bare -EBUSY from mlx5e_macsec_init_sa_fs()
with no netdev_err() or extack, unlike the other error paths in this file.
Would adding a message here help users understand the failure?

[ ... ]

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