DPDK patches and discussions
 help / color / mirror / Atom feed
From: Stephen Hemminger <stephen@networkplumber.org>
To: dev@dpdk.org
Cc: Stephen Hemminger <stephen@networkplumber.org>,
	Olga Shern <olgas@nvidia.com>,
	Keith Wiles <keith.wiles@intel.com>,
	Pascal Mazon <pascal.mazon@6wind.com>,
	Kevin Traynor <ktraynor@redhat.com>,
	Ferruh Yigit <ferruh.yigit@intel.com>
Subject: [PATCH 3/3] net/tap: compute TC handle correctly
Date: Thu, 29 Feb 2024 09:31:08 -0800	[thread overview]
Message-ID: <20240229173154.121491-4-stephen@networkplumber.org> (raw)
In-Reply-To: <20240229173154.121491-1-stephen@networkplumber.org>

The code to take a flow pointer and make a TC handle was incorrect
and would always generate the same handle. This is because it was
hashing the address of the union on the stack (which is invariant)
rather than the contents of the union.

The following testpmd case would cause an error:
testpmd> flow create 0 ingress pattern eth src is 06:05:04:03:02:01 \
  / end actions queue index 2 / end
Flow rule #0 created
testpmd> flow create 0 ingress pattern eth src is 06:05:04:03:02:02 \
  / end actions queue index 3 / end
tap_nl_dump_ext_ack(): Filter already exists
tap_flow_create(): Kernel refused TC filter rule creation (17): File exists
port_flow_complain(): Caught PMD error type 2 (flow rule (handle)):
  overlapping rules or Kernel too old for flower support: File exists

This fix does it in a more robust manner using size independent
code. It also initializes the hash seed so the same hash won't
show up every time and risk potential leakage of address to
other places.

Bugzilla ID: 1382
Fixes: de96fe68ae95 ("net/tap: add basic flow API patterns and actions")
Fixes: a625ab89df11 ("net/tap: fix build with GCC 11")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/tap/tap_flow.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
index 5b0fee906493..94f9b99cdaa6 100644
--- a/drivers/net/tap/tap_flow.c
+++ b/drivers/net/tap/tap_flow.c
@@ -11,6 +11,7 @@
 
 #include <rte_byteorder.h>
 #include <rte_jhash.h>
+#include <rte_random.h>
 #include <rte_malloc.h>
 #include <rte_eth_tap.h>
 #include <tap_flow.h>
@@ -1297,9 +1298,7 @@ tap_flow_validate(struct rte_eth_dev *dev,
  * In those rules, the handle (uint32_t) is the part that would identify
  * specifically each rule.
  *
- * On 32-bit architectures, the handle can simply be the flow's pointer address.
- * On 64-bit architectures, we rely on jhash(flow) to find a (sufficiently)
- * unique handle.
+ * Use jhash of the flow poitner to make a unique handle.
  *
  * @param[in, out] flow
  *   The flow that needs its handle set.
@@ -1309,16 +1308,18 @@ tap_flow_set_handle(struct rte_flow *flow)
 {
 	union {
 		struct rte_flow *flow;
-		const void *key;
-	} tmp;
-	uint32_t handle = 0;
+		uint32_t words[sizeof(flow) / sizeof(uint32_t)];
+	} tmp = {
+		.flow = flow,
+	};
+	uint32_t handle;
+	static uint64_t hash_seed;
 
-	tmp.flow = flow;
+	if (hash_seed == 0)
+		hash_seed = rte_rand();
+
+	handle = rte_jhash_32b(tmp.words, sizeof(flow) / sizeof(uint32_t), hash_seed);
 
-	if (sizeof(flow) > 4)
-		handle = rte_jhash(tmp.key, sizeof(flow), 1);
-	else
-		handle = (uintptr_t)flow;
 	/* must be at least 1 to avoid letting the kernel choose one for us */
 	if (!handle)
 		handle = 1;
-- 
2.43.0


  parent reply	other threads:[~2024-02-29 17:32 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-02-29 17:31 [PATCH 0/3] TAP device fixes Stephen Hemminger
2024-02-29 17:31 ` [PATCH 1/3] MAINTAINERS: add maintainer for TAP device Stephen Hemminger
2024-03-04 16:03   ` Ferruh Yigit
2024-02-29 17:31 ` [PATCH 2/3] net/tap: do not overwrite rte_flow errors Stephen Hemminger
2024-02-29 17:31 ` Stephen Hemminger [this message]
2024-03-04 17:45 ` [PATCH 0/3] TAP device fixes Ferruh Yigit

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=20240229173154.121491-4-stephen@networkplumber.org \
    --to=stephen@networkplumber.org \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=keith.wiles@intel.com \
    --cc=ktraynor@redhat.com \
    --cc=olgas@nvidia.com \
    --cc=pascal.mazon@6wind.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).