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

[netdev-bot+sashiko@kernel.org]

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