DPDK patches and discussions
 help / color / mirror / Atom feed
From: Dariusz Sosnowski <dsosnowski@nvidia.com>
To: Viacheslav Ovsiienko <viacheslavo@nvidia.com>,
	Bing Zhao <bingz@nvidia.com>, Ori Kam <orika@nvidia.com>,
	Suanming Mou <suanmingm@nvidia.com>,
	Matan Azrad <matan@nvidia.com>
Cc: Raslan Darawsheh <rasland@nvidia.com>, <dev@dpdk.org>
Subject: [PATCH 1/2] net/mlx5: fix dangling pointer to params
Date: Wed, 30 Oct 2024 17:30:45 +0100	[thread overview]
Message-ID: <20241030163046.495982-2-dsosnowski@nvidia.com> (raw)
In-Reply-To: <20241030163046.495982-1-dsosnowski@nvidia.com>

Offending commits introduced 2 structures which held temporary
parameters for flow rule created. These parameters were passed
to "rule_acts" buffers stored inside template table and
they were used only during a call to mlx5dr_rule_create().
After mlx5dr_rule_create() finished, "rule_acts" buffer is not cleared,
for performance reasons, since on the next flow rule create
it will be overwritten.

GCC 14.2.1, when compiled with -03, issued the following warning:

```
drivers/net/mlx5/mlx5_flow_hw.c:3520:51:
    error: storing the address of local variable
      'ap' in '*<unknown>.<U2688>.modify_header.data'
      [-Werror=dangling-pointer=]
 3520 |                 rule_acts[pos].modify_header.data =
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
 3521 |                                         (uint8_t *)ap->mhdr_cmd;
      |                                         ~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/mlx5/mlx5_flow_hw.c: In function 'flow_hw_async_flow_create':
drivers/net/mlx5/mlx5_flow_hw.c:3925:43: note: 'ap' declared here
 3925 |         struct mlx5_flow_hw_action_params ap;
      |                                           ^~
drivers/net/mlx5/mlx5_flow_hw.c:3910:59: note: 'table' declared here
 3910 |                           struct rte_flow_template_table *table,
      |                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~
```

Which is technically correct. This warning could be fixed by
casting "ap->mhdr_cmd" to "void *", but this is just a workaround.

Since what mlx5 PMD cares about is that there is only one instance
of mlx5_flow_hw_action_params per queue, this patch moves this struct
from the stack to mlx5_hw_q struct.
The same is done for mlx5_flow_hw_pattern_params for similar reasons.

Fixes: 1d2744f5028d ("net/mlx5: remove flow action parameters from job")
Fixes: 57fd15fa373d ("net/mlx5: remove flow pattern from job")

Signed-off-by: Dariusz Sosnowski <dsosnowski@nvidia.com>
Acked-by: Ori Kam <orika@nvidia.com>
---
 drivers/net/mlx5/mlx5.h         | 28 ++++++++++++++++++++++++++++
 drivers/net/mlx5/mlx5_flow.h    | 25 -------------------------
 drivers/net/mlx5/mlx5_flow_hw.c |  9 +++------
 3 files changed, 31 insertions(+), 31 deletions(-)

diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index 503366580b..bc25d72c0e 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -425,6 +425,30 @@ enum mlx5_hw_indirect_type {
 
 #define MLX5_HW_MAX_ITEMS (16)
 
+#define MLX5_MHDR_MAX_CMD ((MLX5_MAX_MODIFY_NUM) * 2 + 1)
+#define MLX5_PUSH_MAX_LEN 128
+#define MLX5_ENCAP_MAX_LEN 132
+
+/** Container for flow action data constructed during flow rule creation. */
+struct mlx5_flow_hw_action_params {
+	/** Array of constructed modify header commands. */
+	struct mlx5_modification_cmd mhdr_cmd[MLX5_MHDR_MAX_CMD];
+	/** Constructed encap/decap data buffer. */
+	uint8_t encap_data[MLX5_ENCAP_MAX_LEN];
+	/** Constructed IPv6 routing data buffer. */
+	uint8_t ipv6_push_data[MLX5_PUSH_MAX_LEN];
+};
+
+/** Container for dynamically generated flow items used during flow rule creation. */
+struct mlx5_flow_hw_pattern_params {
+	/** Array of dynamically generated flow items. */
+	struct rte_flow_item items[MLX5_HW_MAX_ITEMS];
+	/** Temporary REPRESENTED_PORT item generated by PMD. */
+	struct rte_flow_item_ethdev port_spec;
+	/** Temporary TAG item generated by PMD. */
+	struct rte_flow_item_tag tag_spec;
+};
+
 /* HW steering flow management job descriptor. */
 struct mlx5_hw_q_job {
 	uint32_t type; /* Job type. */
@@ -449,6 +473,10 @@ struct __rte_cache_aligned mlx5_hw_q {
 	struct rte_ring *indir_iq; /* Indirect action SW in progress queue. */
 	struct rte_ring *flow_transfer_pending;
 	struct rte_ring *flow_transfer_completed;
+	/* Action's ARGUMENT resource buffer for rule creation. */
+	struct mlx5_flow_hw_action_params ap;
+	/* Holds spec value for any implicitly added item. */
+	struct mlx5_flow_hw_pattern_params pp;
 };
 
 
diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
index 693e07218d..d4d985fa3b 100644
--- a/drivers/net/mlx5/mlx5_flow.h
+++ b/drivers/net/mlx5/mlx5_flow.h
@@ -646,9 +646,6 @@ struct mlx5_flow_dv_matcher {
 	struct mlx5_flow_dv_match_params mask; /**< Matcher mask. */
 };
 
-#define MLX5_PUSH_MAX_LEN 128
-#define MLX5_ENCAP_MAX_LEN 132
-
 /* Encap/decap resource structure. */
 struct mlx5_flow_dv_encap_decap_resource {
 	struct mlx5_list_entry entry;
@@ -1420,28 +1417,6 @@ typedef int
 			    const struct rte_flow_action *,
 			    struct mlx5dr_rule_action *);
 
-#define MLX5_MHDR_MAX_CMD ((MLX5_MAX_MODIFY_NUM) * 2 + 1)
-
-/** Container for flow action data constructed during flow rule creation. */
-struct mlx5_flow_hw_action_params {
-	/** Array of constructed modify header commands. */
-	struct mlx5_modification_cmd mhdr_cmd[MLX5_MHDR_MAX_CMD];
-	/** Constructed encap/decap data buffer. */
-	uint8_t encap_data[MLX5_ENCAP_MAX_LEN];
-	/** Constructed IPv6 routing data buffer. */
-	uint8_t ipv6_push_data[MLX5_PUSH_MAX_LEN];
-};
-
-/** Container for dynamically generated flow items used during flow rule creation. */
-struct mlx5_flow_hw_pattern_params {
-	/** Array of dynamically generated flow items. */
-	struct rte_flow_item items[MLX5_HW_MAX_ITEMS];
-	/** Temporary REPRESENTED_PORT item generated by PMD. */
-	struct rte_flow_item_ethdev port_spec;
-	/** Temporary TAG item generated by PMD. */
-	struct rte_flow_item_tag tag_spec;
-};
-
 /* rte flow action translate to DR action struct. */
 struct mlx5_action_construct_data {
 	LIST_ENTRY(mlx5_action_construct_data) next;
diff --git a/drivers/net/mlx5/mlx5_flow_hw.c b/drivers/net/mlx5/mlx5_flow_hw.c
index 2a9ef71cd8..88d6859bac 100644
--- a/drivers/net/mlx5/mlx5_flow_hw.c
+++ b/drivers/net/mlx5/mlx5_flow_hw.c
@@ -3978,8 +3978,6 @@ flow_hw_async_flow_create_generic(struct rte_eth_dev *dev,
 		.burst = attr->postpone,
 	};
 	struct mlx5dr_rule_action *rule_acts;
-	struct mlx5_flow_hw_action_params ap;
-	struct mlx5_flow_hw_pattern_params pp;
 	struct rte_flow_hw *flow = NULL;
 	const struct rte_flow_item *rule_items;
 	uint32_t flow_idx = 0;
@@ -4035,14 +4033,14 @@ flow_hw_async_flow_create_generic(struct rte_eth_dev *dev,
 	 * No need to copy and contrust a new "actions" list based on the
 	 * user's input, in order to save the cost.
 	 */
-	if (flow_hw_actions_construct(dev, flow, &ap,
+	if (flow_hw_actions_construct(dev, flow, &priv->hw_q[queue].ap,
 				      &table->ats[action_template_index],
 				      table->its[pattern_template_index]->item_flags,
 				      flow->table, actions,
 				      rule_acts, queue, error))
 		goto error;
 	rule_items = flow_hw_get_rule_items(dev, table, items,
-					    pattern_template_index, &pp);
+					    pattern_template_index, &priv->hw_q[queue].pp);
 	if (!rule_items)
 		goto error;
 	if (likely(!rte_flow_template_table_resizable(dev->data->port_id, &table->cfg.attr))) {
@@ -4185,7 +4183,6 @@ flow_hw_async_flow_update(struct rte_eth_dev *dev,
 		.burst = attr->postpone,
 	};
 	struct mlx5dr_rule_action *rule_acts;
-	struct mlx5_flow_hw_action_params ap;
 	struct rte_flow_hw *of = (struct rte_flow_hw *)flow;
 	struct rte_flow_hw *nf;
 	struct rte_flow_hw_aux *aux;
@@ -4236,7 +4233,7 @@ flow_hw_async_flow_update(struct rte_eth_dev *dev,
 	 * No need to copy and contrust a new "actions" list based on the
 	 * user's input, in order to save the cost.
 	 */
-	if (flow_hw_actions_construct(dev, nf, &ap,
+	if (flow_hw_actions_construct(dev, nf, &priv->hw_q[queue].ap,
 				      &table->ats[action_template_index],
 				      table->its[nf->mt_idx]->item_flags,
 				      table, actions,
-- 
2.39.5


  reply	other threads:[~2024-10-30 16:31 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-10-30 16:30 [PATCH 0/2] net/mlx5: fix counter query loop getting stuck Dariusz Sosnowski
2024-10-30 16:30 ` Dariusz Sosnowski [this message]
2024-10-30 16:30 ` [PATCH 2/2] " Dariusz Sosnowski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20241030163046.495982-2-dsosnowski@nvidia.com \
    --to=dsosnowski@nvidia.com \
    --cc=bingz@nvidia.com \
    --cc=dev@dpdk.org \
    --cc=matan@nvidia.com \
    --cc=orika@nvidia.com \
    --cc=rasland@nvidia.com \
    --cc=suanmingm@nvidia.com \
    --cc=viacheslavo@nvidia.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).