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

[Cosmin Ratiu]

On Mon, 2026-09-21 at 18:49 +0000, netdev-bot+sashiko@xxxxxxxxxx wrote:
> 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?

Seems serious enough, will address (two new patches required).

Cosmin.