Re: [PATCH net 4/7] net/mlx5e: macsec: Track hardware object ownership for SA teardown

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] `mlx5e_macsec_destroy_object()`
  (drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c:303-312)…

Pre-existing issues:
- [High] TX steering rules are still programmed with an unallocated
  MACsec object ID.
- [Medium] `mlx5e_macsec_upd_txsa()` commits `tx_sa->active =
  ctx_tx_sa->active;`…

--

net/mlx5e: macsec: Track hardware object ownership for SA teardown

This adds a macsec_obj_created flag to struct mlx5e_macsec_sa, sets it after
a successful hardware MACsec object creation, and checks it in
mlx5e_macsec_cleanup_sa() before issuing the destroy command. The intent is
to avoid destroying an object the driver never allocated, or one that was
already destroyed while the driver SA kept its object ID.

> 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..20eec345b36cd 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> @@ -58,6 +58,7 @@ struct mlx5e_macsec_async_work {
>  
>  struct mlx5e_macsec_sa {
>  	bool active;
> +	bool macsec_obj_created;
>  	u8  assoc_num;
>  	u32 macsec_obj_id;
>  	u32 enc_key_id;
> @@ -331,7 +332,10 @@ static void mlx5e_macsec_cleanup_sa(struct mlx5e_macsec *macsec,
>  				    struct net_device *netdev, u32 fs_id)
>  {
>  	mlx5e_macsec_cleanup_sa_fs(macsec, sa, is_tx, netdev, fs_id);
> -	mlx5e_macsec_destroy_object(macsec->mdev, sa->macsec_obj_id);
> +	if (sa->macsec_obj_created) {
> +		mlx5e_macsec_destroy_object(macsec->mdev, sa->macsec_obj_id);
> +		sa->macsec_obj_created = false;
> +	}
>  }

[Severity: High]

This isn't a bug introduced by this patch, but the new flag establishes that
macsec_obj_id is only meaningful while macsec_obj_created is set, and the
paths that consume the ID do not honour that.  Should the rule creation path
check it too?

mlx5e_macsec_init_sa_fs() copies the ID with no check:

	rule_attrs.macsec_obj_id = sa->macsec_obj_id;
	rule_attrs.sci = sa->sci;
	rule_attrs.assoc_num = sa->assoc_num;

and macsec_fs_tx_setup_fte() in lib/macsec_fs.c hands that value to the
device as the crypto object of the encrypt rule:

	flow_act->crypto.obj_id = macsec_obj_id;

A TX SA can reach that point with macsec_obj_id == 0 and
macsec_obj_created == false, because mlx5e_macsec_add_txsa() keeps the SA
after creating only the key:

	macsec_device->tx_sa[assoc_num] = tx_sa;
	if (!secy->operational)
		goto out;

Nothing creates the object afterwards: mlx5e_macsec_upd_txsa() and
mlx5e_macsec_upd_secy() only call mlx5e_macsec_init_sa_fs(), never
mlx5e_macsec_init_sa().

Can the following plain configuration sequence reach that state, with no
failure injection?

  ip macsec add <dev> tx sa 0 on key ...      /* object created */
  ip macsec set <dev> tx sa 0 off             /* macsec_upd_txsa() in
                                                 drivers/net/macsec.c sets
                                                 secy->operational = false */
  ip macsec add <dev> tx sa 1 on key ...      /* assoc_num != encoding_sa, so
                                                 operational stays false and
                                                 mlx5e_macsec_add_txsa()
                                                 takes the early exit */
  ip link set <dev> type macsec encoding-sa 1 /* macsec_changelink_common()
                                                 sets secy->operational and
                                                 macsec_changelink() calls
                                                 mdo_upd_secy */

mlx5e_macsec_upd_secy() then deletes all TX rules in its first loop and, in
the second loop, matches SA 1:

		if (tx_sa->assoc_num == tx_sc->encoding_sa && tx_sa->active) {
			err = mlx5e_macsec_init_sa_fs(ctx, tx_sa, tx_sc->encrypt, true, NULL);
			if (err)
				goto out;
		}

so the encrypt rule is installed against object ID 0, which the driver never
allocated.  If firmware rejects it, the error surfaces as -ENOMEM from
mlx5e_macsec_init_sa_fs() after the previous TX rules were already removed;
if it accepts it, the SecY is operational with an encrypt rule bound to an
object the driver does not own.  Would guarding init_sa_fs() on
macsec_obj_created (or creating the object when the SecY becomes operational)
be the matching change?

The ASO event lookups have the same predicate mismatch.
get_macsec_tx_sa_from_obj_id() and get_macsec_rx_sa_from_obj_id() select SAs
by activity, not by ownership:

			macsec_sa = iter->tx_sa[i];
			if (!macsec_sa || !macsec_sa->active)
				continue;
			if (macsec_sa->macsec_obj_id == obj_id)
				return macsec_sa;

An active SA that carries an ID it does not own can be picked up for
macsec_epn_update()/mlx5e_macsec_modify_obj(), while the inactive TX SA whose
object this patch deliberately keeps alive is skipped and its EPN event is
dropped as not found.  Should these lookups test macsec_obj_created instead
of active?

[Severity: Low]

Separately, mlx5e_macsec_destroy_object() discards the command status:

	mlx5_cmd_exec(mdev, in, sizeof(in), out, sizeof(out));

and the hunk above clears sa->macsec_obj_created regardless of the result.
Before this change, a failed destroy would be retried by the next
unconditional cleanup (RX SA deactivation followed by RX SC or SecY
deletion); with the flag cleared unconditionally that retry no longer
happens, so a firmware destroy failure would leave the object allocated for
good.  The destroy_macsec_object unwind in mlx5e_macsec_init_sa() has the
same property.  Ignoring the status of a destroy command in teardown is the
existing mlx5 convention (see mlx5_destroy_ipsec_obj() and the void
mlx5_destroy_encryption_key()), so this is only about the retry that the new
flag removes.

> @@ -396,6 +400,7 @@ static int mlx5e_macsec_init_sa(struct macsec_context *ctx,
>  		if (err)
>  			goto destroy_macsec_object;
>  	}
> +	sa->macsec_obj_created = true;
>  
>  	return 0;
>  

[Severity: Medium]

This is a pre-existing issue and not something the patch changes, but while
looking at the SA activity bookkeeping next door: mlx5e_macsec_upd_txsa()
commits the new activity value before validating the deactivation
precondition, and the error exit does not undo it:

	tx_sa->active = ctx_tx_sa->active;
	if (tx_sa->assoc_num != tx_sc->encoding_sa)
		goto out;

	if (ctx_tx_sa->active) {
		err = mlx5e_macsec_init_sa_fs(ctx, tx_sa, tx_sc->encrypt, true, NULL);
		if (err)
			goto out;
	} else {
		if (!tx_sa->macsec_rule) {
			err = -EINVAL;
			goto out;
		}

The core rolls its own state back on offload error in macsec_upd_txsa():

	tx_sa->active = was_active;
	secy->operational = was_operational;

So if an earlier activation failed in mlx5e_macsec_init_sa_fs(), the driver
SA is left active with macsec_rule == NULL while the core believes it is
inactive.  A later "tx sa N on" then hits the tx_sa->active ==
ctx_tx_sa->active early return and reports success without installing a rule,
and a later "off" returns -EINVAL with the driver flag already flipped to
inactive.  Should tx_sa->active only be updated once the rule work has
succeeded?

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