[dpdk-dev] [PATCH 0/3] segment sanity checks

[dpdk-dev] [PATCH 0/3] segment sanity checks

[David Marchand]

Here is a little series which helped me identify a multi segment issue.
Hope it can help others.

The difference since the RFC patches I sent some time ago is that, rather
than force the user to build the dpdk with CONFIG_RTE_LIBRTE_MBUF_DEBUG
enabled, it uses rx/tx callbacks to apply checks on the mbufs.

This is not perfect, since when an invalid mbuf is detected, we try to free
it and a crash is possible at this time (depends on what went wrong).

On the rx side, this is better than let this packet go through the
application and we end up with harder to identify issues.
On the tx side, this is more about validating that the application did not
break the mbuf before passing it to the driver.

Example:
./testpmd -l 2,14 --socket-mem 2048,0 -w 0000:03:00.0 -w 0000:03:00.1 -- -i
[...]
testpmd> port stop all
testpmd> port config all sanity_check rx+tx
testpmd> port config all scatter on
testpmd> port start all
Configuring Port 0 (socket 0)
Port 0: F4:E9:D4:ED:B4:06
Configuring Port 1 (socket 0)
Port 1: F4:E9:D4:ED:B4:07
Checking link statuses...
Done
testpmd> port config mtu 0 9000
testpmd> port config mtu 1 9000
testpmd> start
testpmd: invalid rx mbuf on port 1 queue 0: data length too big in mbuf segment

testpmd> set verbose 1
Change verbose level from 0 to 1
dump mbuf at 0x7f48db9fa740, iova=2291fa7c0, buf_len=2176
  pkt_len=8014, ol_flags=180, nb_segs=4, in_port=1
  segment at 0x7f48db9fa740, data=0x7f48db9fa840, data_len=2112
  segment at 0x7f48db9fb080, data=0x7f48db9fb180, data_len=2112
  segment at 0x7f48db9fb9c0, data=0x7f48db9fbac0, data_len=2112
  segment at 0x7f48db9fc300, data=0x7f48db9fc400, data_len=1678
testpmd: invalid rx mbuf on port 1 queue 0: data length too big in mbuf segment


-- 
David Marchand

David Marchand (3):
  mbuf: add sanity checks on segment metadata
  mbuf: add a non fatal sanity check helper
  app/testpmd: add sanity checks on received/sent packets

 app/test-pmd/cmdline.c               |  63 ++++++++++++++
 app/test-pmd/config.c                |  23 +++++
 app/test-pmd/parameters.c            |   7 ++
 app/test-pmd/testpmd.c               | 123 +++++++++++++++++++++++++++
 app/test-pmd/testpmd.h               |   9 ++
 lib/librte_mbuf/Makefile             |   2 +
 lib/librte_mbuf/meson.build          |   2 +
 lib/librte_mbuf/rte_mbuf.c           |  71 ++++++++++++----
 lib/librte_mbuf/rte_mbuf.h           |  23 +++++
 lib/librte_mbuf/rte_mbuf_version.map |   6 ++
 10 files changed, 312 insertions(+), 17 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH 1/3] mbuf: add sanity checks on segment metadata

[David Marchand]

Add some basic checks on the segments offset and length metadata:
always funny to have a < 0 tailroom cast to uint16_t ;-).

Signed-off-by: David Marchand <david.marchand@6wind.com>
---
 lib/librte_mbuf/rte_mbuf.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index e714c5a59..137a320ed 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -200,6 +200,11 @@ rte_mbuf_sanity_check(const struct rte_mbuf *m, int is_header)
 	pkt_len = m->pkt_len;
 
 	do {
+		if (m->data_off > m->buf_len)
+			rte_panic("data offset too big in mbuf segment\n");
+		if ((uint32_t)m->data_off + (uint32_t)m->data_len >
+				(uint32_t)m->buf_len)
+			rte_panic("data length too big in mbuf segment\n");
 		nb_segs -= 1;
 		pkt_len -= m->data_len;
 	} while ((m = m->next) != NULL);
-- 
2.17.1

[dpdk-dev] [PATCH 2/3] mbuf: add a non fatal sanity check helper

[David Marchand]

Let's add a little helper that does the same as rte_mbuf_sanity_check but
without the panic.

Signed-off-by: David Marchand <david.marchand@6wind.com>
---
 lib/librte_mbuf/Makefile             |  2 +
 lib/librte_mbuf/meson.build          |  2 +
 lib/librte_mbuf/rte_mbuf.c           | 74 ++++++++++++++++++++--------
 lib/librte_mbuf/rte_mbuf.h           | 23 +++++++++
 lib/librte_mbuf/rte_mbuf_version.map |  6 +++
 5 files changed, 86 insertions(+), 21 deletions(-)

diff --git a/lib/librte_mbuf/Makefile b/lib/librte_mbuf/Makefile
index e2b98a254..7d0c824a3 100644
--- a/lib/librte_mbuf/Makefile
+++ b/lib/librte_mbuf/Makefile
@@ -7,6 +7,8 @@ include $(RTE_SDK)/mk/rte.vars.mk
 LIB = librte_mbuf.a
 
 CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) -O3
+CFLAGS += -DALLOW_EXPERIMENTAL_API
+
 LDLIBS += -lrte_eal -lrte_mempool
 
 EXPORT_MAP := rte_mbuf_version.map
diff --git a/lib/librte_mbuf/meson.build b/lib/librte_mbuf/meson.build
index 45ffb0db5..4d9bdf2ba 100644
--- a/lib/librte_mbuf/meson.build
+++ b/lib/librte_mbuf/meson.build
@@ -5,3 +5,5 @@ version = 3
 sources = files('rte_mbuf.c', 'rte_mbuf_ptype.c', 'rte_mbuf_pool_ops.c')
 headers = files('rte_mbuf.h', 'rte_mbuf_ptype.h', 'rte_mbuf_pool_ops.h')
 deps += ['mempool']
+
+allow_experimental_apis = true
diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 137a320ed..4e65f0f82 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -170,49 +170,81 @@ rte_pktmbuf_pool_create(const char *name, unsigned int n,
 /* do some sanity checks on a mbuf: panic if it fails */
 void
 rte_mbuf_sanity_check(const struct rte_mbuf *m, int is_header)
+{
+	const char *reason;
+
+	if (rte_mbuf_check(m, is_header, &reason))
+		rte_panic("%s\n", reason);
+}
+
+__rte_experimental
+int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
+		   const char **reason)
 {
 	unsigned int nb_segs, pkt_len;
 
-	if (m == NULL)
-		rte_panic("mbuf is NULL\n");
+	if (m == NULL) {
+		*reason = "mbuf is NULL";
+		return -1;
+	}
 
 	/* generic checks */
-	if (m->pool == NULL)
-		rte_panic("bad mbuf pool\n");
-	if (m->buf_iova == 0)
-		rte_panic("bad IO addr\n");
-	if (m->buf_addr == NULL)
-		rte_panic("bad virt addr\n");
+	if (m->pool == NULL) {
+		*reason = "bad mbuf pool";
+		return -1;
+	}
+	if (m->buf_iova == 0) {
+		*reason = "bad IO addr";
+		return -1;
+	}
+	if (m->buf_addr == NULL) {
+		*reason = "bad virt addr";
+		return -1;
+	}
 
 	uint16_t cnt = rte_mbuf_refcnt_read(m);
-	if ((cnt == 0) || (cnt == UINT16_MAX))
-		rte_panic("bad ref cnt\n");
+	if ((cnt == 0) || (cnt == UINT16_MAX)) {
+		*reason = "bad ref cnt";
+		return -1;
+	}
 
 	/* nothing to check for sub-segments */
 	if (is_header == 0)
-		return;
+		return 0;
 
 	/* data_len is supposed to be not more than pkt_len */
-	if (m->data_len > m->pkt_len)
-		rte_panic("bad data_len\n");
+	if (m->data_len > m->pkt_len) {
+		*reason = "bad data_len";
+		return -1;
+	}
 
 	nb_segs = m->nb_segs;
 	pkt_len = m->pkt_len;
 
 	do {
-		if (m->data_off > m->buf_len)
-			rte_panic("data offset too big in mbuf segment\n");
+		if (m->data_off > m->buf_len) {
+			*reason = "data offset too big in mbuf segment";
+			return -1;
+		}
 		if ((uint32_t)m->data_off + (uint32_t)m->data_len >
-				(uint32_t)m->buf_len)
-			rte_panic("data length too big in mbuf segment\n");
+				(uint32_t)m->buf_len) {
+			*reason = "data length too big in mbuf segment";
+			return -1;
+		}
 		nb_segs -= 1;
 		pkt_len -= m->data_len;
 	} while ((m = m->next) != NULL);
 
-	if (nb_segs)
-		rte_panic("bad nb_segs\n");
-	if (pkt_len)
-		rte_panic("bad pkt_len\n");
+	if (nb_segs) {
+		*reason = "bad nb_segs";
+		return -1;
+	}
+	if (pkt_len) {
+		*reason = "bad pkt_len";
+		return -1;
+	}
+
+	return 0;
 }
 
 /* dump a mbuf on console */
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index a50b05c64..e12a4c765 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -977,6 +977,29 @@ rte_mbuf_ext_refcnt_update(struct rte_mbuf_ext_shared_info *shinfo,
 void
 rte_mbuf_sanity_check(const struct rte_mbuf *m, int is_header);
 
+/**
+ * Sanity checks on a mbuf.
+ *
+ * Almost like rte_mbuf_sanity_check(), but this function gives the reason
+ * if corruption is detected rather than panic.
+ *
+ * @param m
+ *   The mbuf to be checked.
+ * @param is_header
+ *   True if the mbuf is a packet header, false if it is a sub-segment
+ *   of a packet (in this case, some fields like nb_segs are not checked)
+ * @param reason
+ *   A reference to a string pointer where to store the reason why a mbuf is
+ *   considered invalid.
+ * @return
+ *   - 0 if no issue has been found, reason is left untouched.
+ *   - -1 if a problem is detected, reason then points to a string describing
+ *     the reason why the mbuf is deemed invalid.
+ */
+__rte_experimental
+int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
+		   const char **reason);
+
 #define MBUF_RAW_ALLOC_CHECK(m) do {				\
 	RTE_ASSERT(rte_mbuf_refcnt_read(m) == 1);		\
 	RTE_ASSERT((m)->next == NULL);				\
diff --git a/lib/librte_mbuf/rte_mbuf_version.map b/lib/librte_mbuf/rte_mbuf_version.map
index cae68db8d..2662a37bf 100644
--- a/lib/librte_mbuf/rte_mbuf_version.map
+++ b/lib/librte_mbuf/rte_mbuf_version.map
@@ -45,3 +45,9 @@ DPDK_18.08 {
 	rte_mbuf_user_mempool_ops;
 	rte_pktmbuf_pool_create_by_ops;
 } DPDK_16.11;
+
+EXPERIMENTAL {
+	global:
+
+	rte_mbuf_check;
+} DPDK_18.08;
-- 
2.17.1

[dpdk-dev] [PATCH 3/3] app/testpmd: add sanity checks on received/sent packets

[David Marchand]

Make use of the newly introduced rte_mbuf_check() to (optionally) check
all packets received/sent through a port.
The idea behind this is to help to quickly identify badly formatted mbufs
coming from the pmd on the rx side, and from the application on the tx
side.
Setting the verbose level to some > 0 value will dump all packets in the
associated rx/tx callback to further help in the debugging.

Signed-off-by: David Marchand <david.marchand@6wind.com>
---
 app/test-pmd/cmdline.c    |  63 +++++++++++++++++++
 app/test-pmd/config.c     |  23 +++++++
 app/test-pmd/parameters.c |   7 +++
 app/test-pmd/testpmd.c    | 123 ++++++++++++++++++++++++++++++++++++++
 app/test-pmd/testpmd.h    |   9 +++
 5 files changed, 225 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 589121d69..1de533999 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -912,6 +912,9 @@ static void cmd_help_long_parsed(void *parsed_result,
 
 			"port config (port_id) udp_tunnel_port add|rm vxlan|geneve (udp_port)\n\n"
 			"    Add/remove UDP tunnel port for tunneling offload\n\n"
+
+			"port config all sanity_check (none|rx|tx|rx+tx)\n\n"
+			"    Configure sanity checks\n\n"
 		);
 	}
 
@@ -17602,6 +17605,65 @@ cmdline_parse_inst_t cmd_config_per_queue_tx_offload = {
 	}
 };
 
+/* *** configure sanity check for all ports *** */
+struct cmd_config_sanity_check_all {
+	cmdline_fixed_string_t port;
+	cmdline_fixed_string_t keyword;
+	cmdline_fixed_string_t all;
+	cmdline_fixed_string_t sanity_check;
+	cmdline_fixed_string_t mode;
+};
+
+static void
+cmd_config_sanity_check_all_parsed(void *parsed_result,
+	__rte_unused struct cmdline *cl, __rte_unused void *data)
+{
+	struct cmd_config_sanity_check_all *res = parsed_result;
+	portid_t pid;
+
+	if (!all_ports_stopped()) {
+		printf("Please stop all ports first\n");
+		return;
+	}
+
+	if (set_sanity_checks(res->mode) < 0)
+		return;
+
+	RTE_ETH_FOREACH_DEV(pid)
+		ports[pid].sanity_checks = sanity_checks;
+
+	cmd_reconfig_device_queue(RTE_PORT_ALL, 0, 1);
+}
+
+cmdline_parse_token_string_t cmd_config_sanity_check_all_port =
+	TOKEN_STRING_INITIALIZER(struct cmd_config_speed_all, port, "port");
+cmdline_parse_token_string_t cmd_config_sanity_check_all_keyword =
+	TOKEN_STRING_INITIALIZER(struct cmd_config_sanity_check_all, keyword,
+		"config");
+cmdline_parse_token_string_t cmd_config_sanity_check_all_all =
+	TOKEN_STRING_INITIALIZER(struct cmd_config_sanity_check_all, all,
+		"all");
+cmdline_parse_token_string_t cmd_config_sanity_check_all_sanity_check =
+	TOKEN_STRING_INITIALIZER(struct cmd_config_sanity_check_all,
+		sanity_check, "sanity_check");
+cmdline_parse_token_string_t cmd_config_sanity_check_all_mode =
+	TOKEN_STRING_INITIALIZER(struct cmd_config_sanity_check_all, mode,
+		"none#rx#tx#rx+tx");
+
+cmdline_parse_inst_t cmd_config_sanity_check_all = {
+	.f = cmd_config_sanity_check_all_parsed,
+	.data = NULL,
+	.help_str = "port config all sanity_check none|rx|tx|rx+tx",
+	.tokens = {
+		(void *)&cmd_config_sanity_check_all_port,
+		(void *)&cmd_config_sanity_check_all_keyword,
+		(void *)&cmd_config_sanity_check_all_all,
+		(void *)&cmd_config_sanity_check_all_sanity_check,
+		(void *)&cmd_config_sanity_check_all_mode,
+		NULL,
+	},
+};
+
 /* ******************************************************************************** */
 
 /* list of instructions */
@@ -17863,6 +17925,7 @@ cmdline_parse_ctx_t main_ctx[] = {
 	(cmdline_parse_inst_t *)&cmd_tx_offload_get_configuration,
 	(cmdline_parse_inst_t *)&cmd_config_per_port_tx_offload,
 	(cmdline_parse_inst_t *)&cmd_config_per_queue_tx_offload,
+	(cmdline_parse_inst_t *)&cmd_config_sanity_check_all,
 #ifdef RTE_LIBRTE_BPF
 	(cmdline_parse_inst_t *)&cmd_operate_bpf_ld_parse,
 	(cmdline_parse_inst_t *)&cmd_operate_bpf_unld_parse,
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 14ccd6864..f34327d02 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -1849,6 +1849,9 @@ rxtx_config_display(void)
 		printf("  port %d: RX queue number: %d Tx queue number: %d\n",
 				(unsigned int)pid, nb_rxq, nb_txq);
 
+		if (ports[pid].sanity_checks & SANITY_CHECK_RX)
+			printf("    RX sanity checks enabled\n");
+
 		printf("    Rx offloads=0x%"PRIx64" Tx offloads=0x%"PRIx64"\n",
 				ports[pid].dev_conf.rxmode.offloads,
 				ports[pid].dev_conf.txmode.offloads);
@@ -1873,6 +1876,9 @@ rxtx_config_display(void)
 				rx_conf[qid].offloads);
 		}
 
+		if (ports[pid].sanity_checks & SANITY_CHECK_TX)
+			printf("    TX sanity checks enabled\n");
+
 		/* per tx queue config only for first queue to be less verbose */
 		for (qid = 0; qid < 1; qid++) {
 			rc = rte_eth_tx_queue_info_get(pid, qid, &tx_qinfo);
@@ -3827,3 +3833,20 @@ port_queue_region_info_display(portid_t port_id, void *buf)
 
 	printf("\n\n");
 }
+
+int
+set_sanity_checks(const char *arg)
+{
+	if (!strcmp(arg, "rx")) {
+		sanity_checks = SANITY_CHECK_RX;
+		return 0;
+	} else if (!strcmp(arg, "tx")) {
+		sanity_checks = SANITY_CHECK_TX;
+		return 0;
+	} else if (!strcmp(arg, "rx+tx")) {
+		sanity_checks =
+			SANITY_CHECK_RX|SANITY_CHECK_TX;
+		return 0;
+	} else
+		return -1;
+}
diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 962fad789..5a06dc592 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -190,6 +190,7 @@ usage(char* progname)
 	printf("  --vxlan-gpe-port=N: UPD port of tunnel VXLAN-GPE\n");
 	printf("  --mlockall: lock all memory\n");
 	printf("  --no-mlockall: do not lock all memory\n");
+	printf("  --sanity-checks <rx|tx|rx+tx>: enable rx/tx sanity checks on mbuf\n");
 }
 
 #ifdef RTE_LIBRTE_CMDLINE
@@ -625,6 +626,7 @@ launch_args_parse(int argc, char** argv)
 		{ "vxlan-gpe-port",		1, 0, 0 },
 		{ "mlockall",			0, 0, 0 },
 		{ "no-mlockall",		0, 0, 0 },
+		{ "sanity-checks",		1, 0, 0 },
 		{ 0, 0, 0, 0 },
 	};
 
@@ -1147,6 +1149,11 @@ launch_args_parse(int argc, char** argv)
 				do_mlockall = 1;
 			if (!strcmp(lgopts[opt_idx].name, "no-mlockall"))
 				do_mlockall = 0;
+			if (!strcmp(lgopts[opt_idx].name, "sanity-checks")) {
+				if (set_sanity_checks(optarg) < 0)
+					rte_exit(EXIT_FAILURE,
+						 "invalid sanity-checks argument\n");
+			}
 			break;
 		case 'h':
 			usage(argv[0]);
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index ee48db2a3..a310431eb 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -210,6 +210,9 @@ uint8_t dcb_test = 0;
 queueid_t nb_rxq = 1; /**< Number of RX queues per port. */
 queueid_t nb_txq = 1; /**< Number of TX queues per port. */
 
+/* Sanity checks configuration */
+uint8_t sanity_checks;
+
 /*
  * Configurable number of RX/TX ring descriptors.
  * Defaults are supplied by drivers via ethdev.
@@ -769,6 +772,9 @@ init_config(void)
 			port->tx_conf[k].offloads =
 				port->dev_conf.txmode.offloads;
 
+		/* Configure sanity checks with initial value from cmdline */
+		port->sanity_checks = sanity_checks;
+
 		/* set flag to initialize port/queue */
 		port->need_reconfig = 1;
 		port->need_reconfig_queues = 1;
@@ -1632,6 +1638,59 @@ port_is_closed(portid_t port_id)
 	return 1;
 }
 
+static uint16_t
+mbuf_tx_check(uint16_t port_id, uint16_t queue, struct rte_mbuf *pkts[],
+	      uint16_t nb_pkts, __rte_unused void *user_param)
+{
+	unsigned int count = 0;
+	unsigned int i;
+
+	for (i = 0; i < nb_pkts; i++) {
+		const char *reason;
+
+		if (verbose_level > 0)
+			rte_pktmbuf_dump(stdout, pkts[i], 0);
+
+		if (!rte_mbuf_check(pkts[i], 1, &reason)) {
+			pkts[count++] = pkts[i];
+			continue;
+		}
+
+		TESTPMD_LOG(ERR, "invalid tx mbuf on port %"PRIu16" queue %"
+			    PRIu16": %s\n", port_id, queue, reason);
+		rte_pktmbuf_free(pkts[i]);
+	}
+
+	return count;
+}
+
+static uint16_t
+mbuf_rx_check(uint16_t port_id, uint16_t queue, struct rte_mbuf *pkts[],
+	      uint16_t nb_pkts, __rte_unused uint16_t max_pkts,
+	      __rte_unused void *user_param)
+{
+	unsigned int count = 0;
+	unsigned int i;
+
+	for (i = 0; i < nb_pkts; i++) {
+		const char *reason;
+
+		if (verbose_level > 0)
+			rte_pktmbuf_dump(stdout, pkts[i], 0);
+
+		if (!rte_mbuf_check(pkts[i], 1, &reason)) {
+			pkts[count++] = pkts[i];
+			continue;
+		}
+
+		TESTPMD_LOG(ERR, "invalid rx mbuf on port %"PRIu16" queue %"
+			    PRIu16": %s\n", port_id, queue, reason);
+		rte_pktmbuf_free(pkts[i]);
+	}
+
+	return count;
+}
+
 int
 start_port(portid_t pid)
 {
@@ -1641,6 +1700,7 @@ start_port(portid_t pid)
 	struct rte_port *port;
 	struct ether_addr mac_addr;
 	enum rte_eth_event_type event_type;
+	struct rte_eth_dev_info dev_info;
 
 	if (port_id_is_invalid(pid, ENABLED_WARN))
 		return 0;
@@ -1671,6 +1731,32 @@ start_port(portid_t pid)
 				}
 			}
 
+			/* Free any remaining rx/tx callbacks before changing
+			 * rxq/txq count.
+			 */
+			rte_eth_dev_info_get(pi, &dev_info);
+			for (qi = 0; qi < dev_info.nb_tx_queues; qi++) {
+				if (!port->tx_checks_cb[qi])
+					continue;
+				if (rte_eth_remove_tx_callback(pi, qi,
+						port->tx_checks_cb[qi]) < 0) {
+					/* try to reconfigure port next time */
+					port->need_reconfig = 1;
+					return -1;
+				}
+				port->tx_checks_cb[qi] = NULL;
+			}
+			for (qi = 0; qi < dev_info.nb_rx_queues; qi++) {
+				if (!port->rx_checks_cb[qi])
+					continue;
+				if (rte_eth_remove_rx_callback(pi, qi,
+						port->rx_checks_cb[qi]) < 0) {
+					/* try to reconfigure port next time */
+					port->need_reconfig = 1;
+					return -1;
+				}
+				port->rx_checks_cb[qi] = NULL;
+			}
 			printf("Configuring Port %d (socket %u)\n", pi,
 					port->socket_id);
 			/* configure port */
@@ -1703,6 +1789,24 @@ start_port(portid_t pid)
 						port->socket_id,
 						&(port->tx_conf[qi]));
 
+				if (diag == 0 &&
+				    port->tx_checks_cb[qi]) {
+					if (!rte_eth_remove_tx_callback(pi, qi,
+							port->tx_checks_cb[qi]))
+						port->tx_checks_cb[qi] = NULL;
+					else
+						diag = -1;
+				}
+
+				if (diag == 0 &&
+				    port->sanity_checks & SANITY_CHECK_TX) {
+					port->tx_checks_cb[qi] =
+						rte_eth_add_tx_callback(pi, qi,
+							mbuf_tx_check, NULL);
+					if (!port->tx_checks_cb[qi])
+						diag = -1;
+				}
+
 				if (diag == 0)
 					continue;
 
@@ -1753,6 +1857,25 @@ start_port(portid_t pid)
 					     &(port->rx_conf[qi]),
 					     mp);
 				}
+
+				if (diag == 0 &&
+				    port->rx_checks_cb[qi]) {
+					if (!rte_eth_remove_rx_callback(pi, qi,
+							port->rx_checks_cb[qi]))
+						port->rx_checks_cb[qi] = NULL;
+					else
+						diag = -1;
+				}
+
+				if (diag == 0 &&
+				    port->sanity_checks & SANITY_CHECK_RX) {
+					port->rx_checks_cb[qi] =
+						rte_eth_add_rx_callback(pi, qi,
+							mbuf_rx_check, NULL);
+					if (!port->rx_checks_cb[qi])
+						diag = -1;
+				}
+
 				if (diag == 0)
 					continue;
 
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index a1f661472..bdab372b2 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -180,6 +180,11 @@ struct rte_port {
 	uint32_t                mc_addr_nb; /**< nb. of addr. in mc_addr_pool */
 	uint8_t                 slave_flag; /**< bonding slave port */
 	struct port_flow        *flow_list; /**< Associated flows. */
+#define SANITY_CHECK_RX ((uint8_t)1 << 0)
+#define SANITY_CHECK_TX ((uint8_t)1 << 1)
+	uint8_t                 sanity_checks;
+	const struct rte_eth_rxtx_callback *rx_checks_cb[MAX_QUEUE_ID+1];
+	const struct rte_eth_rxtx_callback *tx_checks_cb[MAX_QUEUE_ID+1];
 #ifdef SOFTNIC
 	struct softnic_port     softport;  /**< softnic params */
 #endif
@@ -378,6 +383,8 @@ extern int16_t tx_rs_thresh;
 extern uint8_t dcb_config;
 extern uint8_t dcb_test;
 
+extern uint8_t sanity_checks;
+
 extern uint16_t mbuf_data_size; /**< Mbuf data space size. */
 extern uint32_t param_total_num_mbufs;
 
@@ -730,6 +737,8 @@ int close_file(uint8_t *buf);
 
 void port_queue_region_info_display(portid_t port_id, void *buf);
 
+int set_sanity_checks(const char *arg);
+
 enum print_warning {
 	ENABLED_WARN = 0,
 	DISABLED_WARN
-- 
2.17.1

Re: [dpdk-dev] [PATCH 2/3] mbuf: add a non fatal sanity check helper

[David Marchand]

On Mon, Sep 10, 2018 at 7:45 AM, David Marchand
<david.marchand@6wind.com> wrote:
> Let's add a little helper that does the same as rte_mbuf_sanity_check but
> without the panic.
>
> Signed-off-by: David Marchand <david.marchand@6wind.com>
> ---
>  lib/librte_mbuf/Makefile             |  2 +
>  lib/librte_mbuf/meson.build          |  2 +
>  lib/librte_mbuf/rte_mbuf.c           | 74 ++++++++++++++++++++--------
>  lib/librte_mbuf/rte_mbuf.h           | 23 +++++++++
>  lib/librte_mbuf/rte_mbuf_version.map |  6 +++

Applied a little patch on devtools/check-symbol-change.sh
@@ -115,7 +115,7 @@ check_for_rule_violations()
                                if [ $? -ne 0 ]
                                then
                                        echo -n "ERROR: symbol $symname "
-                                       echo -n "is added in a section "
+                                       echo -n "is added in $secname section "
                                        echo -n "other than the EXPERIMENTAL "
                                        echo "section of the version map"
                                        ret=1

And here is what I get when I check my patch with it:

marchand@gribouille:~/git/dpdk$ ./devtools/check-symbol-change.sh
sanity/0002-mbuf-add-a-non-fatal-sanity-check-helper.patch
ERROR: symbol rte_mbuf_check is added in +EXPERIMENTAL section other
than the EXPERIMENTAL section of the version map

Is this because the EXPERIMENTAL section is added in the same patch ?


-- 
David Marchand

Re: [dpdk-dev] [PATCH 2/3] mbuf: add a non fatal sanity check helper

[Andrew Rybchenko]

On 09/10/2018 08:45 AM, David Marchand wrote:
> Let's add a little helper that does the same as rte_mbuf_sanity_check but
> without the panic.
>
> Signed-off-by: David Marchand <david.marchand@6wind.com>
> ---

<...>

> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index a50b05c64..e12a4c765 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -977,6 +977,29 @@ rte_mbuf_ext_refcnt_update(struct rte_mbuf_ext_shared_info *shinfo,
>   void
>   rte_mbuf_sanity_check(const struct rte_mbuf *m, int is_header);
>   
> +/**
> + * Sanity checks on a mbuf.
> + *
> + * Almost like rte_mbuf_sanity_check(), but this function gives the reason
> + * if corruption is detected rather than panic.
> + *
> + * @param m
> + *   The mbuf to be checked.
> + * @param is_header
> + *   True if the mbuf is a packet header, false if it is a sub-segment
> + *   of a packet (in this case, some fields like nb_segs are not checked)
> + * @param reason
> + *   A reference to a string pointer where to store the reason why a mbuf is
> + *   considered invalid.
> + * @return
> + *   - 0 if no issue has been found, reason is left untouched.
> + *   - -1 if a problem is detected, reason then points to a string describing
> + *     the reason why the mbuf is deemed invalid.
> + */
> +__rte_experimental
> +int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
> +		   const char **reason);
> +

May be it would be better to return reason as return value and if it is 
NULL everything is OK?

<...>

Re: [dpdk-dev] [PATCH 2/3] mbuf: add a non fatal sanity check helper

[David Marchand]

On Mon, Sep 10, 2018 at 10:12 AM, Andrew Rybchenko
<arybchenko@solarflare.com> wrote:
> +/**
> + * Sanity checks on a mbuf.
> + *
> + * Almost like rte_mbuf_sanity_check(), but this function gives the reason
> + * if corruption is detected rather than panic.
> + *
> + * @param m
> + *   The mbuf to be checked.
> + * @param is_header
> + *   True if the mbuf is a packet header, false if it is a sub-segment
> + *   of a packet (in this case, some fields like nb_segs are not checked)
> + * @param reason
> + *   A reference to a string pointer where to store the reason why a mbuf
> is
> + *   considered invalid.
> + * @return
> + *   - 0 if no issue has been found, reason is left untouched.
> + *   - -1 if a problem is detected, reason then points to a string
> describing
> + *     the reason why the mbuf is deemed invalid.
> + */
> +__rte_experimental
> +int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
> +   const char **reason);
> +
>
>
> May be it would be better to return reason as return value and if it is NULL
> everything is OK?

This was what I had done with my first attempt.
But given the name "rte_mbuf_check", having a int (used as a bool)
returned makes sense to me.

So no strong opinion here.


-- 
David Marchand

Re: [dpdk-dev] [PATCH 2/3] mbuf: add a non fatal sanity check helper

[Andrew Rybchenko]

On 09/10/2018 11:24 AM, David Marchand wrote:
> On Mon, Sep 10, 2018 at 10:12 AM, Andrew Rybchenko
> <arybchenko@solarflare.com> wrote:
>> +/**
>> + * Sanity checks on a mbuf.
>> + *
>> + * Almost like rte_mbuf_sanity_check(), but this function gives the reason
>> + * if corruption is detected rather than panic.
>> + *
>> + * @param m
>> + *   The mbuf to be checked.
>> + * @param is_header
>> + *   True if the mbuf is a packet header, false if it is a sub-segment
>> + *   of a packet (in this case, some fields like nb_segs are not checked)
>> + * @param reason
>> + *   A reference to a string pointer where to store the reason why a mbuf
>> is
>> + *   considered invalid.
>> + * @return
>> + *   - 0 if no issue has been found, reason is left untouched.
>> + *   - -1 if a problem is detected, reason then points to a string
>> describing
>> + *     the reason why the mbuf is deemed invalid.
>> + */
>> +__rte_experimental
>> +int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
>> +   const char **reason);
>> +
>>
>>
>> May be it would be better to return reason as return value and if it is NULL
>> everything is OK?
> This was what I had done with my first attempt.
> But given the name "rte_mbuf_check", having a int (used as a bool)
> returned makes sense to me.

Yes, good point.

> So no strong opinion here.

Me too.

Re: [dpdk-dev] [PATCH 1/3] mbuf: add sanity checks on segment metadata

[Yongseok Koh]

> On Sep 9, 2018, at 10:45 PM, David Marchand <david.marchand@6wind.com> wrote:
> 
> Add some basic checks on the segments offset and length metadata:
> always funny to have a < 0 tailroom cast to uint16_t ;-).
> 
> Signed-off-by: David Marchand <david.marchand@6wind.com>
> ---
> lib/librte_mbuf/rte_mbuf.c | 5 +++++
> 1 file changed, 5 insertions(+)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> index e714c5a59..137a320ed 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -200,6 +200,11 @@ rte_mbuf_sanity_check(const struct rte_mbuf *m, int is_header)
> 	pkt_len = m->pkt_len;
> 
> 	do {
> +		if (m->data_off > m->buf_len)
> +			rte_panic("data offset too big in mbuf segment\n");
> +		if ((uint32_t)m->data_off + (uint32_t)m->data_len >
> +				(uint32_t)m->buf_len)

Casting to uint32_t is needed? All of the three fields are uint16_t and it would
anyway happen because of the integer promotion rule. Right?


Thanks,
Yongseok

> +			rte_panic("data length too big in mbuf segment\n");
> 		nb_segs -= 1;
> 		pkt_len -= m->data_len;
> 	} while ((m = m->next) != NULL);
> -- 
> 2.17.1
> 

Re: [dpdk-dev] [PATCH 1/3] mbuf: add sanity checks on segment metadata

[David Marchand]

Hello Yongseok,

On Tue, Sep 11, 2018 at 8:16 PM, Yongseok Koh <yskoh@mellanox.com> wrote:
>
>> On Sep 9, 2018, at 10:45 PM, David Marchand <david.marchand@6wind.com> wrote:
>>
>> Add some basic checks on the segments offset and length metadata:
>> always funny to have a < 0 tailroom cast to uint16_t ;-).
>>
>> Signed-off-by: David Marchand <david.marchand@6wind.com>
>> ---
>> lib/librte_mbuf/rte_mbuf.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
>> index e714c5a59..137a320ed 100644
>> --- a/lib/librte_mbuf/rte_mbuf.c
>> +++ b/lib/librte_mbuf/rte_mbuf.c
>> @@ -200,6 +200,11 @@ rte_mbuf_sanity_check(const struct rte_mbuf *m, int is_header)
>>       pkt_len = m->pkt_len;
>>
>>       do {
>> +             if (m->data_off > m->buf_len)
>> +                     rte_panic("data offset too big in mbuf segment\n");
>> +             if ((uint32_t)m->data_off + (uint32_t)m->data_len >
>> +                             (uint32_t)m->buf_len)
>
> Casting to uint32_t is needed? All of the three fields are uint16_t and it would
> anyway happen because of the integer promotion rule. Right?

Indeed, this is unnecessary.
Will send a v2 without this.


-- 
David Marchand

Re: [dpdk-dev] [PATCH 3/3] app/testpmd: add sanity checks on received/sent packets

[Iremonger, Bernard]

Hi David,

> -----Original Message-----
> From: David Marchand [mailto:david.marchand@6wind.com]
> Sent: Monday, September 10, 2018 6:46 AM
> To: dev@dpdk.org
> Cc: olivier.matz@6wind.com; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu,
> Jingjing <jingjing.wu@intel.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>
> Subject: [PATCH 3/3] app/testpmd: add sanity checks on received/sent
> packets
> 
> Make use of the newly introduced rte_mbuf_check() to (optionally) check all
> packets received/sent through a port.
> The idea behind this is to help to quickly identify badly formatted mbufs
> coming from the pmd on the rx side, and from the application on the tx side.
> Setting the verbose level to some > 0 value will dump all packets in the
> associated rx/tx callback to further help in the debugging.
> 
> Signed-off-by: David Marchand <david.marchand@6wind.com>
> ---
>  app/test-pmd/cmdline.c    |  63 +++++++++++++++++++
>  app/test-pmd/config.c     |  23 +++++++
>  app/test-pmd/parameters.c |   7 +++
>  app/test-pmd/testpmd.c    | 123
> ++++++++++++++++++++++++++++++++++++++
>  app/test-pmd/testpmd.h    |   9 +++
>  5 files changed, 225 insertions(+)

There should probably be an entry in section 4.6 of the Testpmd User Guide for the 
"port config all sanity_check"  command.

> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> 589121d69..1de533999 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -912,6 +912,9 @@ static void cmd_help_long_parsed(void
> *parsed_result,
> 
>  			"port config (port_id) udp_tunnel_port add|rm
> vxlan|geneve (udp_port)\n\n"
>  			"    Add/remove UDP tunnel port for tunneling
> offload\n\n"
> +
> +			"port config all sanity_check (none|rx|tx|rx+tx)\n\n"
> +			"    Configure sanity checks\n\n"
>  		);
>  	}
> 
> @@ -17602,6 +17605,65 @@ cmdline_parse_inst_t
> cmd_config_per_queue_tx_offload = {
>  	}
>  };
> 
> +/* *** configure sanity check for all ports *** */ struct
> +cmd_config_sanity_check_all {
> +	cmdline_fixed_string_t port;
> +	cmdline_fixed_string_t keyword;
> +	cmdline_fixed_string_t all;
> +	cmdline_fixed_string_t sanity_check;
> +	cmdline_fixed_string_t mode;
> +};
> +
> +static void
> +cmd_config_sanity_check_all_parsed(void *parsed_result,
> +	__rte_unused struct cmdline *cl, __rte_unused void *data) {
> +	struct cmd_config_sanity_check_all *res = parsed_result;
> +	portid_t pid;
> +
> +	if (!all_ports_stopped()) {
> +		printf("Please stop all ports first\n");
> +		return;
> +	}
> +
> +	if (set_sanity_checks(res->mode) < 0)
> +		return;
> +
> +	RTE_ETH_FOREACH_DEV(pid)
> +		ports[pid].sanity_checks = sanity_checks;
> +
> +	cmd_reconfig_device_queue(RTE_PORT_ALL, 0, 1); }
> +
> +cmdline_parse_token_string_t cmd_config_sanity_check_all_port =
> +	TOKEN_STRING_INITIALIZER(struct cmd_config_speed_all, port,
> "port");
> +cmdline_parse_token_string_t cmd_config_sanity_check_all_keyword =
> +	TOKEN_STRING_INITIALIZER(struct cmd_config_sanity_check_all,
> keyword,
> +		"config");
> +cmdline_parse_token_string_t cmd_config_sanity_check_all_all =
> +	TOKEN_STRING_INITIALIZER(struct cmd_config_sanity_check_all, all,
> +		"all");
> +cmdline_parse_token_string_t cmd_config_sanity_check_all_sanity_check
> =
> +	TOKEN_STRING_INITIALIZER(struct cmd_config_sanity_check_all,
> +		sanity_check, "sanity_check");
> +cmdline_parse_token_string_t cmd_config_sanity_check_all_mode =
> +	TOKEN_STRING_INITIALIZER(struct cmd_config_sanity_check_all,
> mode,
> +		"none#rx#tx#rx+tx");
> +
> +cmdline_parse_inst_t cmd_config_sanity_check_all = {
> +	.f = cmd_config_sanity_check_all_parsed,
> +	.data = NULL,
> +	.help_str = "port config all sanity_check none|rx|tx|rx+tx",
> +	.tokens = {
> +		(void *)&cmd_config_sanity_check_all_port,
> +		(void *)&cmd_config_sanity_check_all_keyword,
> +		(void *)&cmd_config_sanity_check_all_all,
> +		(void *)&cmd_config_sanity_check_all_sanity_check,
> +		(void *)&cmd_config_sanity_check_all_mode,
> +		NULL,
> +	},
> +};
> +
>  /*
> **********************************************************
> ********************** */
> 
>  /* list of instructions */
> @@ -17863,6 +17925,7 @@ cmdline_parse_ctx_t main_ctx[] = {
>  	(cmdline_parse_inst_t *)&cmd_tx_offload_get_configuration,
>  	(cmdline_parse_inst_t *)&cmd_config_per_port_tx_offload,
>  	(cmdline_parse_inst_t *)&cmd_config_per_queue_tx_offload,
> +	(cmdline_parse_inst_t *)&cmd_config_sanity_check_all,
>  #ifdef RTE_LIBRTE_BPF
>  	(cmdline_parse_inst_t *)&cmd_operate_bpf_ld_parse,
>  	(cmdline_parse_inst_t *)&cmd_operate_bpf_unld_parse, diff --git
> a/app/test-pmd/config.c b/app/test-pmd/config.c index
> 14ccd6864..f34327d02 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -1849,6 +1849,9 @@ rxtx_config_display(void)
>  		printf("  port %d: RX queue number: %d Tx queue number:
> %d\n",
>  				(unsigned int)pid, nb_rxq, nb_txq);
> 
> +		if (ports[pid].sanity_checks & SANITY_CHECK_RX)
> +			printf("    RX sanity checks enabled\n");
> +
>  		printf("    Rx offloads=0x%"PRIx64" Tx
> offloads=0x%"PRIx64"\n",
>  				ports[pid].dev_conf.rxmode.offloads,
>  				ports[pid].dev_conf.txmode.offloads);
> @@ -1873,6 +1876,9 @@ rxtx_config_display(void)
>  				rx_conf[qid].offloads);
>  		}
> 
> +		if (ports[pid].sanity_checks & SANITY_CHECK_TX)
> +			printf("    TX sanity checks enabled\n");
> +
>  		/* per tx queue config only for first queue to be less verbose
> */
>  		for (qid = 0; qid < 1; qid++) {
>  			rc = rte_eth_tx_queue_info_get(pid, qid,
> &tx_qinfo); @@ -3827,3 +3833,20 @@
> port_queue_region_info_display(portid_t port_id, void *buf)
> 
>  	printf("\n\n");
>  }
> +
> +int
> +set_sanity_checks(const char *arg)
> +{
> +	if (!strcmp(arg, "rx")) {
> +		sanity_checks = SANITY_CHECK_RX;
> +		return 0;
> +	} else if (!strcmp(arg, "tx")) {
> +		sanity_checks = SANITY_CHECK_TX;
> +		return 0;
> +	} else if (!strcmp(arg, "rx+tx")) {
> +		sanity_checks =
> +			SANITY_CHECK_RX|SANITY_CHECK_TX;
> +		return 0;
> +	} else
> +		return -1;
> +}
> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c index
> 962fad789..5a06dc592 100644
> --- a/app/test-pmd/parameters.c
> +++ b/app/test-pmd/parameters.c
> @@ -190,6 +190,7 @@ usage(char* progname)
>  	printf("  --vxlan-gpe-port=N: UPD port of tunnel VXLAN-GPE\n");
>  	printf("  --mlockall: lock all memory\n");
>  	printf("  --no-mlockall: do not lock all memory\n");
> +	printf("  --sanity-checks <rx|tx|rx+tx>: enable rx/tx sanity checks on
> +mbuf\n");
>  }
> 
>  #ifdef RTE_LIBRTE_CMDLINE
> @@ -625,6 +626,7 @@ launch_args_parse(int argc, char** argv)
>  		{ "vxlan-gpe-port",		1, 0, 0 },
>  		{ "mlockall",			0, 0, 0 },
>  		{ "no-mlockall",		0, 0, 0 },
> +		{ "sanity-checks",		1, 0, 0 },
>  		{ 0, 0, 0, 0 },
>  	};
> 
> @@ -1147,6 +1149,11 @@ launch_args_parse(int argc, char** argv)
>  				do_mlockall = 1;
>  			if (!strcmp(lgopts[opt_idx].name, "no-mlockall"))
>  				do_mlockall = 0;
> +			if (!strcmp(lgopts[opt_idx].name, "sanity-checks")) {
> +				if (set_sanity_checks(optarg) < 0)
> +					rte_exit(EXIT_FAILURE,
> +						 "invalid sanity-checks
> argument\n");
> +			}
>  			break;
>  		case 'h':
>  			usage(argv[0]);
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> ee48db2a3..a310431eb 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -210,6 +210,9 @@ uint8_t dcb_test = 0;  queueid_t nb_rxq = 1; /**<
> Number of RX queues per port. */  queueid_t nb_txq = 1; /**< Number of
> TX queues per port. */
> 
> +/* Sanity checks configuration */
> +uint8_t sanity_checks;
> +
>  /*
>   * Configurable number of RX/TX ring descriptors.
>   * Defaults are supplied by drivers via ethdev.
> @@ -769,6 +772,9 @@ init_config(void)
>  			port->tx_conf[k].offloads =
>  				port->dev_conf.txmode.offloads;
> 
> +		/* Configure sanity checks with initial value from cmdline */
> +		port->sanity_checks = sanity_checks;
> +
>  		/* set flag to initialize port/queue */
>  		port->need_reconfig = 1;
>  		port->need_reconfig_queues = 1;
> @@ -1632,6 +1638,59 @@ port_is_closed(portid_t port_id)
>  	return 1;
>  }
> 
> +static uint16_t
> +mbuf_tx_check(uint16_t port_id, uint16_t queue, struct rte_mbuf *pkts[],
> +	      uint16_t nb_pkts, __rte_unused void *user_param) {
> +	unsigned int count = 0;
> +	unsigned int i;
> +
> +	for (i = 0; i < nb_pkts; i++) {
> +		const char *reason;
> +
> +		if (verbose_level > 0)
> +			rte_pktmbuf_dump(stdout, pkts[i], 0);
> +
> +		if (!rte_mbuf_check(pkts[i], 1, &reason)) {
> +			pkts[count++] = pkts[i];
> +			continue;
> +		}
> +
> +		TESTPMD_LOG(ERR, "invalid tx mbuf on port %"PRIu16"
> queue %"
> +			    PRIu16": %s\n", port_id, queue, reason);
> +		rte_pktmbuf_free(pkts[i]);
> +	}
> +
> +	return count;
> +}
> +
> +static uint16_t
> +mbuf_rx_check(uint16_t port_id, uint16_t queue, struct rte_mbuf *pkts[],
> +	      uint16_t nb_pkts, __rte_unused uint16_t max_pkts,
> +	      __rte_unused void *user_param)
> +{
> +	unsigned int count = 0;
> +	unsigned int i;
> +
> +	for (i = 0; i < nb_pkts; i++) {
> +		const char *reason;
> +
> +		if (verbose_level > 0)
> +			rte_pktmbuf_dump(stdout, pkts[i], 0);
> +
> +		if (!rte_mbuf_check(pkts[i], 1, &reason)) {
> +			pkts[count++] = pkts[i];
> +			continue;
> +		}
> +
> +		TESTPMD_LOG(ERR, "invalid rx mbuf on port %"PRIu16"
> queue %"
> +			    PRIu16": %s\n", port_id, queue, reason);
> +		rte_pktmbuf_free(pkts[i]);
> +	}
> +
> +	return count;
> +}
> +
>  int
>  start_port(portid_t pid)
>  {
> @@ -1641,6 +1700,7 @@ start_port(portid_t pid)
>  	struct rte_port *port;
>  	struct ether_addr mac_addr;
>  	enum rte_eth_event_type event_type;
> +	struct rte_eth_dev_info dev_info;
> 
>  	if (port_id_is_invalid(pid, ENABLED_WARN))
>  		return 0;
> @@ -1671,6 +1731,32 @@ start_port(portid_t pid)
>  				}
>  			}
> 
> +			/* Free any remaining rx/tx callbacks before changing
> +			 * rxq/txq count.
> +			 */
> +			rte_eth_dev_info_get(pi, &dev_info);
> +			for (qi = 0; qi < dev_info.nb_tx_queues; qi++) {
> +				if (!port->tx_checks_cb[qi])
> +					continue;
> +				if (rte_eth_remove_tx_callback(pi, qi,
> +						port->tx_checks_cb[qi]) < 0) {
> +					/* try to reconfigure port next time
> */
> +					port->need_reconfig = 1;
> +					return -1;
> +				}
> +				port->tx_checks_cb[qi] = NULL;
> +			}
> +			for (qi = 0; qi < dev_info.nb_rx_queues; qi++) {
> +				if (!port->rx_checks_cb[qi])
> +					continue;
> +				if (rte_eth_remove_rx_callback(pi, qi,
> +						port->rx_checks_cb[qi]) < 0) {
> +					/* try to reconfigure port next time
> */
> +					port->need_reconfig = 1;
> +					return -1;
> +				}
> +				port->rx_checks_cb[qi] = NULL;
> +			}
>  			printf("Configuring Port %d (socket %u)\n", pi,
>  					port->socket_id);
>  			/* configure port */
> @@ -1703,6 +1789,24 @@ start_port(portid_t pid)
>  						port->socket_id,
>  						&(port->tx_conf[qi]));
> 
> +				if (diag == 0 &&
> +				    port->tx_checks_cb[qi]) {
> +					if (!rte_eth_remove_tx_callback(pi,
> qi,
> +							port-
> >tx_checks_cb[qi]))
> +						port->tx_checks_cb[qi] =
> NULL;
> +					else
> +						diag = -1;
> +				}
> +
> +				if (diag == 0 &&
> +				    port->sanity_checks & SANITY_CHECK_TX)
> {
> +					port->tx_checks_cb[qi] =
> +						rte_eth_add_tx_callback(pi,
> qi,
> +							mbuf_tx_check,
> NULL);
> +					if (!port->tx_checks_cb[qi])
> +						diag = -1;
> +				}
> +
>  				if (diag == 0)
>  					continue;
> 
> @@ -1753,6 +1857,25 @@ start_port(portid_t pid)
>  					     &(port->rx_conf[qi]),
>  					     mp);
>  				}
> +
> +				if (diag == 0 &&
> +				    port->rx_checks_cb[qi]) {
> +					if (!rte_eth_remove_rx_callback(pi,
> qi,
> +							port-
> >rx_checks_cb[qi]))
> +						port->rx_checks_cb[qi] =
> NULL;
> +					else
> +						diag = -1;
> +				}
> +
> +				if (diag == 0 &&
> +				    port->sanity_checks & SANITY_CHECK_RX)
> {
> +					port->rx_checks_cb[qi] =
> +						rte_eth_add_rx_callback(pi,
> qi,
> +							mbuf_rx_check,
> NULL);
> +					if (!port->rx_checks_cb[qi])
> +						diag = -1;
> +				}
> +
>  				if (diag == 0)
>  					continue;
> 
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h index
> a1f661472..bdab372b2 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -180,6 +180,11 @@ struct rte_port {
>  	uint32_t                mc_addr_nb; /**< nb. of addr. in mc_addr_pool */
>  	uint8_t                 slave_flag; /**< bonding slave port */
>  	struct port_flow        *flow_list; /**< Associated flows. */
> +#define SANITY_CHECK_RX ((uint8_t)1 << 0) #define SANITY_CHECK_TX
> +((uint8_t)1 << 1)
> +	uint8_t                 sanity_checks;
> +	const struct rte_eth_rxtx_callback
> *rx_checks_cb[MAX_QUEUE_ID+1];
> +	const struct rte_eth_rxtx_callback
> *tx_checks_cb[MAX_QUEUE_ID+1];
>  #ifdef SOFTNIC
>  	struct softnic_port     softport;  /**< softnic params */
>  #endif
> @@ -378,6 +383,8 @@ extern int16_t tx_rs_thresh;  extern uint8_t
> dcb_config;  extern uint8_t dcb_test;
> 
> +extern uint8_t sanity_checks;
> +
>  extern uint16_t mbuf_data_size; /**< Mbuf data space size. */  extern
> uint32_t param_total_num_mbufs;
> 
> @@ -730,6 +737,8 @@ int close_file(uint8_t *buf);
> 
>  void port_queue_region_info_display(portid_t port_id, void *buf);
> 
> +int set_sanity_checks(const char *arg);
> +
>  enum print_warning {
>  	ENABLED_WARN = 0,
>  	DISABLED_WARN
> --
> 2.17.1

Regards,

Bernard.

Re: [dpdk-dev] [PATCH 3/3] app/testpmd: add sanity checks on received/sent packets

[David Marchand]

On Tue, Sep 25, 2018 at 3:17 PM, Iremonger, Bernard
<bernard.iremonger@intel.com> wrote:
> Hi David,
>
>> -----Original Message-----
>> From: David Marchand [mailto:david.marchand@6wind.com]
>> Sent: Monday, September 10, 2018 6:46 AM
>> To: dev@dpdk.org
>> Cc: olivier.matz@6wind.com; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu,
>> Jingjing <jingjing.wu@intel.com>; Iremonger, Bernard
>> <bernard.iremonger@intel.com>
>> Subject: [PATCH 3/3] app/testpmd: add sanity checks on received/sent
>> packets
>>
>> Make use of the newly introduced rte_mbuf_check() to (optionally) check all
>> packets received/sent through a port.
>> The idea behind this is to help to quickly identify badly formatted mbufs
>> coming from the pmd on the rx side, and from the application on the tx side.
>> Setting the verbose level to some > 0 value will dump all packets in the
>> associated rx/tx callback to further help in the debugging.
>>
>> Signed-off-by: David Marchand <david.marchand@6wind.com>
>> ---
>>  app/test-pmd/cmdline.c    |  63 +++++++++++++++++++
>>  app/test-pmd/config.c     |  23 +++++++
>>  app/test-pmd/parameters.c |   7 +++
>>  app/test-pmd/testpmd.c    | 123
>> ++++++++++++++++++++++++++++++++++++++
>>  app/test-pmd/testpmd.h    |   9 +++
>>  5 files changed, 225 insertions(+)
>
> There should probably be an entry in section 4.6 of the Testpmd User Guide for the
> "port config all sanity_check"  command.

Sure, will do.


-- 
David Marchand

Re: [dpdk-dev] [PATCH 2/3] mbuf: add a non fatal sanity check helper

[Olivier Matz]

On Mon, Sep 10, 2018 at 11:33:44AM +0300, Andrew Rybchenko wrote:
> On 09/10/2018 11:24 AM, David Marchand wrote:
> > On Mon, Sep 10, 2018 at 10:12 AM, Andrew Rybchenko
> > <arybchenko@solarflare.com> wrote:
> > > +/**
> > > + * Sanity checks on a mbuf.
> > > + *
> > > + * Almost like rte_mbuf_sanity_check(), but this function gives the reason
> > > + * if corruption is detected rather than panic.
> > > + *
> > > + * @param m
> > > + *   The mbuf to be checked.
> > > + * @param is_header
> > > + *   True if the mbuf is a packet header, false if it is a sub-segment
> > > + *   of a packet (in this case, some fields like nb_segs are not checked)
> > > + * @param reason
> > > + *   A reference to a string pointer where to store the reason why a mbuf
> > > is
> > > + *   considered invalid.
> > > + * @return
> > > + *   - 0 if no issue has been found, reason is left untouched.
> > > + *   - -1 if a problem is detected, reason then points to a string
> > > describing
> > > + *     the reason why the mbuf is deemed invalid.
> > > + */
> > > +__rte_experimental
> > > +int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
> > > +   const char **reason);
> > > +
> > > 
> > > 
> > > May be it would be better to return reason as return value and if it is NULL
> > > everything is OK?
> > This was what I had done with my first attempt.
> > But given the name "rte_mbuf_check", having a int (used as a bool)
> > returned makes sense to me.
> 
> Yes, good point.
> 
> > So no strong opinion here.
> 
> Me too.

I think an int is fine.

Acked-by: Olivier Matz <olivier.matz@6wind.com>

Re: [dpdk-dev] [PATCH 1/3] mbuf: add sanity checks on segment metadata

[Olivier Matz]

Hi David,

On Thu, Sep 13, 2018 at 08:55:40AM +0200, David Marchand wrote:
> Hello Yongseok,
> 
> On Tue, Sep 11, 2018 at 8:16 PM, Yongseok Koh <yskoh@mellanox.com> wrote:
> >
> >> On Sep 9, 2018, at 10:45 PM, David Marchand <david.marchand@6wind.com> wrote:
> >>
> >> Add some basic checks on the segments offset and length metadata:
> >> always funny to have a < 0 tailroom cast to uint16_t ;-).
> >>
> >> Signed-off-by: David Marchand <david.marchand@6wind.com>
> >> ---
> >> lib/librte_mbuf/rte_mbuf.c | 5 +++++
> >> 1 file changed, 5 insertions(+)
> >>
> >> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> >> index e714c5a59..137a320ed 100644
> >> --- a/lib/librte_mbuf/rte_mbuf.c
> >> +++ b/lib/librte_mbuf/rte_mbuf.c
> >> @@ -200,6 +200,11 @@ rte_mbuf_sanity_check(const struct rte_mbuf *m, int is_header)
> >>       pkt_len = m->pkt_len;
> >>
> >>       do {
> >> +             if (m->data_off > m->buf_len)
> >> +                     rte_panic("data offset too big in mbuf segment\n");
> >> +             if ((uint32_t)m->data_off + (uint32_t)m->data_len >
> >> +                             (uint32_t)m->buf_len)
> >
> > Casting to uint32_t is needed? All of the three fields are uint16_t and it would
> > anyway happen because of the integer promotion rule. Right?
> 
> Indeed, this is unnecessary.
> Will send a v2 without this.

You can add my ack in your v2 with this change.

Thanks,
Olivier