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

[netdev-bot+sashiko]

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_fib_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?

[ ... ]

> @@ -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?

>  	*__flow = flow;
>  	*__parse_attr = parse_attr;
>  
>  	return 0;
>  
> +err_free_attr:
> +	kfree(attr);
>  err_free:
>  	kfree(flow);
>  	kvfree(parse_attr);

[ ... ]

> @@ -4865,12 +4888,6 @@ int mlx5e_configure_flower(struct net_device *dev, struct mlx5e_priv *priv,
>  	if (!mlx5_esw_hold(priv->mdev))
>  		return -EBUSY;
>  
> -	err = mlx5e_tc_block_ipsec_offload(dev, priv);
> -	if (err)
> -		goto esw_release;
> -
> -	mlx5_esw_get(priv->mdev);
> -
>  	rcu_read_lock();
>  	flow = rhashtable_lookup(tc_ht, &f->cookie, tc_ht_params);
>  	if (flow) {

[ ... ]

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