[PATCH v2 0/3] segment sanity checks

[PATCH v2 0/3] segment sanity checks

[David Marchand]

Resubmitting this series that I did not finish in my previous life (6WIND
people are okay with this).

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.

Changelog since v1:
- dropped unnecessary casts in patch 1,
- rewrote patch 3: reused the existing rx/tx callbacks and left the invalid
  mbufs in rx bulk

Example (with [1] that generates invalid mbufs):
./testpmd --no-huge -l 2,3 -m 512 --log-level *:debug --vdev=eth_ring0
 --vdev=eth_ring1 -- -i --total-num-mbufs 2048

[...]

testpmd> set verbose 1
testpmd> set burst 4
Number of packets per burst set to 4
testpmd> start tx_first
port 0/queue 0: received 4 packets
  src=00:00:00:00:00:00 - dst=02:00:00:00:00:00 - type=0x0800 - length=64
 - nb_segs=2 - sw ptype: L2_ETHER L3_IPV4 L4_UDP  - l2_len=14 - l3_len=20
 - l4_len=8 - Receive queue=0x0
  ol_flags: PKT_RX_L4_CKSUM_UNKNOWN PKT_RX_IP_CKSUM_UNKNOWN
 PKT_RX_OUTER_L4_CKSUM_UNKNOWN 
INVALID mbuf: bad nb_segs
  src=00:00:00:00:00:00 - dst=02:00:00:00:00:00 - type=0x0800 - length=64
 - nb_segs=1 - sw ptype: L2_ETHER L3_IPV4 L4_UDP  - l2_len=14 - l3_len=20
 - l4_len=8 - Receive queue=0x0
  ol_flags: PKT_RX_L4_CKSUM_UNKNOWN PKT_RX_IP_CKSUM_UNKNOWN
 PKT_RX_OUTER_L4_CKSUM_UNKNOWN 
INVALID mbuf: bad pkt_len
  src=00:00:00:00:00:00 - dst=02:00:00:00:00:00 - type=0x0800 - length=64
 - nb_segs=1 - sw ptype: L2_ETHER L3_IPV4 L4_UDP  - l2_len=14 - l3_len=20
 - l4_len=8 - Receive queue=0x0
  ol_flags: PKT_RX_L4_CKSUM_UNKNOWN PKT_RX_IP_CKSUM_UNKNOWN
 PKT_RX_OUTER_L4_CKSUM_UNKNOWN 
INVALID mbuf: bad nb_segs
  src=00:00:00:00:00:00 - dst=02:00:00:00:00:00 - type=0x0800 - length=64
 - nb_segs=2 - sw ptype: L2_ETHER L3_IPV4 L4_UDP  - l2_len=14 - l3_len=20
 - l4_len=8 - Receive queue=0x0
  ol_flags: PKT_RX_L4_CKSUM_UNKNOWN PKT_RX_IP_CKSUM_UNKNOWN
 PKT_RX_OUTER_L4_CKSUM_UNKNOWN 
INVALID mbuf: bad nb_segs
port 1/queue 0: received 4 packets
[...]
testpmd> stop


1: https://github.com/david-marchand/dpdk/commit/601630d8db0e

-- 
David Marchand

David Marchand (3):
  mbuf: add sanity checks on segment metadata
  mbuf: add a non fatal sanity check helper
  app/testpmd: check mbufs in verbose mode

 app/test-pmd/util.c                  |  3 ++
 lib/librte_mbuf/Makefile             |  2 ++
 lib/librte_mbuf/meson.build          |  2 ++
 lib/librte_mbuf/rte_mbuf.c           | 70 +++++++++++++++++++++++++++---------
 lib/librte_mbuf/rte_mbuf.h           | 23 ++++++++++++
 lib/librte_mbuf/rte_mbuf_version.map |  6 ++++
 6 files changed, 89 insertions(+), 17 deletions(-)

-- 
1.8.3.1

[PATCH v2 1/3] mbuf: add sanity checks on segment metadata

[David Marchand]

From: David Marchand <david.marchand@6wind.com>

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>
Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
---
 lib/librte_mbuf/rte_mbuf.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 9790b4f..3bbd3f5 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -200,6 +200,10 @@ struct rte_mempool *
 	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->data_len > 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);
-- 
1.8.3.1

[PATCH v2 2/3] mbuf: add a non fatal sanity check helper

[David Marchand]

From: David Marchand <david.marchand@6wind.com>

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>
Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Olivier Matz <olivier.matz@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 8c4c7d7..c8f6d26 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 e37da02..6cc11eb 100644
--- a/lib/librte_mbuf/meson.build
+++ b/lib/librte_mbuf/meson.build
@@ -5,3 +5,5 @@ version = 5
 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 3bbd3f5..21f6f74 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -171,47 +171,79 @@ struct rte_mempool *
 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->data_len > m->buf_len)
-			rte_panic("data length too big in mbuf segment\n");
+		if (m->data_off > m->buf_len) {
+			*reason = "data offset too big in mbuf segment";
+			return -1;
+		}
+		if (m->data_off + m->data_len > 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 bc562dc..564e08c 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1052,6 +1052,29 @@ struct rte_pktmbuf_pool_private {
 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 cae68db..2662a37 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;
-- 
1.8.3.1

[PATCH v2 3/3] app/testpmd: check mbufs in verbose mode

[David Marchand]

Let's check the received/sent mbufs, it can help debugging.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 app/test-pmd/util.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/app/test-pmd/util.c b/app/test-pmd/util.c
index 687bfa4..6b0791d 100644
--- a/app/test-pmd/util.c
+++ b/app/test-pmd/util.c
@@ -36,6 +36,7 @@
 	uint32_t sw_packet_type;
 	uint16_t udp_port;
 	uint32_t vx_vni;
+	const char *reason;
 
 	if (!nb_pkts)
 		return;
@@ -147,6 +148,8 @@
 		printf("\n");
 		rte_get_rx_ol_flag_list(mb->ol_flags, buf, sizeof(buf));
 		printf("  ol_flags: %s\n", buf);
+		if (rte_mbuf_check(mb, 1, &reason) < 0)
+			printf("INVALID mbuf: %s\n", reason);
 	}
 }
 
-- 
1.8.3.1

Re: [PATCH v2 3/3] app/testpmd: check mbufs in verbose mode

[Iremonger, Bernard]

> -----Original Message-----
> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Monday, January 7, 2019 8:57 AM
> To: dev@dpdk.org
> Cc: olivier.matz@6wind.com; yskoh@mellanox.com;
> arybchenko@solarflare.com; Iremonger, Bernard
> <bernard.iremonger@intel.com>
> Subject: [PATCH v2 3/3] app/testpmd: check mbufs in verbose mode
> 
> Let's check the received/sent mbufs, it can help debugging.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>

Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>

Re: [PATCH v2 0/3] segment sanity checks

[Thomas Monjalon]

07/01/2019 09:57, David Marchand:
> Resubmitting this series that I did not finish in my previous life (6WIND
> people are okay with this).
> 
> 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.
> 
> Changelog since v1:
> - dropped unnecessary casts in patch 1,
> - rewrote patch 3: reused the existing rx/tx callbacks and left the invalid
>   mbufs in rx bulk

Applied, thanks