[PATCH] net/mlx5e: TC, fix internal port leak when a flow is reoffloaded

[PATCH] net/mlx5e: TC, fix internal port leak when a flow is reoffloaded

[Wentao Liang]

For a flow originating from an OVS internal port,
mlx5e_tc_add_fdb_flow() takes a reference on the internal port and stores
it in the flow's esw attribute.

If the flow cannot be offloaded because the encap route is not available
(-ENETUNREACH) on a multipath LAG setup, __mlx5e_add_fdb_flow() keeps the
flow on the unready list instead of freeing it. When
mlx5e_tc_reoffload_flows_work() retries, mlx5e_tc_add_fdb_flow() takes
another reference and overwrites the stored pointer, so the previous
reference is never dropped.

Only look up the internal port if the flow does not hold a reference
already; the reference is released together with the flow attributes.

Fixes: 166f431ec6be ("net/mlx5e: Add indirect tc offload of ovs internal port")
Cc: stable@vger.kernel.org
Assisted-by: DeepSeek-V4.1-Flash and OpenAI Codex
Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
---
 .../net/ethernet/mellanox/mlx5/core/en_tc.c   | 22 ++++++++++---------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index 17bd92529a44..8246378b089f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -2005,17 +2005,19 @@ mlx5e_tc_add_fdb_flow(struct mlx5e_priv *priv,
 			goto err_out;
 		}
 
-		int_port = mlx5e_tc_int_port_get(mlx5e_get_int_port_priv(priv),
-						 parse_attr->filter_dev->ifindex,
-						 flow_flag_test(flow, EGRESS) ?
-						 MLX5E_TC_INT_PORT_EGRESS :
-						 MLX5E_TC_INT_PORT_INGRESS);
-		if (IS_ERR(int_port)) {
-			err = PTR_ERR(int_port);
-			goto err_out;
-		}
+		if (!esw_attr->int_port) {
+			int_port = mlx5e_tc_int_port_get(mlx5e_get_int_port_priv(priv),
+							 parse_attr->filter_dev->ifindex,
+							 flow_flag_test(flow, EGRESS) ?
+							 MLX5E_TC_INT_PORT_EGRESS :
+							 MLX5E_TC_INT_PORT_INGRESS);
+			if (IS_ERR(int_port)) {
+				err = PTR_ERR(int_port);
+				goto err_out;
+			}
 
-		esw_attr->int_port = int_port;
+			esw_attr->int_port = int_port;
+		}
 	}
 
 	err = post_process_attr(flow, attr, extack);
-- 
2.34.1

Re: [PATCH] net/mlx5e: TC, fix internal port leak when a flow is reoffloaded

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH] net/mlx5e: TC, fix internal port leak when a flow is reoffloaded

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 (1) · Medium: 0 · 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] The fix is incomplete: internal-port references still leak when
  a flow is reoffloaded, through other writers in the same retry path…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  mlx5e_tc_add_fdb_flow() does not undo the resources it attached when…

--

net/mlx5e: TC, fix internal port leak when a flow is reoffloaded

This patch makes mlx5e_tc_add_fdb_flow() skip the internal port lookup
for OVS master filter devices when esw_attr->int_port is already set. The
goal is that a retry from mlx5e_tc_reoffload_flows_work() does not take a
second reference and overwrite the stored pointer.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> index 708b7987ea4cd..fba4b7b97d2e4 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> @@ -1998,17 +1998,19 @@ mlx5e_tc_add_fdb_flow(struct mlx5e_priv *priv,
>  			goto err_out;
>  		}
>  
> -		int_port = mlx5e_tc_int_port_get(mlx5e_get_int_port_priv(priv),
> -						 parse_attr->filter_dev->ifindex,
> -						 flow_flag_test(flow, EGRESS) ?
> -						 MLX5E_TC_INT_PORT_EGRESS :
> -						 MLX5E_TC_INT_PORT_INGRESS);
> -		if (IS_ERR(int_port)) {
> -			err = PTR_ERR(int_port);
> -			goto err_out;
> -		}
> +		if (!esw_attr->int_port) {
> +			int_port = mlx5e_tc_int_port_get(mlx5e_get_int_port_priv(priv),
> +							 parse_attr->filter_dev->ifindex,
> +							 flow_flag_test(flow, EGRESS) ?
> +							 MLX5E_TC_INT_PORT_EGRESS :
> +							 MLX5E_TC_INT_PORT_INGRESS);
> +			if (IS_ERR(int_port)) {
> +				err = PTR_ERR(int_port);
> +				goto err_out;
> +			}
>  
> -		esw_attr->int_port = int_port;
> +			esw_attr->int_port = int_port;
> +		}

[Severity: High]
Does this fully close the leak on the reoffload path? The new guard only
covers the writer used when filter_dev is an OVS master. The same retry
path has two other writers that are not guarded.

For a TUN_RX flow, mlx5e_tc_add_fdb_flow() calls
mlx5e_attach_decap_route() before this block. That function always calls
mlx5e_tc_tun_route_lookup():

drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c:mlx5e_tc_tun_route_lookup() {
    ...
	} else if (netif_is_ovs_master(attr.route_dev) && mlx5e_tc_int_port_supported(esw)) {
		int_port = mlx5e_tc_int_port_get(mlx5e_get_int_port_priv(priv),
						 attr.route_dev->ifindex,
						 MLX5E_TC_INT_PORT_INGRESS);
		...
		esw_attr->int_port = int_port;
	}
    ...
}

On that branch decap_vport stays 0, so mlx5e_attach_decap_route()
returns 0 and the flow continues. Suppose a later encap lookup in
post_process_attr() fails with -ENETUNREACH. __mlx5e_add_fdb_flow() then
keeps the flow on the unready list. Each run of
mlx5e_tc_reoffload_flows_work() takes another int_port reference here and
overwrites the previous one.

For TUN_RX flows filter_dev is the tunnel netdev, so the guard added by
this patch never runs for this case.

In multipath mode, mlx5e_route_lookup_ipv4_get() forces flowi4_oif to the
uplink rep. mlx5e_route_lookup_ipv6_get() does not, so an IPv6 decap
route through an OVS internal port such as br-phy seems to reach this
path.

The encap side does the same thing with dest_int_port. Take several
encap destinations where dest 0 attaches and a later one fails with
-ENETUNREACH. The retry goes through:

post_process_attr()
  mlx5e_tc_tun_encap_dests_set()
    mlx5e_attach_encap()
      mlx5e_set_int_port_tunnel()
        mlx5e_set_fwd_to_int_port_actions()

mlx5e_set_fwd_to_int_port_actions() then does:

	dest_int_port = mlx5e_tc_int_port_get(int_port_priv, ifindex, type);
	...
	esw_attr->dest_int_port = dest_int_port;

It does not check or release an existing value. is_duplicated_encap_entry()
only looks at indices below out_index, so it does not catch this case
either.

mlx5_free_flow_attr_actions() puts only the last stored int_port and
dest_int_port. The earlier references keep the int port, its rx rule and
its metadata mappings alive until the driver is torn down.

The commit message says "the reference is released together with the
flow attributes". That only holds for the writer guarded here.

Could the writers in mlx5e_tc_tun_route_lookup() and
mlx5e_set_fwd_to_int_port_actions() be handled as well? Another option
would be to unwind the partial state before the flow is retried.

>  	}
>  
>  	err = post_process_attr(flow, attr, extack);

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. The
int_port leak looks like one symptom of mlx5e_tc_add_fdb_flow() not being
idempotent across reoffload retries. When it fails with -ENETUNREACH,
nothing attached before the failure is undone. mlx5e_tc_reoffload_flows_work()
then runs it again on the same flow.

Several of those attach steps take a reference and list_add() a node that
is still linked from the previous attempt.

mlx5e_attach_decap_route(), for a VF-tunnel TUN_RX flow:

	flow->decap_route = r;
	list_add(&flow->decap_routes, &r->decap_flows);

mlx5e_attach_decap(), for L3_TO_L2_DECAP:

	flow->decap_reformat = d;
	attr->decap_pkt_reformat = d->pkt_reformat;
	list_add(&flow->l3_to_l2_reformat, &d->flows);

mlx5e_attach_encap(), for an encap dest that attached in an earlier
attempt:

	flow->encaps[out_index].e = e;
	list_add(&flow->encaps[out_index].list, &e->flows);

In the encap case, mlx5e_encap_get() bumps the entry refcount again.
is_duplicated_encap_entry() only checks indices below out_index, so it
misses the earlier attachment at the same index.

Can the repeated list_add() corrupt e->flows, r->decap_flows or d->flows?
With CONFIG_DEBUG_LIST it is reported as a double add. Without it, the
list_del() at flow teardown seems to leave the list pointing at the freed
flow. The next neigh or route update that walks the list would then be a
use-after-free.

The extra references on r, d and e are never dropped either, because
teardown puts each one only once.

The VPORT_TO_REG set action in the TUN_RX branch of
mlx5e_tc_add_fdb_flow() is also appended to parse_attr->mod_hdr_acts again
on every retry.

Would unwinding the partially attached state in __mlx5e_add_fdb_flow(),
before the flow goes on the unready list, fix both this and the int_port
leak?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261103102855.1340650-1-vulab%40iscas.ac.cn