|SUCCESS| net/mlx5: prepend implicit items in sync flow creation path

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163004

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-14

I'll review this DPDK patch for correctness bugs, coding style compliance, and guideline violations.

commit d05cd2075c

Author: Maxime Peim <maxime.peim at gmail.com>

net/mlx5: prepend implicit items in sync flow creation path

In eSwitch mode, the async (template) flow creation path automatically
prepends implicit match items to scope flow rules to the correct
representor port:
  - Ingress: REPRESENTED_PORT item matching dev->data->port_id
  - Egress: REG_C_0 TAG item matching the port's tx tag value

The sync path (flow_hw_list_create) was missing this logic, causing all
flow rules created via the non-template API to match traffic from all
ports rather than being scoped to the specific representor.

Add the same implicit item prepending to flow_hw_list_create, right
after pattern validation and before any branching (sample/RSS/single/
prefix), mirroring the behavior of flow_hw_pattern_template_create
and flow_hw_get_rule_items. The ingress case prepends
REPRESENTED_PORT with the current port_id; the egress case prepends
MLX5_RTE_FLOW_ITEM_TYPE_TAG with REG_C_0 value/mask (skipped when
user provides an explicit SQ item).

Also fix a pre-existing bug where 'return split' on metadata split
failure returned a negative int cast to uintptr_t, which callers
would treat as a valid flow handle instead of an error.

> @@ -14282,6 +14283,28 @@ static uintptr_t flow_hw_list_create(struct rte_eth_dev *dev,
>  			.actions = actions,
>  		},
>  	};
> +	struct rte_flow_item *prepend_items = NULL;
> +	struct rte_flow_item_ethdev port_spec = {.port_id = dev->data->port_id};
> +	struct rte_flow_item port = {
> +		.type = RTE_FLOW_ITEM_TYPE_REPRESENTED_PORT,
> +		.spec = &port_spec,
> +		.mask = &rte_flow_item_ethdev_mask,
> +	};
> +	struct mlx5_rte_flow_item_tag tag_v = {
> +		.id = REG_C_0,
> +		.data = flow_hw_tx_tag_regc_value(dev),
> +	};
> +	struct mlx5_rte_flow_item_tag tag_m = {
> +		.id = REG_C_0,
> +		.data = flow_hw_tx_tag_regc_mask(dev),
> +	};
> +	struct rte_flow_item tag = {
> +		.type = (enum rte_flow_item_type)MLX5_RTE_FLOW_ITEM_TYPE_TAG,
> +		.spec = &tag_v,
> +		.mask = &tag_m,
> +		.last = NULL,
> +	};
> +	uint32_t nb_items;

The local variables port_spec, port, tag_v, tag_m, and tag contain
pointers to stack memory (&port_spec, &tag_v, &tag_m). Later in the code
these are copied via flow_hw_prepend_item(), but does the prepend_items
array retain these pointers to stack-allocated data? If
flow_hw_prepend_item() performs a shallow copy, then setting items =
prepend_items and resource.suffix.items = items would result in dangling
pointers when flow_hw_list_create() returns, as the stack frame is
destroyed.

Does flow_hw_prepend_item() deep-copy the spec/mask pointers, or does it
retain the addresses passed in the item structures?

[ ... ]

> @@ -14296,13 +14319,50 @@ static uintptr_t flow_hw_list_create(struct rte_eth_dev *dev,
>  	if (ret < 0)
>  		return 0;
>  
> +	nb_items = ret;
> +
> +	if (priv->sh->config.dv_esw_en &&
> +	    attr->ingress && !attr->egress && !attr->transfer) {
> +		prepend_items = flow_hw_prepend_item(items, nb_items,
> +						     &port, error);
> +		if (!prepend_items)
> +			return 0;
> +		items = prepend_items;
> +		resource.suffix.items = items;
> +	} else if (priv->sh->config.dv_esw_en &&
> +		   !attr->ingress && attr->egress && !attr->transfer) {
> +		if (item_flags & MLX5_FLOW_ITEM_SQ) {
> +			DRV_LOG(DEBUG,
> +				"Port %u omitting implicit REG_C_0 match for egress "
> +				"pattern template",
> +				dev->data->port_id);
> +			goto setup_pattern;
> +		}

Does this goto setup_pattern leave prepend_items as NULL in the case
where item_flags has MLX5_FLOW_ITEM_SQ set? If so, the subsequent code
paths do not free prepend_items (it is NULL), but items and
resource.suffix.items remain pointing to the original items array. Is
this intentional behavior?

> +		prepend_items = flow_hw_prepend_item(items, nb_items,
> +						     &tag, error);
> +		if (!prepend_items)
> +			return 0;
> +		items = prepend_items;
> +		resource.suffix.items = items;
> +	}
> +setup_pattern:
>  	RTE_SET_USED(encap_idx);
>  	if (!error)
>  		error = &shadow_error;
>  	split = mlx5_flow_nta_split_metadata(dev, attr, actions, qrss, action_flags,
>  					     actions_n, external, &resource, error);
> -	if (split < 0)
> -		return split;
> +	if (split < 0) {
> +		mlx5_free(prepend_items);
> +		return 0;
> +	}

The fix for the pre-existing bug is correct (returning 0 instead of
casting negative split to uintptr_t). However, is the cleanup complete
here? Does mlx5_flow_nta_split_metadata() modify any state in resource
that needs cleanup on this error path?

[ ... ]

> @@ -14315,8 +14375,10 @@ static uintptr_t flow_hw_list_create(struct rte_eth_dev *dev,
>  	if (action_flags & MLX5_FLOW_ACTION_SAMPLE) {
>  		flow = mlx5_nta_sample_flow_list_create(dev, type, attr, items, actions,
>  							item_flags, action_flags, error);
> -		if (flow != NULL)
> +		if (flow != NULL) {
> +			mlx5_free(prepend_items);
>  			return (uintptr_t)flow;
> +		}

Does mlx5_nta_sample_flow_list_create() take ownership of the items
pointer passed to it? If it stores items internally and later accesses
it, then freeing prepend_items here would create a use-after-free when
the sample flow attempts to reference the pattern items.

Similar question for mlx5_nta_rss_flow_list_create() and
mlx5_nta_flow_list_create() below - do these functions copy or take
ownership of the items array?

[ ... ]

> @@ -14328,8 +14390,10 @@ static uintptr_t flow_hw_list_create(struct rte_eth_dev *dev,
>  		if (flow) {
>  			flow->nt2hws->rix_mreg_copy = cpy_idx;
>  			cpy_idx = 0;
> -			if (!split)
> +			if (!split) {
> +				mlx5_free(prepend_items);
>  				return (uintptr_t)flow;
> +			}

When split is true, execution continues to prefix_flow. Does this mean
prepend_items is leaked in the split=true case, or is it cleaned up
later at the success path after prefix_flow creation?

> @@ -14343,8 +14407,10 @@ static uintptr_t flow_hw_list_create(struct rte_eth_dev *dev,
>  	if (flow) {
>  		flow->nt2hws->rix_mreg_copy = cpy_idx;
>  		cpy_idx = 0;
> -		if (!split)
> +		if (!split) {
> +			mlx5_free(prepend_items);
>  			return (uintptr_t)flow;
> +		}

Same question as above regarding cleanup in the split=true case.

> @@ -14357,6 +14423,7 @@ static uintptr_t flow_hw_list_create(struct rte_eth_dev *dev,
>  		flow->nt2hws->chaned_flow = 1;
>  		SLIST_INSERT_AFTER(prfx_flow, flow, nt2hws->next);
>  		mlx5_flow_nta_split_resource_free(dev, &resource);
> +		mlx5_free(prepend_items);
>  		return (uintptr_t)prfx_flow;
>  	}

This is the success path after prefix flow creation - prepend_items is
correctly freed here.

> @@ -14368,6 +14435,7 @@ static uintptr_t flow_hw_list_create(struct rte_eth_dev *dev,
>  		mlx5_flow_nta_del_copy_action(dev, cpy_idx);
>  	if (split > 0)
>  		mlx5_flow_nta_split_resource_free(dev, &resource);
> +	mlx5_free(prepend_items);
>  	return 0;
>  }

And this is the error cleanup path - prepend_items is correctly freed
here as well.

However, there is still the question about whether the created flows take
ownership of or reference the items array. If they do, then freeing
prepend_items on success paths would cause issues.