[PATCH 0/3] TAP device fixes

[PATCH 0/3] TAP device fixes

[Stephen Hemminger]

Add myself as maintainer and fix some bugs found while testing.

Stephen Hemminger (3):
  MAINTAINERS: add maintainer for TAP device
  net/tap: do not overwrite rte_flow errors
  net/tap: compute TC handle correctly

 MAINTAINERS                |  1 +
 drivers/net/tap/tap_flow.c | 44 ++++++++++++++++++++++----------------
 2 files changed, 27 insertions(+), 18 deletions(-)

-- 
2.43.0

[PATCH 1/3] MAINTAINERS: add maintainer for TAP device

[Stephen Hemminger]

Add myself as maintainer for TAP device.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 962c359cdd85..e0959992d998 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1021,6 +1021,7 @@ F: doc/guides/nics/pcap_ring.rst
 F: doc/guides/nics/features/pcap.ini
 
 Tap PMD
+M: Stephen Hemminger <stephen@networkplumber.org>
 F: drivers/net/tap/
 F: doc/guides/nics/tap.rst
 F: doc/guides/nics/features/tap.ini
-- 
2.43.0

[PATCH 2/3] net/tap: do not overwrite rte_flow errors

[Stephen Hemminger]

All flow errors were ending up being reported as not supported,
even when the error path was previously setting a valid and
better error message.

Example, asking for a non-existent queue in flow.

Before:
testpmd> flow create 0 ingress pattern eth src is 06:05:04:03:02:01 \
  / end actions queue index 12 / end
port_flow_complain(): Caught PMD error type 16 (specific action):
cause: 0x7fffc46c1e18, action not supported: Operation not supported

After:
testpmd> flow create 0 ingress pattern eth src is 06:05:04:03:02:01 \
  / end actions queue index 12 / end
port_flow_complain(): Caught PMD error type 16 (specific action):
cause: 0x7fffa54e1d88, queue index out of range: Numerical result out of range

Fixes: f46900d03823 ("net/tap: fix flow and port commands")
Fixes: de96fe68ae95 ("net/tap: add basic flow API patterns and actions")

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/tap/tap_flow.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
index ed4d42f92f9f..5b0fee906493 100644
--- a/drivers/net/tap/tap_flow.c
+++ b/drivers/net/tap/tap_flow.c
@@ -1082,8 +1082,11 @@ priv_flow_process(struct pmd_internals *pmd,
 		}
 		/* use flower filter type */
 		tap_nlattr_add(&flow->msg.nh, TCA_KIND, sizeof("flower"), "flower");
-		if (tap_nlattr_nested_start(&flow->msg, TCA_OPTIONS) < 0)
-			goto exit_item_not_supported;
+		if (tap_nlattr_nested_start(&flow->msg, TCA_OPTIONS) < 0) {
+			rte_flow_error_set(error, ENOMEM, RTE_FLOW_ERROR_TYPE_ACTION,
+					   actions, "could not allocated netlink msg");
+			goto exit_return_error;
+		}
 	}
 	for (; items->type != RTE_FLOW_ITEM_TYPE_END; ++items) {
 		const struct tap_flow_items *token = NULL;
@@ -1199,9 +1202,12 @@ priv_flow_process(struct pmd_internals *pmd,
 			if (action)
 				goto exit_action_not_supported;
 			action = 1;
-			if (!queue ||
-			    (queue->index > pmd->dev->data->nb_rx_queues - 1))
-				goto exit_action_not_supported;
+			if (queue->index >= pmd->dev->data->nb_rx_queues) {
+				rte_flow_error_set(error, ERANGE,
+						   RTE_FLOW_ERROR_TYPE_ACTION, actions,
+						   "queue index out of range");
+				goto exit_return_error;
+			}
 			if (flow) {
 				struct action_data adata = {
 					.id = "skbedit",
@@ -1227,7 +1233,7 @@ priv_flow_process(struct pmd_internals *pmd,
 			if (!pmd->rss_enabled) {
 				err = rss_enable(pmd, attr, error);
 				if (err)
-					goto exit_action_not_supported;
+					goto exit_return_error;
 			}
 			if (flow)
 				err = rss_add_actions(flow, pmd, rss, error);
@@ -1235,7 +1241,7 @@ priv_flow_process(struct pmd_internals *pmd,
 			goto exit_action_not_supported;
 		}
 		if (err)
-			goto exit_action_not_supported;
+			goto exit_return_error;
 	}
 	/* When fate is unknown, drop traffic. */
 	if (!action) {
@@ -1258,6 +1264,7 @@ priv_flow_process(struct pmd_internals *pmd,
 exit_action_not_supported:
 	rte_flow_error_set(error, ENOTSUP, RTE_FLOW_ERROR_TYPE_ACTION,
 			   actions, "action not supported");
+exit_return_error:
 	return -rte_errno;
 }
 
-- 
2.43.0

[PATCH 3/3] net/tap: compute TC handle correctly

[Stephen Hemminger]

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

Re: [PATCH 1/3] MAINTAINERS: add maintainer for TAP device

[Ferruh Yigit]

On 2/29/2024 5:31 PM, Stephen Hemminger wrote:
> Add myself as maintainer for TAP device.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>

Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>

Re: [PATCH 0/3] TAP device fixes

[Ferruh Yigit]

On 2/29/2024 5:31 PM, Stephen Hemminger wrote:
> Add myself as maintainer and fix some bugs found while testing.
> 
> Stephen Hemminger (3):
>   MAINTAINERS: add maintainer for TAP device
>   net/tap: do not overwrite rte_flow errors
>   net/tap: compute TC handle correctly
>

Series applied to dpdk-next-net/main, thanks.