Re: [PATCH v2] net/mlx5: fix connection tracking state item validation

[Dariusz Sosnowski <dsosnowski@nvidia.com>]

Hi,

Thank you for the patch. Please see comments inline.

On Tue, Aug 12, 2025 at 08:46:30AM -0400, Khadem Ullah wrote:
> This patch validate a connection tracking state when matching
> 'conntrack is' in rte_flow rules. Since conntrack item flags
> is a bitmap, then any combination of RTE_FLOW_CONNTRACK_PKT_STATE_*
> flags is a valid value to match on.
> 
> This patch validate the CT state item.
> Fixes: aca19061e4b9 ('net/mlx5: validate connection tracking item')

When used locally ./devtool/checkpatches.sh reports the following:

	WARNING:BAD_FIXES_TAG: Please use correct Fixes: style 'Fixes: <12+ chars of sha1> ("<title line>")' - ie: 'Fixes: aca19061e4b9 ("net/mlx5: validate connection tracking item")'
	#12:
	Fixes: aca19061e4b9 ('net/mlx5: validate connection tracking item')

but it's not reported in "ci/checkpatch" job in Patchwork.
This check was added in Linux 6.1: https://github.com/torvalds/linux/commit/bd17e036b495bebbf07a5fc814c868e30e1dc131
so it appears that checkpatch.pl version used in CI is older than that.
Locally, I have a version from Linux 6.16.

Ali, Thomas, David: What do you think about updating the version of
checkpatch.pl used in community CI?

Khadem: Could you please fix the quotations marks in Fixes tag according to the error?

For the future, you can set up a git alias, which can be used
to automatically generate a correct tag.
It is described in: https://doc.dpdk.org/guides/contributing/patches.html#commit-messages-body

> Cc: stable@dpdk.org
> 
> Signed-off-by: Khadem Ullah <14pwcse1224@uetpeshawar.edu.pk>
> ---
>  drivers/net/mlx5/mlx5_flow_dv.c | 12 +++++++++++-
>  drivers/net/mlx5/mlx5_flow_hw.c | 17 ++++++++++++++++-
>  2 files changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
> index 7b9e5018b8..19475b931f 100644
> --- a/drivers/net/mlx5/mlx5_flow_dv.c
> +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> @@ -3271,7 +3271,7 @@ mlx5_flow_dv_validate_item_aso_ct(struct rte_eth_dev *dev,
>  {
>  	const struct rte_flow_item_conntrack *spec = item->spec;
>  	const struct rte_flow_item_conntrack *mask = item->mask;
> -	uint32_t flags;
> +	uint32_t flags, flags_all;
>  
>  	if (*item_flags & MLX5_FLOW_LAYER_ASO_CT)
>  		return rte_flow_error_set(error, EINVAL,
> @@ -3289,6 +3289,16 @@ mlx5_flow_dv_validate_item_aso_ct(struct rte_eth_dev *dev,
>  						  RTE_FLOW_ERROR_TYPE_ITEM,
>  						  NULL,
>  						  "Conflict status bits");
> +		flags_all = (RTE_FLOW_CONNTRACK_PKT_STATE_VALID |
> +				RTE_FLOW_CONNTRACK_PKT_STATE_CHANGED |
> +				RTE_FLOW_CONNTRACK_PKT_STATE_INVALID |
> +				RTE_FLOW_CONNTRACK_PKT_STATE_DISABLED |
> +				RTE_FLOW_CONNTRACK_PKT_STATE_BAD);

I think it would be better to define a macro for "all possible packet state
flags", because now the definition of all flags is repeated in 2 places.
Could you please add MLX5_FLOW_CONNTRACK_PKT_STATE_ALL macro to mlx5_flow.h?

> +		if (spec->flags & ~flags_all)
> +			return rte_flow_error_set(error, EINVAL,
> +					RTE_FLOW_ERROR_TYPE_ITEM,
> +					NULL,
> +					"Invalid CT item matching \n");

Please remove the new line character and preceding space from error message here.
rte_flow_error_set() does not print the message, it only stores
the pointer to error message in rte_flow_error struct.
Users later can print it/log it however they like.

I also propose to change the message itself, so it better pinpoints
the exact error. For example: "Invalid CT item flags".

Please apply the same to the second instance of that error.

>  	}
>  	/* State change also needs to be considered. */
>  	*item_flags |= MLX5_FLOW_LAYER_ASO_CT;
> diff --git a/drivers/net/mlx5/mlx5_flow_hw.c b/drivers/net/mlx5/mlx5_flow_hw.c
> index 6dc16f80d3..6dbbc44819 100644
> --- a/drivers/net/mlx5/mlx5_flow_hw.c
> +++ b/drivers/net/mlx5/mlx5_flow_hw.c
> @@ -16957,7 +16957,6 @@ flow_hw_validate_rule_pattern(struct rte_eth_dev *dev,
>  {
>  	const struct rte_flow_pattern_template *pt;
>  	const struct rte_flow_item *pt_item;
> -

When you'll be sending a new version, could you reapply this empty line?
It's removal is not needed.

> /* snip rest of the diff */

Best regards,
Dariusz Sosnowski