Re: [PATCH net 3/7] net/mlx5e: tc: Tie esw & accel blocking refs to the flow's lifetime

[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 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 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_tc_del_flow()` (en_tc.c:5396-5403, the
>   `rhashtable_free_and_destroy()` callback used by…
> 
> Pre-existing issues:
> - [Medium] The TC/IPsec mutual-exclusion counters `mdev-
> >num_block_tc` /
>   `mdev->num_block_ipsec` are plain `u64` fields…
> 
> --
> 
> net/mlx5e: tc: Tie esw & accel blocking refs to the flow's lifetime
> 
> This moves the eswitch user reference and the IPsec-blocking
> reference
> acquisition from mlx5e_configure_flower() into mlx5e_alloc_flow(),
> and moves
> the matching release from mlx5e_delete_flower() into
> mlx5e_tc_del_flow() so
> bulk flow destruction also releases them. A couple of questions below
> about
> the new release site.
> 
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > index b290beb4369ad..44fc421e7b8c1 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> 
> [ ... ]
> 
> > @@ -2158,13 +2162,16 @@ static void
> > mlx5e_tc_del_fdb_peers_flow(struct mlx5e_tc_flow *flow)
> >  static void mlx5e_tc_del_flow(struct mlx5e_priv *priv,
> >  			      struct mlx5e_tc_flow *flow)
> >  {
> > +	struct net_device *filter_dev = flow->attr->parse_attr-
> > >filter_dev;
> > +	bool peer = flow_flag_test(flow, PEER);
> > +
> >  	if (mlx5e_is_eswitch_flow(flow)) {
> >  		struct mlx5_devcom_comp_dev *devcom = flow->priv-
> > >mdev->priv.eswitch->devcom;
> >  
> > -		if (flow_flag_test(flow, PEER) ||
> > +		if (peer ||
> >  		    !mlx5_devcom_for_each_peer_begin(devcom)) {
> >  			mlx5e_tc_del_fdb_flow(priv, flow);
> > -			return;
> > +			goto out;
> >  		}
> >  
> >  		mlx5e_tc_del_fdb_peers_flow(flow);
> > @@ -2173,6 +2180,11 @@ static void
> > mlx5e_tc_del_fdb_peers_flow(struct mlx5e_tc_flow *flow)
> >  	} else {
> >  		mlx5e_tc_del_nic_flow(priv, flow);
> >  	}
> > +out:
> > +	if (!peer) {
> > +		mlx5e_tc_unblock_ipsec_offload(filter_dev, flow-
> > >priv);
> > +		mlx5_esw_put(flow->priv->mdev);
> > +	}
> >  }
> 
> [Severity: High]
> Can this new release pair run twice for the same flow?
> 
> The bulk cleanup callback used by mlx5e_tc_nic_cleanup() and
> mlx5e_tc_ht_cleanup() ignores flow->refcnt entirely:
> 
> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:_mlx5e_tc_del_flow()
> {
> 	struct mlx5e_tc_flow *flow = ptr;
> 	struct mlx5e_priv *priv = flow->priv;
> 
> 	mlx5e_tc_del_flow(priv, flow);
> 	kfree(flow);
> }
> 
> If another context is holding a reference taken with mlx5e_flow_get()
> when
> rhashtable_free_and_destroy() runs this callback, its later
> mlx5e_flow_put() calls mlx5e_tc_del_flow() a second time on the
> already
> freed flow, so mlx5e_tc_unblock_ipsec_offload() and mlx5_esw_put()
> execute
> twice for one acquisition.
> 
> Does that underflow mdev->num_block_ipsec (leaving IPsec packet
> offload
> rejected with -EBUSY until reload), and does the extra mlx5_esw_put()
> consume an esw->user_count belonging to a different, still-live flow,
> re-opening the mode-change-versus-live-rules window that 7dc84de98bab
> closes?
> 
> The window looks structurally open during teardown.
> mlx5e_cleanup_rep_tx()
> destroys the flow hash before the reference-holding contexts are
> quiesced:
> 
> drivers/net/ethernet/mellanox/mlx5/core/en_rep.c:mlx5e_cleanup_rep_tx
> () {
> 	mlx5e_tc_ht_cleanup(&rpriv->tc_ht);
> 
> 	if (rpriv->rep->vport == MLX5_VPORT_UPLINK)
> 		mlx5e_cleanup_uplink_rep_tx(rpriv);
> 
> 	mlx5e_rep_neigh_cleanup(rpriv);
> }
> 
> The netevent notifier unregister, flush_workqueue(priv->wq) and
> cancel_delayed_work_sync(&rpriv->neigh_update.neigh_stats_work) all
> happen
> inside mlx5e_rep_neigh_cleanup(), i.e. after the hash is gone, and
> the FIB
> notifier is unregistered even later via mlx5e_cleanup_uplink_rep_tx()
> ->
> mlx5e_tc_tun_cleanup(). Both of those workers drop their flow
> references
> outside any lock that the cleanup path takes:
> 
> drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c:mlx5e_tc_fi
> b_event_work() {
> 	mutex_unlock(&esw->offloads.encap_tbl_lock);
> 	rtnl_unlock();
> 
> 	mlx5e_put_flow_list(priv, &flow_list);
> }
> 
> mlx5e_tc_update_neigh_used_value() does the same after unlocking
> encap_tbl_lock, and mlx5e_rep_neigh_stats_work() is queued on
> fc_stats->wq
> rather than priv->wq, so the earlier flush_workqueue(priv->wq) in
> mlx5e_detach_netdev() does not cover it.
> 
> Would it be safer for _mlx5e_tc_del_flow() to go through
> mlx5e_flow_put()
> (or for the reservations to be tied to the tc_ht-owned reference),
> and for
> the neigh/FIB work to be quiesced before the flow table is destroyed?

The problem is more complicated than just leaking the new refs and pre-
existing. Also, fixing the lack of reference counting on hash table
destroy is non-trivial. I tried sketching out a fix and it very much
looks like net-next material, so I will address it separately there.

> 
> [ ... ]
> 
> > @@ -4497,11 +4510,23 @@ mlx5e_alloc_flow(struct mlx5e_priv *priv,
> > int attr_size,
> >  	init_completion(&flow->init_done);
> >  	init_completion(&flow->del_hw_done);
> >  
> > +	parse_attr->filter_dev = filter_dev;
> > +	attr->parse_attr = parse_attr;
> > +	/* Non-peer flows own the reservations until final
> > destruction. */
> > +	if (!flow_flag_test(flow, PEER)) {
> > +		err = mlx5e_tc_block_ipsec_offload(filter_dev,
> > priv);
> > +		if (err)
> > +			goto err_free_attr;
> > +		mlx5_esw_get(priv->mdev);
> > +	}
> > +
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but it does widen the set
> of
> unsynchronized contexts, so it seems worth asking: what serializes
> the
> TC/IPsec exclusion counters now?
> 
> At this commit num_block_tc and num_block_ipsec are plain u64 fields
> in
> struct mlx5_core_dev, and mlx5e_tc_block_ipsec_offload() does a
> check-then-increment with no lock of its own:
> 
> 	if (mdev->num_block_tc)
> 		return -EBUSY;
> 
> 	mdev->num_block_ipsec++;
> 
> On this path the only thing held is the eswitch mode_lock read side
> taken by
> mlx5_esw_hold() in mlx5e_configure_flower(), which does not exclude a
> concurrent TC add on another netdev of the same mdev. The decrement
> side is
> now reached from mlx5e_tc_del_flow() with no eswitch lock at all,
> including
> from bulk cleanup and from mlx5e_tc_fib_event_work() /
> mlx5e_tc_update_neigh_used_value() after they drop rtnl and
> encap_tbl_lock.
> 
> The peer side in mlx5e_ipsec_block_tc_offload() reads num_block_ipsec
> and
> increments num_block_tc under mlx5_esw_lock(), and only when an
> eswitch
> exists.
> 
> Can a lost update here leave num_block_ipsec permanently non-zero
> (IPsec
> offload stuck at -EBUSY) or prematurely zero (IPsec offload accepted
> while
> TC rules exist), i.e. break the invariant of c8e350e62fc5 in either
> direction? On 32-bit the u64 read-modify-write can also tear.
> 
> A later patch in the series, "net/mlx5e: Serialize TC and IPsec
> offload
> exclusion counters", moves both counters into mdev->offload_block and
> wraps
> every check, increment and decrement in a dedicated mutex. Would it
> make
> sense to order that patch before this one, since this one adds new
> unlocked decrement contexts?

Makes sense, will do.

Cosmin.