Re: [PATCH] net/mlx5e: TC, release meter if rule offload fails

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net/mlx5e: TC, dangling meter pointers after mlx5e_tc_del_flow_meter()
--

commit b953a898af94c9215dfc36d4dbe7f5054db010fd
Author: Wentao Liang <vulab@iscas.ac.cn>

net/mlx5e: TC, release meter if rule offload fails

This commit cleans up the flow meter and post-meter state in
mlx5e_tc_rule_offload() if mlx5_eswitch_add_offloaded_rule() fails after
mlx5e_tc_add_flow_meter() has succeeded.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> index 708b7987ea4cd..17bd92529a44a 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> @@ -491,6 +491,8 @@ mlx5e_tc_rule_offload(struct mlx5e_priv *priv,
>  		      struct mlx5_flow_attr *attr)
>  {
>  	struct mlx5_eswitch *esw = priv->mdev->priv.eswitch;
> +	struct mlx5_flow_handle *rule;
> +	bool meter_added = false;
>  	int err;
>  
>  	if (!is_mdev_switchdev_mode(priv->mdev))
> @@ -503,9 +505,14 @@ mlx5e_tc_rule_offload(struct mlx5e_priv *priv,
>  		err = mlx5e_tc_add_flow_meter(priv, attr);
>  		if (err)
>  			return ERR_PTR(err);
> +		meter_added = true;
>  	}
>  
> -	return mlx5_eswitch_add_offloaded_rule(esw, spec, attr);
> +	rule = mlx5_eswitch_add_offloaded_rule(esw, spec, attr);
> +	if (IS_ERR(rule) && meter_added)
> +		mlx5e_tc_del_flow_meter(esw, attr);

[Severity: High]
Does calling mlx5e_tc_del_flow_meter() here leave dangling pointers in
attr->meter_attr.meter, attr->meter_attr.post_meter, and attr->dest_ft that
can lead to a use-after-free or double-free when the flow is not immediately
freed?

In drivers/net/ethernet/mellanox/mlx5/core/en_tc.c,
mlx5e_tc_add_flow_meter() populates attr and mlx5e_tc_del_flow_meter()
releases the resources without clearing those fields:

mlx5e_tc_add_flow_meter() {
	...
	attr->meter_attr.meter = meter;
	attr->meter_attr.post_meter = post_meter;
	attr->dest_ft = mlx5e_post_meter_get_ft(post_meter);
	attr->action |= MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;
	return 0;
}

mlx5e_tc_del_flow_meter() {
	mlx5e_post_meter_cleanup(esw, attr->meter_attr.post_meter);
	mlx5e_tc_meter_put(attr->meter_attr.meter);
}

While mlx5e_tc_add_fdb_flow() frees the flow on error, other callers keep
flow->attr alive when mlx5e_tc_offload_fdb_rules() ->
mlx5e_tc_rule_offload() fails.

For example, in drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c,
mlx5e_reoffload_encap() keeps the flow and still sets the OFFLOADED flag:

mlx5e_reoffload_encap() {
	...
			rule = mlx5e_tc_offload_fdb_rules(esw, flow, spec, flow->attr);
			if (IS_ERR(rule)) {
				mlx5e_tc_unoffload_flow_post_acts(flow);
				err = PTR_ERR(rule);
				mlx5_core_warn(priv->mdev, "Failed to update cached encapsulation flow, %d\n",
					       err);
			} else {
				flow->rule[0] = rule;
			}
		} else {
			...
		}
		flow_flag_set(flow, OFFLOADED);
}

When that flow is later deleted or invalidated via
mlx5e_tc_unoffload_fdb_rules() -> mlx5e_tc_rule_unoffload():

mlx5e_tc_rule_unoffload() {
	...
	if (attr->meter_attr.meter)
		mlx5e_tc_del_flow_meter(esw, attr);
}

attr->meter_attr.meter is still non-NULL, so mlx5e_tc_del_flow_meter() is
called a second time on the already-cleaned-up post_meter and meter.

Similarly, if mlx5e_tc_offload_fdb_rules() fails in
mlx5e_tc_encap_flows_add(), the flow remains on the slow path with a
dangling non-NULL flow->attr->dest_ft. When
mlx5e_tc_unoffload_from_slow_path() later copies flow->attr into slow_attr
and calls mlx5e_tc_unoffload_fdb_rules() -> mlx5e_tc_rule_unoffload() ->
mlx5_eswitch_del_offloaded_rule() -> esw_cleanup_dests(), the non-NULL
attr->dest_ft causes esw_cleanup_dests() to take the attr->dest_ft branch on
a slow-path rule.

Should mlx5e_tc_del_flow_meter() (or the error path in
mlx5e_tc_rule_offload()) also clear attr->meter_attr.meter,
attr->meter_attr.post_meter, and attr->dest_ft?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103102856.1340672-1-vulab@iscas.ac.cn?part=1