Hi,
+Cc mlx5 maintainers
Thank you for the contribution. Please see the comments inline.
On Tue, Jun 24, 2025 at 01:10:15AM -0400, Khadem Ullah wrote:
> This patch fixes a segmentation fault that occurs when querying the
> age action of an indirect flow rule using connection tracking.
>
> Steps to reproduce:
> 1. Create an indirect action:
> flow indirect_action 0 create ingress action conntrack / end
>
> 2. Create a root flow rule with a jump:
> flow create 0 ingress pattern eth / ipv4 / tcp / end /
> actions jump group 3 / end
>
> 3. Create a group 3 rule using the indirect action:
> flow create 0 group 3 ingress pattern eth / ipv4 / tcp / end /
> actions indirect 0 / jump group 5 / end
>
> 4. Create a group 5 rule matching on conntrack state:
> flow create 0 group 5 ingress pattern eth / ipv4 / tcp /
> conntrack is 1 / end actions queue index 5 / end
>
> 5. Querying the first rule causes a segmentation fault:
> flow query 0 1 age
>
> This patch ensures proper handling of the indirect action with
> conntrack to prevent this crash.
Could you please add the following Fixes tag to the commit message?
Fixes: 2d084f69aa26 ("net/mlx5: add translation of connection tracking
action")
This would allow LTS maintainers to pick up the fix for future LTS
releases.
>
> Signed-off-by: Khadem Ullah <14pwcse1...@uetpeshawar.edu.pk>
> ---
> .mailmap | 1 +
> drivers/net/mlx5/mlx5_flow.c | 2 ++
> drivers/net/mlx5/mlx5_flow_dv.c | 5 +++++
> 3 files changed, 8 insertions(+)
>
> diff --git a/.mailmap b/.mailmap
> index 8483d96ec5..5c9ea95346 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -812,6 +812,7 @@ Kevin Scott <kevin.c.sc...@intel.com>
> Kevin Traynor <ktray...@redhat.com>
> Ke Xu <ke1...@intel.com>
> Ke Zhang <ke1x.zh...@intel.com>
> +Khadem Ullah <14pw...@uetpeshawar.edu.pk>
Could you please make sure that the mail address in .mailmap and
Signed-off-by tag match?
> Khoa To <k...@microsoft.com>
> Kiran KN <kira...@juniper.net>
> Kiran Kumar K <kirankum...@marvell.com> <kkokkilaga...@caviumnetworks.com>
> <kiran.kokkilaga...@caviumnetworks.com>
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index 3d49a2d833..5c799ea4ce 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -4550,6 +4550,8 @@ flow_aso_age_get_by_idx(struct rte_eth_dev *dev,
> uint32_t age_idx)
> struct mlx5_aso_age_pool *pool;
>
> rte_rwlock_read_lock(&mng->resize_rwl);
> + if (mng->pools == NULL)
> + return NULL;
It is an interesting case.
When DV flow engine is used (current default),
connection tracking and age action cannot be used together.
Because of that, to optimize memory usage, age and CT index
are placed in a union.
The root cause of the crash is not a lack of ageing pools
(they are not initialized since no flow rule uses age action),
but the fact that age and CT index are in the union.
Since flow rule in repro steps use conntrack action,
flow->age value is misinterpreted.
In this case flow_aso_age_get_by_idx() should not be reached at all.
There's a check missing in flow_dv_query() for AGE action:
flow_dv_query(...) {
for (; actions->type != RTE_FLOW_ACTION_TYPE_END; actions++) {
switch (actions->type) {
/* snip */
case RTE_FLOW_ACTION_TYPE_AGE:
/* missing check; if true, flow->age should not
be read */
if (flow->indirect_type ==
MLX5_INDIRECT_ACTION_TYPE_CT)
return rte_flow_error_set(..., "age not
available");
ret = flow_dv_query_age(dev, flow, data, error);
break;
/* snip */
}
}
This check should resolve the segfault.
Would you be able to test that approach on your side and if all is good
resend the patch?
> pool = mng->pools[pool_idx];
> rte_rwlock_read_unlock(&mng->resize_rwl);
> return &pool->actions[offset - 1];
> diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
> index c217634d9b..f81ce20385 100644
> --- a/drivers/net/mlx5/mlx5_flow_dv.c
> +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> @@ -18086,6 +18086,11 @@ flow_dv_query_age(struct rte_eth_dev *dev, struct
> rte_flow *flow,
> if (flow->age) {
> struct mlx5_aso_age_action *act =
> flow_aso_age_get_by_idx(dev, flow->age);
> + if (!act)
> + return rte_flow_error_set
> + (error, EINVAL,
> + RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> + NULL, "cannot read age data");
>
> age_param = &act->age_params;
> } else if (flow->counter) {
> --
> 2.43.0
>
Best regards,
Dariusz Sosnowski
