[dpdk-dev] [PATCH 0/4] ip_frag: add a unit test for fragmentation

[dpdk-dev] [PATCH 0/4] ip_frag: add a unit test for fragmentation

[Aaron Conole]

This adds a simple unit test for the ip fragmentation library
and covers fragmenting ipv4 and ipv6.  Additionally, some fixes
are introduced, which are really just sanity rather than real
issues in the field.

Aaron Conole (4):
  ip_frag: ensure minimum v4 fragmentation length
  ip_frag: ensure minimum v6 fragmentation length
  ip_frag: ipv6 fragments must not be resubmitted to fragmentation
  ipfrag: add unit test case

 MAINTAINERS                                 |   1 +
 app/test/meson.build                        |   2 +
 app/test/test_ipfrag.c                      | 276 ++++++++++++++++++++
 lib/librte_ip_frag/rte_ipv4_fragmentation.c |   6 +
 lib/librte_ip_frag/rte_ipv6_fragmentation.c |  15 ++
 5 files changed, 300 insertions(+)
 create mode 100644 app/test/test_ipfrag.c

-- 
2.25.1

[dpdk-dev] [PATCH 1/4] ip_frag: ensure minimum v4 fragmentation length

[Aaron Conole]

The IPv4 specification says that each fragment must at least the size of
an IP header plus 8 octets.  When attempting to run ipfrag using a
smaller size, the fragment library will return successful completion,
even though it is a violation of RFC791 (and updates).

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 lib/librte_ip_frag/rte_ipv4_fragmentation.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/librte_ip_frag/rte_ipv4_fragmentation.c b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
index 9e9f986cc5..4baaf6355c 100644
--- a/lib/librte_ip_frag/rte_ipv4_fragmentation.c
+++ b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
@@ -76,6 +76,12 @@ rte_ipv4_fragment_packet(struct rte_mbuf *pkt_in,
 	uint16_t fragment_offset, flag_offset, frag_size;
 	uint16_t frag_bytes_remaining;
 
+	/*
+	 * Ensure the IP fragmentation size is at least iphdr length + 8 octets
+	 */
+	if (unlikely(mtu_size < (sizeof(struct rte_ipv4_hdr) + 8*sizeof(char))))
+		return -EINVAL;
+
 	/*
 	 * Ensure the IP payload length of all fragments is aligned to a
 	 * multiple of 8 bytes as per RFC791 section 2.3.
-- 
2.25.1

[dpdk-dev] [PATCH 2/4] ip_frag: ensure minimum v6 fragmentation length

[Aaron Conole]

Similar to v4, we should ensure that at least a minimum fragmentation
length can be achieved.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 lib/librte_ip_frag/rte_ipv6_fragmentation.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/lib/librte_ip_frag/rte_ipv6_fragmentation.c b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
index 43449970e5..820a5dc725 100644
--- a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
+++ b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
@@ -79,6 +79,17 @@ rte_ipv6_fragment_packet(struct rte_mbuf *pkt_in,
 	uint16_t fragment_offset, frag_size;
 	uint64_t frag_bytes_remaining;
 
+	/*
+	 * The rules on ipv6 fragmentation means we need at least to be
+	 * greater than the smallest fragment possible - ipv6hdr + frag header
+	 * + min octets.  We rely on the ipv6_extension_fragment header to
+	 * include 'data' octets.
+	 */
+	if (unlikely(mtu_size <
+		     (sizeof(struct rte_ipv6_hdr) +
+		      sizeof(struct ipv6_extension_fragment))))
+		return -EINVAL;
+
 	/*
 	 * Ensure the IP payload length of all fragments (except the
 	 * the last fragment) are a multiple of 8 bytes per RFC2460.
-- 
2.25.1

[dpdk-dev] [PATCH 3/4] ip_frag: ipv6 fragments must not be resubmitted to fragmentation

[Aaron Conole]

IPv6 only allows traffic source nodes to fragment, so submitting
a packet with next header of IPPROTO_FRAGMENT would be invalid.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 lib/librte_ip_frag/rte_ipv6_fragmentation.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/librte_ip_frag/rte_ipv6_fragmentation.c b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
index 820a5dc725..aebcfa4325 100644
--- a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
+++ b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
@@ -106,6 +106,10 @@ rte_ipv6_fragment_packet(struct rte_mbuf *pkt_in,
 
 	in_hdr = rte_pktmbuf_mtod(pkt_in, struct rte_ipv6_hdr *);
 
+	/* Fragmenting a fragmented packet?! */
+	if (unlikely(in_hdr->proto == IPPROTO_FRAGMENT))
+		return -ENOTSUP;
+
 	in_seg = pkt_in;
 	in_seg_data_pos = sizeof(struct rte_ipv6_hdr);
 	out_pkt_pos = 0;
-- 
2.25.1

[dpdk-dev] [PATCH 4/4] ipfrag: add unit test case

[Aaron Conole]

Initial IP fragmentation unit test.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 MAINTAINERS            |   1 +
 app/test/meson.build   |   2 +
 app/test/test_ipfrag.c | 276 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 279 insertions(+)
 create mode 100644 app/test/test_ipfrag.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 4800f6884a..c81f07ac9c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1230,6 +1230,7 @@ F: app/test/test_crc.c
 IP fragmentation & reassembly
 M: Konstantin Ananyev <konstantin.ananyev@intel.com>
 F: lib/librte_ip_frag/
+F: app/test/test_ipfrag.c
 F: doc/guides/prog_guide/ip_fragment_reassembly_lib.rst
 F: examples/ip_fragmentation/
 F: doc/guides/sample_app_ug/ip_frag.rst
diff --git a/app/test/meson.build b/app/test/meson.build
index 351d29cb65..c2b07fa62f 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -58,6 +58,7 @@ test_sources = files('commands.c',
 	'test_hash_perf.c',
 	'test_hash_readwrite_lf_perf.c',
 	'test_interrupts.c',
+        'test_ipfrag.c',
 	'test_ipsec.c',
 	'test_ipsec_sad.c',
 	'test_kni.c',
@@ -187,6 +188,7 @@ fast_tests = [
         ['flow_classify_autotest', false],
         ['hash_autotest', true],
         ['interrupt_autotest', true],
+        ['ipfrag_autotest', false],
         ['logs_autotest', true],
         ['lpm_autotest', true],
         ['lpm6_autotest', true],
diff --git a/app/test/test_ipfrag.c b/app/test/test_ipfrag.c
new file mode 100644
index 0000000000..c881effe8b
--- /dev/null
+++ b/app/test/test_ipfrag.c
@@ -0,0 +1,276 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2020 Red Hat, Inc.
+ */
+
+#include <time.h>
+
+#include <rte_common.h>
+#include <rte_cycles.h>
+#include <rte_hexdump.h>
+#include <rte_ip.h>
+#include <rte_ip_frag.h>
+#include <rte_mbuf.h>
+#include <rte_memcpy.h>
+#include <rte_random.h>
+
+#include "test.h"
+
+#ifndef ARRAY_SIZE
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+#endif
+
+static struct rte_mempool *pkt_pool,
+			  *direct_pool,
+			  *indirect_pool;
+
+static int
+setup_buf_pool(void)
+{
+#define NUM_MBUFS (128)
+#define BURST 32
+
+	if (!pkt_pool)
+		pkt_pool = rte_pktmbuf_pool_create("FRAG_MBUF_POOL",
+						   NUM_MBUFS, BURST, 0,
+						   RTE_MBUF_DEFAULT_BUF_SIZE,
+						   SOCKET_ID_ANY);
+	if (pkt_pool == NULL) {
+		printf("%s: Error creating pkt mempool\n", __func__);
+		goto bad_setup;
+	}
+
+	if (!direct_pool)
+		direct_pool = rte_pktmbuf_pool_create("FRAG_D_MBUF_POOL",
+						      NUM_MBUFS, BURST, 0,
+						      RTE_MBUF_DEFAULT_BUF_SIZE,
+						      SOCKET_ID_ANY);
+	if (!direct_pool) {
+		printf("%s: Error creating direct mempool\n", __func__);
+		goto bad_setup;
+	}
+
+	if (!indirect_pool)
+		indirect_pool = rte_pktmbuf_pool_create("FRAG_I_MBUF_POOL",
+							NUM_MBUFS, BURST, 0,
+							0, SOCKET_ID_ANY);
+	if (!indirect_pool) {
+		printf("%s: Error creating indirect mempool\n", __func__);
+		goto bad_setup;
+	}
+
+	return 0;
+
+bad_setup:
+	if (pkt_pool)
+		rte_mempool_free(pkt_pool);
+
+	if (direct_pool)
+		rte_mempool_free(direct_pool);
+
+	return TEST_FAILED;
+}
+
+static int testsuite_setup(void)
+{
+	if (setup_buf_pool())
+		return TEST_FAILED;
+	return TEST_SUCCESS;
+}
+
+static void testsuite_teardown(void)
+{
+	if (pkt_pool)
+		rte_mempool_free(pkt_pool);
+
+	if (direct_pool)
+		rte_mempool_free(direct_pool);
+
+	if (indirect_pool)
+		rte_mempool_free(indirect_pool);
+
+	pkt_pool = NULL;
+}
+
+static int ut_setup(void)
+{
+	return TEST_SUCCESS;
+}
+
+static void ut_teardown(void)
+{
+}
+
+static int
+v4_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s, int df,
+		      uint8_t ttl, uint8_t proto, uint16_t pktid)
+{
+	/* Create a packet, 2k bytes long */
+	b->data_off = 0;
+	char *data = rte_pktmbuf_mtod(b, char *);
+
+	memset(data, fill, sizeof(struct rte_ipv4_hdr) + s);
+
+	struct rte_ipv4_hdr *hdr = (struct rte_ipv4_hdr *)data;
+
+	hdr->version_ihl = 0x45; /* standard IP header... */
+	hdr->type_of_service = 0;
+	b->pkt_len = s + sizeof(struct rte_ipv4_hdr);
+	b->data_len = b->pkt_len;
+	hdr->total_length = htonl(b->pkt_len);
+	hdr->packet_id = htons(pktid);
+	hdr->fragment_offset = 0;
+	if (df)
+		hdr->fragment_offset = htons(0x4000);
+
+	if (!ttl)
+		ttl = 64; /* default to 64 */
+
+	if (!proto)
+		proto = 1; /* icmp */
+
+	hdr->time_to_live = ttl;
+	hdr->next_proto_id = proto;
+	hdr->hdr_checksum = 0;
+	hdr->src_addr = htonl(0x8080808);
+	hdr->dst_addr = htonl(0x8080404);
+
+	return 0;
+}
+
+static int
+v6_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s, uint8_t ttl,
+		      uint8_t proto, uint16_t pktid)
+{
+	/* Create a packet, 2k bytes long */
+	b->data_off = 0;
+	char *data = rte_pktmbuf_mtod(b, char *);
+
+	memset(data, fill, sizeof(struct rte_ipv6_hdr) + s);
+
+	struct rte_ipv6_hdr *hdr = (struct rte_ipv6_hdr *)data;
+	b->pkt_len = s + sizeof(struct rte_ipv6_hdr);
+	b->data_len = b->pkt_len;
+
+	/* basic v6 header */
+	hdr->vtc_flow = htonl(0x60 << 24 | pktid);
+	hdr->payload_len = htons(b->pkt_len);
+	hdr->proto = proto;
+	hdr->hop_limits = ttl;
+
+	memset(hdr->src_addr, 0x08, sizeof(hdr->src_addr));
+	memset(hdr->dst_addr, 0x04, sizeof(hdr->src_addr));
+
+	return 0;
+}
+
+static inline void
+test_free_fragments(struct rte_mbuf *mb[], uint32_t num)
+{
+	uint32_t i;
+	for (i = 0; i < num; i++)
+		rte_pktmbuf_free(mb[i]);
+}
+
+static int
+test_ip_frag(void)
+{
+	int result = TEST_SUCCESS;
+
+	struct test_ip_frags {
+		int     ipv;
+		size_t  mtu_size;
+		size_t  pkt_size;
+		int     set_df;
+		int     ttl;
+		uint8_t proto;
+		int     pkt_id;
+		int     expected_frags;
+	} tests[] = {
+		     {4, 1280, 1400, 0, 64, IPPROTO_ICMP, -1, 2},
+		     {4, 1280, 1400, 0, 64, IPPROTO_ICMP,  0, 2},
+		     {4,  600, 1400, 0, 64, IPPROTO_ICMP, -1, 3},
+		     {4,    4, 1400, 0, 64, IPPROTO_ICMP, -1, -EINVAL},
+		     {4,  600, 1400, 1, 64, IPPROTO_ICMP, -1, -ENOTSUP},
+		     {4,  600, 1400, 0,  0, IPPROTO_ICMP, -1, 3},
+
+		     {6, 1280, 1400, 0, 64, IPPROTO_ICMP, -1, 2},
+		     {6, 1300, 1400, 0, 64, IPPROTO_ICMP, -1, 2},
+		     {6,    4, 1400, 0, 64, IPPROTO_ICMP, -1, -EINVAL},
+		     {6, 1300, 1400, 0,  0, IPPROTO_ICMP, -1, 2},
+		     {6, 1280, 1400, 0, 64, IPPROTO_FRAGMENT, -1, -ENOTSUP},
+	};
+
+	for (size_t i = 0; i < ARRAY_SIZE(tests); i++) {
+		int32_t len;
+		uint16_t pktid;
+		struct rte_mbuf *pkts_out[BURST];
+		struct rte_mbuf *b = rte_pktmbuf_alloc(pkt_pool);
+
+		if (!b)
+			return TEST_FAILED; /* Serious error.. abort here */
+
+		if (tests[i].pkt_id == -1)
+			pktid = rte_rand_max(UINT16_MAX);
+
+		if (tests[i].ipv == 4) {
+			if (v4_allocate_packet_of(b, 0x41414141,
+						  tests[i].pkt_size,
+						  tests[i].set_df,
+						  tests[i].ttl,
+						  tests[i].proto,
+						  pktid))
+				result = TEST_FAILED;
+		} else if (tests[i].ipv == 6) {
+			if (v6_allocate_packet_of(b, 0x41414141,
+						  tests[i].pkt_size,
+						  tests[i].ttl,
+						  tests[i].proto,
+						  pktid))
+				result = TEST_FAILED;
+		}
+
+		if (tests[i].ipv == 4)
+			len = rte_ipv4_fragment_packet(b, pkts_out, BURST,
+						       tests[i].mtu_size,
+						       direct_pool,
+						       indirect_pool);
+		else
+			len = rte_ipv6_fragment_packet(b, pkts_out, BURST,
+						       tests[i].mtu_size,
+						       direct_pool,
+						       indirect_pool);
+
+		rte_pktmbuf_free(b);
+
+		if (len > 0)
+			test_free_fragments(pkts_out, len);
+
+		printf("%d: checking %d with %d\n", (int)i, len,
+		       (int)tests[i].expected_frags);
+		RTE_TEST_ASSERT_EQUAL(len, tests[i].expected_frags,
+				      "Failed case %u\n", (unsigned int)i);
+
+	}
+
+	return result;
+}
+
+static struct unit_test_suite ipfrag_testsuite  = {
+	.suite_name = "IP Frag Unit Test Suite",
+	.setup = testsuite_setup,
+	.teardown = testsuite_teardown,
+	.unit_test_cases = {
+		TEST_CASE_ST(ut_setup, ut_teardown,
+			     test_ip_frag),
+
+		TEST_CASES_END() /**< NULL terminate unit test array */
+	}
+};
+
+static int
+test_ipfrag(void)
+{
+	return unit_test_suite_runner(&ipfrag_testsuite);
+}
+
+REGISTER_TEST_COMMAND(ipfrag_autotest, test_ipfrag);
-- 
2.25.1

Re: [dpdk-dev] |WARNING| pw67494 ipfrag: add unit test case

[Aaron Conole]

0-day Robot <robot@bytheb.org> writes:

> From: robot@bytheb.org
>
> Test-Label: travis-robot
> Test-Status: WARNING
> http://dpdk.org/patch/67494
>
> _Travis build: failed_
> Build URL: https://travis-ci.com/ovsrobot/dpdk/builds/157275800

D'oh!   Thanks 0-day Robot.  :-/ v2 coming soon.

[dpdk-dev] [PATCH v2 0/4] ip_frag: add a unit test for fragmentation

[Aaron Conole]

This adds a simple unit test for the ip fragmentation library
and covers fragmenting ipv4 and ipv6.  Additionally, some fixes
are introduced, which are really just sanity rather than real
issues in the field.

v1->v2:
- Fix patch 4/4 which had a missing assignment for pktid.

Aaron Conole (4):
  ip_frag: ensure minimum v4 fragmentation length
  ip_frag: ensure minimum v6 fragmentation length
  ip_frag: ipv6 fragments must not be resubmitted to fragmentation
  ipfrag: add unit test case

 MAINTAINERS                                 |   1 +
 app/test/meson.build                        |   2 +
 app/test/test_ipfrag.c                      | 276 ++++++++++++++++++++
 lib/librte_ip_frag/rte_ipv4_fragmentation.c |   6 +
 lib/librte_ip_frag/rte_ipv6_fragmentation.c |  15 ++
 5 files changed, 300 insertions(+)
 create mode 100644 app/test/test_ipfrag.c

-- 
2.25.1

[dpdk-dev] [PATCH v2 1/4] ip_frag: ensure minimum v4 fragmentation length

[Aaron Conole]

The IPv4 specification says that each fragment must at least the size of
an IP header plus 8 octets.  When attempting to run ipfrag using a
smaller size, the fragment library will return successful completion,
even though it is a violation of RFC791 (and updates).

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 lib/librte_ip_frag/rte_ipv4_fragmentation.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/librte_ip_frag/rte_ipv4_fragmentation.c b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
index 9e9f986cc5..4baaf6355c 100644
--- a/lib/librte_ip_frag/rte_ipv4_fragmentation.c
+++ b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
@@ -76,6 +76,12 @@ rte_ipv4_fragment_packet(struct rte_mbuf *pkt_in,
 	uint16_t fragment_offset, flag_offset, frag_size;
 	uint16_t frag_bytes_remaining;
 
+	/*
+	 * Ensure the IP fragmentation size is at least iphdr length + 8 octets
+	 */
+	if (unlikely(mtu_size < (sizeof(struct rte_ipv4_hdr) + 8*sizeof(char))))
+		return -EINVAL;
+
 	/*
 	 * Ensure the IP payload length of all fragments is aligned to a
 	 * multiple of 8 bytes as per RFC791 section 2.3.
-- 
2.25.1

[dpdk-dev] [PATCH v2 2/4] ip_frag: ensure minimum v6 fragmentation length

[Aaron Conole]

Similar to v4, we should ensure that at least a minimum fragmentation
length can be achieved.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 lib/librte_ip_frag/rte_ipv6_fragmentation.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/lib/librte_ip_frag/rte_ipv6_fragmentation.c b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
index 43449970e5..820a5dc725 100644
--- a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
+++ b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
@@ -79,6 +79,17 @@ rte_ipv6_fragment_packet(struct rte_mbuf *pkt_in,
 	uint16_t fragment_offset, frag_size;
 	uint64_t frag_bytes_remaining;
 
+	/*
+	 * The rules on ipv6 fragmentation means we need at least to be
+	 * greater than the smallest fragment possible - ipv6hdr + frag header
+	 * + min octets.  We rely on the ipv6_extension_fragment header to
+	 * include 'data' octets.
+	 */
+	if (unlikely(mtu_size <
+		     (sizeof(struct rte_ipv6_hdr) +
+		      sizeof(struct ipv6_extension_fragment))))
+		return -EINVAL;
+
 	/*
 	 * Ensure the IP payload length of all fragments (except the
 	 * the last fragment) are a multiple of 8 bytes per RFC2460.
-- 
2.25.1

[dpdk-dev] [PATCH v2 3/4] ip_frag: ipv6 fragments must not be resubmitted to fragmentation

[Aaron Conole]

IPv6 only allows traffic source nodes to fragment, so submitting
a packet with next header of IPPROTO_FRAGMENT would be invalid.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 lib/librte_ip_frag/rte_ipv6_fragmentation.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/librte_ip_frag/rte_ipv6_fragmentation.c b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
index 820a5dc725..aebcfa4325 100644
--- a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
+++ b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
@@ -106,6 +106,10 @@ rte_ipv6_fragment_packet(struct rte_mbuf *pkt_in,
 
 	in_hdr = rte_pktmbuf_mtod(pkt_in, struct rte_ipv6_hdr *);
 
+	/* Fragmenting a fragmented packet?! */
+	if (unlikely(in_hdr->proto == IPPROTO_FRAGMENT))
+		return -ENOTSUP;
+
 	in_seg = pkt_in;
 	in_seg_data_pos = sizeof(struct rte_ipv6_hdr);
 	out_pkt_pos = 0;
-- 
2.25.1

[dpdk-dev] [PATCH v2 4/4] ipfrag: add unit test case

[Aaron Conole]

Initial IP fragmentation unit test.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 MAINTAINERS            |   1 +
 app/test/meson.build   |   2 +
 app/test/test_ipfrag.c | 276 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 279 insertions(+)
 create mode 100644 app/test/test_ipfrag.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 4800f6884a..c81f07ac9c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1230,6 +1230,7 @@ F: app/test/test_crc.c
 IP fragmentation & reassembly
 M: Konstantin Ananyev <konstantin.ananyev@intel.com>
 F: lib/librte_ip_frag/
+F: app/test/test_ipfrag.c
 F: doc/guides/prog_guide/ip_fragment_reassembly_lib.rst
 F: examples/ip_fragmentation/
 F: doc/guides/sample_app_ug/ip_frag.rst
diff --git a/app/test/meson.build b/app/test/meson.build
index 351d29cb65..c2b07fa62f 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -58,6 +58,7 @@ test_sources = files('commands.c',
 	'test_hash_perf.c',
 	'test_hash_readwrite_lf_perf.c',
 	'test_interrupts.c',
+        'test_ipfrag.c',
 	'test_ipsec.c',
 	'test_ipsec_sad.c',
 	'test_kni.c',
@@ -187,6 +188,7 @@ fast_tests = [
         ['flow_classify_autotest', false],
         ['hash_autotest', true],
         ['interrupt_autotest', true],
+        ['ipfrag_autotest', false],
         ['logs_autotest', true],
         ['lpm_autotest', true],
         ['lpm6_autotest', true],
diff --git a/app/test/test_ipfrag.c b/app/test/test_ipfrag.c
new file mode 100644
index 0000000000..4cdc1dcd54
--- /dev/null
+++ b/app/test/test_ipfrag.c
@@ -0,0 +1,276 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2020 Red Hat, Inc.
+ */
+
+#include <time.h>
+
+#include <rte_common.h>
+#include <rte_cycles.h>
+#include <rte_hexdump.h>
+#include <rte_ip.h>
+#include <rte_ip_frag.h>
+#include <rte_mbuf.h>
+#include <rte_memcpy.h>
+#include <rte_random.h>
+
+#include "test.h"
+
+#ifndef ARRAY_SIZE
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+#endif
+
+static struct rte_mempool *pkt_pool,
+			  *direct_pool,
+			  *indirect_pool;
+
+static int
+setup_buf_pool(void)
+{
+#define NUM_MBUFS (128)
+#define BURST 32
+
+	if (!pkt_pool)
+		pkt_pool = rte_pktmbuf_pool_create("FRAG_MBUF_POOL",
+						   NUM_MBUFS, BURST, 0,
+						   RTE_MBUF_DEFAULT_BUF_SIZE,
+						   SOCKET_ID_ANY);
+	if (pkt_pool == NULL) {
+		printf("%s: Error creating pkt mempool\n", __func__);
+		goto bad_setup;
+	}
+
+	if (!direct_pool)
+		direct_pool = rte_pktmbuf_pool_create("FRAG_D_MBUF_POOL",
+						      NUM_MBUFS, BURST, 0,
+						      RTE_MBUF_DEFAULT_BUF_SIZE,
+						      SOCKET_ID_ANY);
+	if (!direct_pool) {
+		printf("%s: Error creating direct mempool\n", __func__);
+		goto bad_setup;
+	}
+
+	if (!indirect_pool)
+		indirect_pool = rte_pktmbuf_pool_create("FRAG_I_MBUF_POOL",
+							NUM_MBUFS, BURST, 0,
+							0, SOCKET_ID_ANY);
+	if (!indirect_pool) {
+		printf("%s: Error creating indirect mempool\n", __func__);
+		goto bad_setup;
+	}
+
+	return 0;
+
+bad_setup:
+	if (pkt_pool)
+		rte_mempool_free(pkt_pool);
+
+	if (direct_pool)
+		rte_mempool_free(direct_pool);
+
+	return TEST_FAILED;
+}
+
+static int testsuite_setup(void)
+{
+	if (setup_buf_pool())
+		return TEST_FAILED;
+	return TEST_SUCCESS;
+}
+
+static void testsuite_teardown(void)
+{
+	if (pkt_pool)
+		rte_mempool_free(pkt_pool);
+
+	if (direct_pool)
+		rte_mempool_free(direct_pool);
+
+	if (indirect_pool)
+		rte_mempool_free(indirect_pool);
+
+	pkt_pool = NULL;
+}
+
+static int ut_setup(void)
+{
+	return TEST_SUCCESS;
+}
+
+static void ut_teardown(void)
+{
+}
+
+static int
+v4_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s, int df,
+		      uint8_t ttl, uint8_t proto, uint16_t pktid)
+{
+	/* Create a packet, 2k bytes long */
+	b->data_off = 0;
+	char *data = rte_pktmbuf_mtod(b, char *);
+
+	memset(data, fill, sizeof(struct rte_ipv4_hdr) + s);
+
+	struct rte_ipv4_hdr *hdr = (struct rte_ipv4_hdr *)data;
+
+	hdr->version_ihl = 0x45; /* standard IP header... */
+	hdr->type_of_service = 0;
+	b->pkt_len = s + sizeof(struct rte_ipv4_hdr);
+	b->data_len = b->pkt_len;
+	hdr->total_length = htonl(b->pkt_len);
+	hdr->packet_id = htons(pktid);
+	hdr->fragment_offset = 0;
+	if (df)
+		hdr->fragment_offset = htons(0x4000);
+
+	if (!ttl)
+		ttl = 64; /* default to 64 */
+
+	if (!proto)
+		proto = 1; /* icmp */
+
+	hdr->time_to_live = ttl;
+	hdr->next_proto_id = proto;
+	hdr->hdr_checksum = 0;
+	hdr->src_addr = htonl(0x8080808);
+	hdr->dst_addr = htonl(0x8080404);
+
+	return 0;
+}
+
+static int
+v6_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s, uint8_t ttl,
+		      uint8_t proto, uint16_t pktid)
+{
+	/* Create a packet, 2k bytes long */
+	b->data_off = 0;
+	char *data = rte_pktmbuf_mtod(b, char *);
+
+	memset(data, fill, sizeof(struct rte_ipv6_hdr) + s);
+
+	struct rte_ipv6_hdr *hdr = (struct rte_ipv6_hdr *)data;
+	b->pkt_len = s + sizeof(struct rte_ipv6_hdr);
+	b->data_len = b->pkt_len;
+
+	/* basic v6 header */
+	hdr->vtc_flow = htonl(0x60 << 24 | pktid);
+	hdr->payload_len = htons(b->pkt_len);
+	hdr->proto = proto;
+	hdr->hop_limits = ttl;
+
+	memset(hdr->src_addr, 0x08, sizeof(hdr->src_addr));
+	memset(hdr->dst_addr, 0x04, sizeof(hdr->src_addr));
+
+	return 0;
+}
+
+static inline void
+test_free_fragments(struct rte_mbuf *mb[], uint32_t num)
+{
+	uint32_t i;
+	for (i = 0; i < num; i++)
+		rte_pktmbuf_free(mb[i]);
+}
+
+static int
+test_ip_frag(void)
+{
+	int result = TEST_SUCCESS;
+
+	struct test_ip_frags {
+		int     ipv;
+		size_t  mtu_size;
+		size_t  pkt_size;
+		int     set_df;
+		int     ttl;
+		uint8_t proto;
+		int     pkt_id;
+		int     expected_frags;
+	} tests[] = {
+		     {4, 1280, 1400, 0, 64, IPPROTO_ICMP, -1, 2},
+		     {4, 1280, 1400, 0, 64, IPPROTO_ICMP,  0, 2},
+		     {4,  600, 1400, 0, 64, IPPROTO_ICMP, -1, 3},
+		     {4,    4, 1400, 0, 64, IPPROTO_ICMP, -1, -EINVAL},
+		     {4,  600, 1400, 1, 64, IPPROTO_ICMP, -1, -ENOTSUP},
+		     {4,  600, 1400, 0,  0, IPPROTO_ICMP, -1, 3},
+
+		     {6, 1280, 1400, 0, 64, IPPROTO_ICMP, -1, 2},
+		     {6, 1300, 1400, 0, 64, IPPROTO_ICMP, -1, 2},
+		     {6,    4, 1400, 0, 64, IPPROTO_ICMP, -1, -EINVAL},
+		     {6, 1300, 1400, 0,  0, IPPROTO_ICMP, -1, 2},
+		     {6, 1280, 1400, 0, 64, IPPROTO_FRAGMENT, -1, -ENOTSUP},
+	};
+
+	for (size_t i = 0; i < ARRAY_SIZE(tests); i++) {
+		int32_t len;
+		uint16_t pktid = tests[i].pkt_id;
+		struct rte_mbuf *pkts_out[BURST];
+		struct rte_mbuf *b = rte_pktmbuf_alloc(pkt_pool);
+
+		if (!b)
+			return TEST_FAILED; /* Serious error.. abort here */
+
+		if (tests[i].pkt_id == -1)
+			pktid = rte_rand_max(UINT16_MAX);
+
+		if (tests[i].ipv == 4) {
+			if (v4_allocate_packet_of(b, 0x41414141,
+						  tests[i].pkt_size,
+						  tests[i].set_df,
+						  tests[i].ttl,
+						  tests[i].proto,
+						  pktid))
+				result = TEST_FAILED;
+		} else if (tests[i].ipv == 6) {
+			if (v6_allocate_packet_of(b, 0x41414141,
+						  tests[i].pkt_size,
+						  tests[i].ttl,
+						  tests[i].proto,
+						  pktid))
+				result = TEST_FAILED;
+		}
+
+		if (tests[i].ipv == 4)
+			len = rte_ipv4_fragment_packet(b, pkts_out, BURST,
+						       tests[i].mtu_size,
+						       direct_pool,
+						       indirect_pool);
+		else
+			len = rte_ipv6_fragment_packet(b, pkts_out, BURST,
+						       tests[i].mtu_size,
+						       direct_pool,
+						       indirect_pool);
+
+		rte_pktmbuf_free(b);
+
+		if (len > 0)
+			test_free_fragments(pkts_out, len);
+
+		printf("%d: checking %d with %d\n", (int)i, len,
+		       (int)tests[i].expected_frags);
+		RTE_TEST_ASSERT_EQUAL(len, tests[i].expected_frags,
+				      "Failed case %u\n", (unsigned int)i);
+
+	}
+
+	return result;
+}
+
+static struct unit_test_suite ipfrag_testsuite  = {
+	.suite_name = "IP Frag Unit Test Suite",
+	.setup = testsuite_setup,
+	.teardown = testsuite_teardown,
+	.unit_test_cases = {
+		TEST_CASE_ST(ut_setup, ut_teardown,
+			     test_ip_frag),
+
+		TEST_CASES_END() /**< NULL terminate unit test array */
+	}
+};
+
+static int
+test_ipfrag(void)
+{
+	return unit_test_suite_runner(&ipfrag_testsuite);
+}
+
+REGISTER_TEST_COMMAND(ipfrag_autotest, test_ipfrag);
-- 
2.25.1

[dpdk-dev] [PATCH v3 0/4] ip_frag: add a unit test for fragmentation

[Aaron Conole]

This adds a simple unit test for the ip fragmentation library
and covers fragmenting ipv4 and ipv6.  Additionally, some fixes
are introduced, which are really just sanity rather than real
issues in the field.

v2->v3:
- Remove c99-ism from 4/4

v1->v2:
- Fix patch 4/4 which had a missing assignment for pktid.

Aaron Conole (4):
  ip_frag: ensure minimum v4 fragmentation length
  ip_frag: ensure minimum v6 fragmentation length
  ip_frag: ipv6 fragments must not be resubmitted to fragmentation
  ipfrag: add unit test case

 MAINTAINERS                                 |   1 +
 app/test/meson.build                        |   2 +
 app/test/test_ipfrag.c                      | 276 ++++++++++++++++++++
 lib/librte_ip_frag/rte_ipv4_fragmentation.c |   6 +
 lib/librte_ip_frag/rte_ipv6_fragmentation.c |  15 ++
 5 files changed, 300 insertions(+)
 create mode 100644 app/test/test_ipfrag.c

-- 
2.25.1

[dpdk-dev] [PATCH v3 1/4] ip_frag: ensure minimum v4 fragmentation length

[Aaron Conole]

The IPv4 specification says that each fragment must at least the size of
an IP header plus 8 octets.  When attempting to run ipfrag using a
smaller size, the fragment library will return successful completion,
even though it is a violation of RFC791 (and updates).

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 lib/librte_ip_frag/rte_ipv4_fragmentation.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/librte_ip_frag/rte_ipv4_fragmentation.c b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
index 9e9f986cc5..4baaf6355c 100644
--- a/lib/librte_ip_frag/rte_ipv4_fragmentation.c
+++ b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
@@ -76,6 +76,12 @@ rte_ipv4_fragment_packet(struct rte_mbuf *pkt_in,
 	uint16_t fragment_offset, flag_offset, frag_size;
 	uint16_t frag_bytes_remaining;
 
+	/*
+	 * Ensure the IP fragmentation size is at least iphdr length + 8 octets
+	 */
+	if (unlikely(mtu_size < (sizeof(struct rte_ipv4_hdr) + 8*sizeof(char))))
+		return -EINVAL;
+
 	/*
 	 * Ensure the IP payload length of all fragments is aligned to a
 	 * multiple of 8 bytes as per RFC791 section 2.3.
-- 
2.25.1

[dpdk-dev] [PATCH v3 2/4] ip_frag: ensure minimum v6 fragmentation length

[Aaron Conole]

Similar to v4, we should ensure that at least a minimum fragmentation
length can be achieved.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 lib/librte_ip_frag/rte_ipv6_fragmentation.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/lib/librte_ip_frag/rte_ipv6_fragmentation.c b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
index 43449970e5..820a5dc725 100644
--- a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
+++ b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
@@ -79,6 +79,17 @@ rte_ipv6_fragment_packet(struct rte_mbuf *pkt_in,
 	uint16_t fragment_offset, frag_size;
 	uint64_t frag_bytes_remaining;
 
+	/*
+	 * The rules on ipv6 fragmentation means we need at least to be
+	 * greater than the smallest fragment possible - ipv6hdr + frag header
+	 * + min octets.  We rely on the ipv6_extension_fragment header to
+	 * include 'data' octets.
+	 */
+	if (unlikely(mtu_size <
+		     (sizeof(struct rte_ipv6_hdr) +
+		      sizeof(struct ipv6_extension_fragment))))
+		return -EINVAL;
+
 	/*
 	 * Ensure the IP payload length of all fragments (except the
 	 * the last fragment) are a multiple of 8 bytes per RFC2460.
-- 
2.25.1

[dpdk-dev] [PATCH v3 3/4] ip_frag: ipv6 fragments must not be resubmitted to fragmentation

[Aaron Conole]

IPv6 only allows traffic source nodes to fragment, so submitting
a packet with next header of IPPROTO_FRAGMENT would be invalid.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 lib/librte_ip_frag/rte_ipv6_fragmentation.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/librte_ip_frag/rte_ipv6_fragmentation.c b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
index 820a5dc725..aebcfa4325 100644
--- a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
+++ b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
@@ -106,6 +106,10 @@ rte_ipv6_fragment_packet(struct rte_mbuf *pkt_in,
 
 	in_hdr = rte_pktmbuf_mtod(pkt_in, struct rte_ipv6_hdr *);
 
+	/* Fragmenting a fragmented packet?! */
+	if (unlikely(in_hdr->proto == IPPROTO_FRAGMENT))
+		return -ENOTSUP;
+
 	in_seg = pkt_in;
 	in_seg_data_pos = sizeof(struct rte_ipv6_hdr);
 	out_pkt_pos = 0;
-- 
2.25.1

[dpdk-dev] [PATCH v3 4/4] ipfrag: add unit test case

[Aaron Conole]

Initial IP fragmentation unit test.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 MAINTAINERS            |   1 +
 app/test/meson.build   |   2 +
 app/test/test_ipfrag.c | 277 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 280 insertions(+)
 create mode 100644 app/test/test_ipfrag.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 4800f6884a..c81f07ac9c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1230,6 +1230,7 @@ F: app/test/test_crc.c
 IP fragmentation & reassembly
 M: Konstantin Ananyev <konstantin.ananyev@intel.com>
 F: lib/librte_ip_frag/
+F: app/test/test_ipfrag.c
 F: doc/guides/prog_guide/ip_fragment_reassembly_lib.rst
 F: examples/ip_fragmentation/
 F: doc/guides/sample_app_ug/ip_frag.rst
diff --git a/app/test/meson.build b/app/test/meson.build
index 351d29cb65..c2b07fa62f 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -58,6 +58,7 @@ test_sources = files('commands.c',
 	'test_hash_perf.c',
 	'test_hash_readwrite_lf_perf.c',
 	'test_interrupts.c',
+        'test_ipfrag.c',
 	'test_ipsec.c',
 	'test_ipsec_sad.c',
 	'test_kni.c',
@@ -187,6 +188,7 @@ fast_tests = [
         ['flow_classify_autotest', false],
         ['hash_autotest', true],
         ['interrupt_autotest', true],
+        ['ipfrag_autotest', false],
         ['logs_autotest', true],
         ['lpm_autotest', true],
         ['lpm6_autotest', true],
diff --git a/app/test/test_ipfrag.c b/app/test/test_ipfrag.c
new file mode 100644
index 0000000000..1fa7b69e60
--- /dev/null
+++ b/app/test/test_ipfrag.c
@@ -0,0 +1,277 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2020 Red Hat, Inc.
+ */
+
+#include <time.h>
+
+#include <rte_common.h>
+#include <rte_cycles.h>
+#include <rte_hexdump.h>
+#include <rte_ip.h>
+#include <rte_ip_frag.h>
+#include <rte_mbuf.h>
+#include <rte_memcpy.h>
+#include <rte_random.h>
+
+#include "test.h"
+
+#ifndef ARRAY_SIZE
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+#endif
+
+static struct rte_mempool *pkt_pool,
+			  *direct_pool,
+			  *indirect_pool;
+
+static int
+setup_buf_pool(void)
+{
+#define NUM_MBUFS (128)
+#define BURST 32
+
+	if (!pkt_pool)
+		pkt_pool = rte_pktmbuf_pool_create("FRAG_MBUF_POOL",
+						   NUM_MBUFS, BURST, 0,
+						   RTE_MBUF_DEFAULT_BUF_SIZE,
+						   SOCKET_ID_ANY);
+	if (pkt_pool == NULL) {
+		printf("%s: Error creating pkt mempool\n", __func__);
+		goto bad_setup;
+	}
+
+	if (!direct_pool)
+		direct_pool = rte_pktmbuf_pool_create("FRAG_D_MBUF_POOL",
+						      NUM_MBUFS, BURST, 0,
+						      RTE_MBUF_DEFAULT_BUF_SIZE,
+						      SOCKET_ID_ANY);
+	if (!direct_pool) {
+		printf("%s: Error creating direct mempool\n", __func__);
+		goto bad_setup;
+	}
+
+	if (!indirect_pool)
+		indirect_pool = rte_pktmbuf_pool_create("FRAG_I_MBUF_POOL",
+							NUM_MBUFS, BURST, 0,
+							0, SOCKET_ID_ANY);
+	if (!indirect_pool) {
+		printf("%s: Error creating indirect mempool\n", __func__);
+		goto bad_setup;
+	}
+
+	return 0;
+
+bad_setup:
+	if (pkt_pool)
+		rte_mempool_free(pkt_pool);
+
+	if (direct_pool)
+		rte_mempool_free(direct_pool);
+
+	return TEST_FAILED;
+}
+
+static int testsuite_setup(void)
+{
+	if (setup_buf_pool())
+		return TEST_FAILED;
+	return TEST_SUCCESS;
+}
+
+static void testsuite_teardown(void)
+{
+	if (pkt_pool)
+		rte_mempool_free(pkt_pool);
+
+	if (direct_pool)
+		rte_mempool_free(direct_pool);
+
+	if (indirect_pool)
+		rte_mempool_free(indirect_pool);
+
+	pkt_pool = NULL;
+}
+
+static int ut_setup(void)
+{
+	return TEST_SUCCESS;
+}
+
+static void ut_teardown(void)
+{
+}
+
+static int
+v4_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s, int df,
+		      uint8_t ttl, uint8_t proto, uint16_t pktid)
+{
+	/* Create a packet, 2k bytes long */
+	b->data_off = 0;
+	char *data = rte_pktmbuf_mtod(b, char *);
+
+	memset(data, fill, sizeof(struct rte_ipv4_hdr) + s);
+
+	struct rte_ipv4_hdr *hdr = (struct rte_ipv4_hdr *)data;
+
+	hdr->version_ihl = 0x45; /* standard IP header... */
+	hdr->type_of_service = 0;
+	b->pkt_len = s + sizeof(struct rte_ipv4_hdr);
+	b->data_len = b->pkt_len;
+	hdr->total_length = htonl(b->pkt_len);
+	hdr->packet_id = htons(pktid);
+	hdr->fragment_offset = 0;
+	if (df)
+		hdr->fragment_offset = htons(0x4000);
+
+	if (!ttl)
+		ttl = 64; /* default to 64 */
+
+	if (!proto)
+		proto = 1; /* icmp */
+
+	hdr->time_to_live = ttl;
+	hdr->next_proto_id = proto;
+	hdr->hdr_checksum = 0;
+	hdr->src_addr = htonl(0x8080808);
+	hdr->dst_addr = htonl(0x8080404);
+
+	return 0;
+}
+
+static int
+v6_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s, uint8_t ttl,
+		      uint8_t proto, uint16_t pktid)
+{
+	/* Create a packet, 2k bytes long */
+	b->data_off = 0;
+	char *data = rte_pktmbuf_mtod(b, char *);
+
+	memset(data, fill, sizeof(struct rte_ipv6_hdr) + s);
+
+	struct rte_ipv6_hdr *hdr = (struct rte_ipv6_hdr *)data;
+	b->pkt_len = s + sizeof(struct rte_ipv6_hdr);
+	b->data_len = b->pkt_len;
+
+	/* basic v6 header */
+	hdr->vtc_flow = htonl(0x60 << 24 | pktid);
+	hdr->payload_len = htons(b->pkt_len);
+	hdr->proto = proto;
+	hdr->hop_limits = ttl;
+
+	memset(hdr->src_addr, 0x08, sizeof(hdr->src_addr));
+	memset(hdr->dst_addr, 0x04, sizeof(hdr->src_addr));
+
+	return 0;
+}
+
+static inline void
+test_free_fragments(struct rte_mbuf *mb[], uint32_t num)
+{
+	uint32_t i;
+	for (i = 0; i < num; i++)
+		rte_pktmbuf_free(mb[i]);
+}
+
+static int
+test_ip_frag(void)
+{
+	int result = TEST_SUCCESS;
+	size_t i;
+
+	struct test_ip_frags {
+		int     ipv;
+		size_t  mtu_size;
+		size_t  pkt_size;
+		int     set_df;
+		int     ttl;
+		uint8_t proto;
+		int     pkt_id;
+		int     expected_frags;
+	} tests[] = {
+		     {4, 1280, 1400, 0, 64, IPPROTO_ICMP, -1, 2},
+		     {4, 1280, 1400, 0, 64, IPPROTO_ICMP,  0, 2},
+		     {4,  600, 1400, 0, 64, IPPROTO_ICMP, -1, 3},
+		     {4,    4, 1400, 0, 64, IPPROTO_ICMP, -1, -EINVAL},
+		     {4,  600, 1400, 1, 64, IPPROTO_ICMP, -1, -ENOTSUP},
+		     {4,  600, 1400, 0,  0, IPPROTO_ICMP, -1, 3},
+
+		     {6, 1280, 1400, 0, 64, IPPROTO_ICMP, -1, 2},
+		     {6, 1300, 1400, 0, 64, IPPROTO_ICMP, -1, 2},
+		     {6,    4, 1400, 0, 64, IPPROTO_ICMP, -1, -EINVAL},
+		     {6, 1300, 1400, 0,  0, IPPROTO_ICMP, -1, 2},
+		     {6, 1280, 1400, 0, 64, IPPROTO_FRAGMENT, -1, -ENOTSUP},
+	};
+
+	for (i = 0; i < ARRAY_SIZE(tests); i++) {
+		int32_t len;
+		uint16_t pktid = tests[i].pkt_id;
+		struct rte_mbuf *pkts_out[BURST];
+		struct rte_mbuf *b = rte_pktmbuf_alloc(pkt_pool);
+
+		if (!b)
+			return TEST_FAILED; /* Serious error.. abort here */
+
+		if (tests[i].pkt_id == -1)
+			pktid = rte_rand_max(UINT16_MAX);
+
+		if (tests[i].ipv == 4) {
+			if (v4_allocate_packet_of(b, 0x41414141,
+						  tests[i].pkt_size,
+						  tests[i].set_df,
+						  tests[i].ttl,
+						  tests[i].proto,
+						  pktid))
+				result = TEST_FAILED;
+		} else if (tests[i].ipv == 6) {
+			if (v6_allocate_packet_of(b, 0x41414141,
+						  tests[i].pkt_size,
+						  tests[i].ttl,
+						  tests[i].proto,
+						  pktid))
+				result = TEST_FAILED;
+		}
+
+		if (tests[i].ipv == 4)
+			len = rte_ipv4_fragment_packet(b, pkts_out, BURST,
+						       tests[i].mtu_size,
+						       direct_pool,
+						       indirect_pool);
+		else
+			len = rte_ipv6_fragment_packet(b, pkts_out, BURST,
+						       tests[i].mtu_size,
+						       direct_pool,
+						       indirect_pool);
+
+		rte_pktmbuf_free(b);
+
+		if (len > 0)
+			test_free_fragments(pkts_out, len);
+
+		printf("%d: checking %d with %d\n", (int)i, len,
+		       (int)tests[i].expected_frags);
+		RTE_TEST_ASSERT_EQUAL(len, tests[i].expected_frags,
+				      "Failed case %u\n", (unsigned int)i);
+
+	}
+
+	return result;
+}
+
+static struct unit_test_suite ipfrag_testsuite  = {
+	.suite_name = "IP Frag Unit Test Suite",
+	.setup = testsuite_setup,
+	.teardown = testsuite_teardown,
+	.unit_test_cases = {
+		TEST_CASE_ST(ut_setup, ut_teardown,
+			     test_ip_frag),
+
+		TEST_CASES_END() /**< NULL terminate unit test array */
+	}
+};
+
+static int
+test_ipfrag(void)
+{
+	return unit_test_suite_runner(&ipfrag_testsuite);
+}
+
+REGISTER_TEST_COMMAND(ipfrag_autotest, test_ipfrag);
-- 
2.25.1

Re: [dpdk-dev] [PATCH v3 4/4] ipfrag: add unit test case

[Pavan Nikhilesh Bhagavatula]

>Subject: [dpdk-dev] [PATCH v3 4/4] ipfrag: add unit test case
>
>Initial IP fragmentation unit test.
>
>Signed-off-by: Aaron Conole <aconole@redhat.com>
>---
> MAINTAINERS            |   1 +
> app/test/meson.build   |   2 +
> app/test/test_ipfrag.c | 277
>+++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 280 insertions(+)
> create mode 100644 app/test/test_ipfrag.c
>

<snip>

>+static int
>+v4_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s, int df,
>+		      uint8_t ttl, uint8_t proto, uint16_t pktid)
>+{
>+	/* Create a packet, 2k bytes long */
>+	b->data_off = 0;
>+	char *data = rte_pktmbuf_mtod(b, char *);
>+
>+	memset(data, fill, sizeof(struct rte_ipv4_hdr) + s);
>+
>+	struct rte_ipv4_hdr *hdr = (struct rte_ipv4_hdr *)data;
>+
>+	hdr->version_ihl = 0x45; /* standard IP header... */
>+	hdr->type_of_service = 0;
>+	b->pkt_len = s + sizeof(struct rte_ipv4_hdr);
>+	b->data_len = b->pkt_len;
>+	hdr->total_length = htonl(b->pkt_len);	

Minor nit, please use rte_cpu_to_be_xx() for byte order conversion across the file.

>+	hdr->packet_id = htons(pktid);
>+	hdr->fragment_offset = 0;
>+	if (df)
>+		hdr->fragment_offset = htons(0x4000);
>+
>+	if (!ttl)
>+		ttl = 64; /* default to 64 */
>+
>+	if (!proto)
>+		proto = 1; /* icmp */
>+
>+	hdr->time_to_live = ttl;
>+	hdr->next_proto_id = proto;
>+	hdr->hdr_checksum = 0;
>+	hdr->src_addr = htonl(0x8080808);
>+	hdr->dst_addr = htonl(0x8080404);
>+
>+	return 0;
>+}
>+

Re: [dpdk-dev] [PATCH v3 3/4] ip_frag: ipv6 fragments must not be resubmitted to fragmentation

[Ananyev, Konstantin]

> IPv6 only allows traffic source nodes to fragment,

Yes.

> so submitting
> a packet with next header of IPPROTO_FRAGMENT would be invalid.

If only source is allowed to fragment packet, then this check seems redundant, no?
I can't imagine source calling fragment() twice for the same packet, and
I don't see any point for us to check such situations.
Besides, strictly speaking the check below is insufficient,
as fragmentation ext header could be not the first one. 
Konstantin

> 
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>  lib/librte_ip_frag/rte_ipv6_fragmentation.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/lib/librte_ip_frag/rte_ipv6_fragmentation.c b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> index 820a5dc725..aebcfa4325 100644
> --- a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> +++ b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> @@ -106,6 +106,10 @@ rte_ipv6_fragment_packet(struct rte_mbuf *pkt_in,
> 
>  	in_hdr = rte_pktmbuf_mtod(pkt_in, struct rte_ipv6_hdr *);
> 
> +	/* Fragmenting a fragmented packet?! */
> +	if (unlikely(in_hdr->proto == IPPROTO_FRAGMENT))
> +		return -ENOTSUP;
> +
>  	in_seg = pkt_in;
>  	in_seg_data_pos = sizeof(struct rte_ipv6_hdr);
>  	out_pkt_pos = 0;
> --
> 2.25.1

Re: [dpdk-dev] [PATCH v3 2/4] ip_frag: ensure minimum v6 fragmentation length

[Ananyev, Konstantin]

> 
> Similar to v4, we should ensure that at least a minimum fragmentation
> length can be achieved.
> 
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>  lib/librte_ip_frag/rte_ipv6_fragmentation.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/lib/librte_ip_frag/rte_ipv6_fragmentation.c b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> index 43449970e5..820a5dc725 100644
> --- a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> +++ b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> @@ -79,6 +79,17 @@ rte_ipv6_fragment_packet(struct rte_mbuf *pkt_in,
>  	uint16_t fragment_offset, frag_size;
>  	uint64_t frag_bytes_remaining;
> 
> +	/*
> +	 * The rules on ipv6 fragmentation means we need at least to be
> +	 * greater than the smallest fragment possible - ipv6hdr + frag header
> +	 * + min octets.  We rely on the ipv6_extension_fragment header to
> +	 * include 'data' octets.
> +	 */
> +	if (unlikely(mtu_size <
> +		     (sizeof(struct rte_ipv6_hdr) +
> +		      sizeof(struct ipv6_extension_fragment))))
> +		return -EINVAL;
> +		

Don't know where it comes from.
Reading RFC 8200, it clearly states that min MTU for ipv6 is 1280B.
In fact, I don't see much point in these extra checking.

>  	/*
>  	 * Ensure the IP payload length of all fragments (except the
>  	 * the last fragment) are a multiple of 8 bytes per RFC2460.
> --
> 2.25.1

Re: [dpdk-dev] [PATCH v3 1/4] ip_frag: ensure minimum v4 fragmentation length

[Ananyev, Konstantin]

> 
> The IPv4 specification says that each fragment must at least the size of
> an IP header plus 8 octets.  When attempting to run ipfrag using a
> smaller size, the fragment library will return successful completion,
> even though it is a violation of RFC791 (and updates).
> 
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>  lib/librte_ip_frag/rte_ipv4_fragmentation.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/lib/librte_ip_frag/rte_ipv4_fragmentation.c b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
> index 9e9f986cc5..4baaf6355c 100644
> --- a/lib/librte_ip_frag/rte_ipv4_fragmentation.c
> +++ b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
> @@ -76,6 +76,12 @@ rte_ipv4_fragment_packet(struct rte_mbuf *pkt_in,
>  	uint16_t fragment_offset, flag_offset, frag_size;
>  	uint16_t frag_bytes_remaining;
> 
> +	/*
> +	 * Ensure the IP fragmentation size is at least iphdr length + 8 octets
> +	 */
> +	if (unlikely(mtu_size < (sizeof(struct rte_ipv4_hdr) + 8*sizeof(char))))
> +		return -EINVAL;
> +

Same comment as for ipv6: ipv4 min MTU is 68B.
Why do we need extra checking here?

>  	/*
>  	 * Ensure the IP payload length of all fragments is aligned to a
>  	 * multiple of 8 bytes as per RFC791 section 2.3.
> --
> 2.25.1

Re: [dpdk-dev] [PATCH v3 3/4] ip_frag: ipv6 fragments must not be resubmitted to fragmentation

[Aaron Conole]

"Ananyev, Konstantin" <konstantin.ananyev@intel.com> writes:

>> IPv6 only allows traffic source nodes to fragment,
>
> Yes.
>
>> so submitting
>> a packet with next header of IPPROTO_FRAGMENT would be invalid.
>
> If only source is allowed to fragment packet, then this check seems
> redundant, no?

Hrrm?  How so?  Is there something that prevents someone from calling
the library function twice?

> I can't imagine source calling fragment() twice for the same packet, and
> I don't see any point for us to check such situations.

Should we not check any error conditions at all?  I don't understand.

> Besides, strictly speaking the check below is insufficient,
> as fragmentation ext header could be not the first one.

You're right - we could probably walk next headers until we see one of
the auth header types or upper layer header as "next header".  I can
respin with that if you'd like.

> Konstantin
>
>> 
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> ---
>>  lib/librte_ip_frag/rte_ipv6_fragmentation.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/lib/librte_ip_frag/rte_ipv6_fragmentation.c b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
>> index 820a5dc725..aebcfa4325 100644
>> --- a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
>> +++ b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
>> @@ -106,6 +106,10 @@ rte_ipv6_fragment_packet(struct rte_mbuf *pkt_in,
>> 
>>  	in_hdr = rte_pktmbuf_mtod(pkt_in, struct rte_ipv6_hdr *);
>> 
>> +	/* Fragmenting a fragmented packet?! */
>> +	if (unlikely(in_hdr->proto == IPPROTO_FRAGMENT))
>> +		return -ENOTSUP;
>> +
>>  	in_seg = pkt_in;
>>  	in_seg_data_pos = sizeof(struct rte_ipv6_hdr);
>>  	out_pkt_pos = 0;
>> --
>> 2.25.1

Re: [dpdk-dev] [PATCH v3 1/4] ip_frag: ensure minimum v4 fragmentation length

[Aaron Conole]

"Ananyev, Konstantin" <konstantin.ananyev@intel.com> writes:

>> 
>> The IPv4 specification says that each fragment must at least the size of
>> an IP header plus 8 octets.  When attempting to run ipfrag using a
>> smaller size, the fragment library will return successful completion,
>> even though it is a violation of RFC791 (and updates).
>> 
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> ---
>>  lib/librte_ip_frag/rte_ipv4_fragmentation.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/lib/librte_ip_frag/rte_ipv4_fragmentation.c b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
>> index 9e9f986cc5..4baaf6355c 100644
>> --- a/lib/librte_ip_frag/rte_ipv4_fragmentation.c
>> +++ b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
>> @@ -76,6 +76,12 @@ rte_ipv4_fragment_packet(struct rte_mbuf *pkt_in,
>>  	uint16_t fragment_offset, flag_offset, frag_size;
>>  	uint16_t frag_bytes_remaining;
>> 
>> +	/*
>> +	 * Ensure the IP fragmentation size is at least iphdr length + 8 octets
>> +	 */
>> +	if (unlikely(mtu_size < (sizeof(struct rte_ipv4_hdr) + 8*sizeof(char))))
>> +		return -EINVAL;
>> +
>
> Same comment as for ipv6: ipv4 min MTU is 68B.

I can change it.  I suspected that if I went with 68 here and 1280 in
the v6 code, I would get pushback, but I should have just submitted it
that way to begin.

> Why do we need extra checking here?

These are error conditions to submit to fragmentation module.  Someone
needs to do the check - either it is done in the application or the
library.  If the library doesn't, and the application writer doesn't
know they must write these checks (it isn't documented anywhere), then
we get non compliant behavior.  By putting it in the library, we can
clearly signal the application writer such a case has occurred.

Should we not do error checking?

>>  	/*
>>  	 * Ensure the IP payload length of all fragments is aligned to a
>>  	 * multiple of 8 bytes as per RFC791 section 2.3.
>> --
>> 2.25.1

Re: [dpdk-dev] [PATCH v3 1/4] ip_frag: ensure minimum v4 fragmentation length

[Ananyev, Konstantin]

> 
> "Ananyev, Konstantin" <konstantin.ananyev@intel.com> writes:
> 
> >>
> >> The IPv4 specification says that each fragment must at least the size of
> >> an IP header plus 8 octets.  When attempting to run ipfrag using a
> >> smaller size, the fragment library will return successful completion,
> >> even though it is a violation of RFC791 (and updates).
> >>
> >> Signed-off-by: Aaron Conole <aconole@redhat.com>
> >> ---
> >>  lib/librte_ip_frag/rte_ipv4_fragmentation.c | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/lib/librte_ip_frag/rte_ipv4_fragmentation.c b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
> >> index 9e9f986cc5..4baaf6355c 100644
> >> --- a/lib/librte_ip_frag/rte_ipv4_fragmentation.c
> >> +++ b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
> >> @@ -76,6 +76,12 @@ rte_ipv4_fragment_packet(struct rte_mbuf *pkt_in,
> >>  	uint16_t fragment_offset, flag_offset, frag_size;
> >>  	uint16_t frag_bytes_remaining;
> >>
> >> +	/*
> >> +	 * Ensure the IP fragmentation size is at least iphdr length + 8 octets
> >> +	 */
> >> +	if (unlikely(mtu_size < (sizeof(struct rte_ipv4_hdr) + 8*sizeof(char))))
> >> +		return -EINVAL;
> >> +
> >
> > Same comment as for ipv6: ipv4 min MTU is 68B.
> 
> I can change it.  I suspected that if I went with 68 here and 1280 in
> the v6 code, I would get pushback, but I should have just submitted it
> that way to begin.
> 
> > Why do we need extra checking here?
> 
> These are error conditions to submit to fragmentation module.  Someone
> needs to do the check - either it is done in the application or the
> library.  If the library doesn't, and the application writer doesn't
> know they must write these checks (it isn't documented anywhere), then
> we get non compliant behavior.  By putting it in the library, we can
> clearly signal the application writer such a case has occurred.
> 
> Should we not do error checking?

It depends I think...
In many data-path functions we skip parameter checking.
These fragment() functions are data-path too.
Agree, it is not stated clearly in these functions formal comments,
as it should be.
After another thought - these functions are quite heavy-weighed anyway,
so probably formal parameter checking, something like:
if (pkt_in == NULL || pkts_out == NULL || pool_direct == NULL ||
		pool_indirect == NULL || mtu < MIN_MTU)
	return -EINVAL;

wouldn't introduce any real slowdown.
About more intense checking - like parsing all extension
headers, etc. - I think it would be too much overhead.
Again for that there is a special function that user can call directly:
rte_ipv6_frag_get_ipv6_fragment_header
(though its current implementation also checks only first extension header).
So, I think we just need to document that it is a user responsibility to
provide not fragmented yet packet, without any pre-fragment headers.
 Konstantin

> 
> >>  	/*
> >>  	 * Ensure the IP payload length of all fragments is aligned to a
> >>  	 * multiple of 8 bytes as per RFC791 section 2.3.
> >> --
> >> 2.25.1

Re: [dpdk-dev] [PATCH v3 1/4] ip_frag: ensure minimum v4 fragmentation length

[Aaron Conole]

"Ananyev, Konstantin" <konstantin.ananyev@intel.com> writes:

>> 
>> "Ananyev, Konstantin" <konstantin.ananyev@intel.com> writes:
>> 
>> >>
>> >> The IPv4 specification says that each fragment must at least the size of
>> >> an IP header plus 8 octets.  When attempting to run ipfrag using a
>> >> smaller size, the fragment library will return successful completion,
>> >> even though it is a violation of RFC791 (and updates).
>> >>
>> >> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> >> ---
>> >>  lib/librte_ip_frag/rte_ipv4_fragmentation.c | 6 ++++++
>> >>  1 file changed, 6 insertions(+)
>> >>
>> >> diff --git a/lib/librte_ip_frag/rte_ipv4_fragmentation.c b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
>> >> index 9e9f986cc5..4baaf6355c 100644
>> >> --- a/lib/librte_ip_frag/rte_ipv4_fragmentation.c
>> >> +++ b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
>> >> @@ -76,6 +76,12 @@ rte_ipv4_fragment_packet(struct rte_mbuf *pkt_in,
>> >>  	uint16_t fragment_offset, flag_offset, frag_size;
>> >>  	uint16_t frag_bytes_remaining;
>> >>
>> >> +	/*
>> >> +	 * Ensure the IP fragmentation size is at least iphdr length + 8 octets
>> >> +	 */
>> >> +	if (unlikely(mtu_size < (sizeof(struct rte_ipv4_hdr) + 8*sizeof(char))))
>> >> +		return -EINVAL;
>> >> +
>> >
>> > Same comment as for ipv6: ipv4 min MTU is 68B.
>> 
>> I can change it.  I suspected that if I went with 68 here and 1280 in
>> the v6 code, I would get pushback, but I should have just submitted it
>> that way to begin.
>> 
>> > Why do we need extra checking here?
>> 
>> These are error conditions to submit to fragmentation module.  Someone
>> needs to do the check - either it is done in the application or the
>> library.  If the library doesn't, and the application writer doesn't
>> know they must write these checks (it isn't documented anywhere), then
>> we get non compliant behavior.  By putting it in the library, we can
>> clearly signal the application writer such a case has occurred.
>> 
>> Should we not do error checking?
>
> It depends I think...
> In many data-path functions we skip parameter checking.
> These fragment() functions are data-path too.
> Agree, it is not stated clearly in these functions formal comments,
> as it should be.

I'll add documentation as another patch.

> After another thought - these functions are quite heavy-weighed anyway,
> so probably formal parameter checking, something like:
> if (pkt_in == NULL || pkts_out == NULL || pool_direct == NULL ||
> 		pool_indirect == NULL || mtu < MIN_MTU)
> 	return -EINVAL;
>
> wouldn't introduce any real slowdown.

Agreed.

> About more intense checking - like parsing all extension
> headers, etc. - I think it would be too much overhead.
> Again for that there is a special function that user can call directly:
> rte_ipv6_frag_get_ipv6_fragment_header
> (though its current implementation also checks only first extension header).
> So, I think we just need to document that it is a user responsibility to
> provide not fragmented yet packet, without any pre-fragment headers.

Makes sense.  Then again, the v6 frag code will need to preserve many of
the headers anyway, so since we have to read them, maybe it makes
sense to do the check here anyway.  WDYT?

>  Konstantin
>
>> 
>> >>  	/*
>> >>  	 * Ensure the IP payload length of all fragments is aligned to a
>> >>  	 * multiple of 8 bytes as per RFC791 section 2.3.
>> >> --
>> >> 2.25.1

Re: [dpdk-dev] [PATCH v3 1/4] ip_frag: ensure minimum v4 fragmentation length

[Ananyev, Konstantin]

> >> >> The IPv4 specification says that each fragment must at least the size of
> >> >> an IP header plus 8 octets.  When attempting to run ipfrag using a
> >> >> smaller size, the fragment library will return successful completion,
> >> >> even though it is a violation of RFC791 (and updates).
> >> >>
> >> >> Signed-off-by: Aaron Conole <aconole@redhat.com>
> >> >> ---
> >> >>  lib/librte_ip_frag/rte_ipv4_fragmentation.c | 6 ++++++
> >> >>  1 file changed, 6 insertions(+)
> >> >>
> >> >> diff --git a/lib/librte_ip_frag/rte_ipv4_fragmentation.c b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
> >> >> index 9e9f986cc5..4baaf6355c 100644
> >> >> --- a/lib/librte_ip_frag/rte_ipv4_fragmentation.c
> >> >> +++ b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
> >> >> @@ -76,6 +76,12 @@ rte_ipv4_fragment_packet(struct rte_mbuf *pkt_in,
> >> >>  	uint16_t fragment_offset, flag_offset, frag_size;
> >> >>  	uint16_t frag_bytes_remaining;
> >> >>
> >> >> +	/*
> >> >> +	 * Ensure the IP fragmentation size is at least iphdr length + 8 octets
> >> >> +	 */
> >> >> +	if (unlikely(mtu_size < (sizeof(struct rte_ipv4_hdr) + 8*sizeof(char))))
> >> >> +		return -EINVAL;
> >> >> +
> >> >
> >> > Same comment as for ipv6: ipv4 min MTU is 68B.
> >>
> >> I can change it.  I suspected that if I went with 68 here and 1280 in
> >> the v6 code, I would get pushback, but I should have just submitted it
> >> that way to begin.
> >>
> >> > Why do we need extra checking here?
> >>
> >> These are error conditions to submit to fragmentation module.  Someone
> >> needs to do the check - either it is done in the application or the
> >> library.  If the library doesn't, and the application writer doesn't
> >> know they must write these checks (it isn't documented anywhere), then
> >> we get non compliant behavior.  By putting it in the library, we can
> >> clearly signal the application writer such a case has occurred.
> >>
> >> Should we not do error checking?
> >
> > It depends I think...
> > In many data-path functions we skip parameter checking.
> > These fragment() functions are data-path too.
> > Agree, it is not stated clearly in these functions formal comments,
> > as it should be.
> 
> I'll add documentation as another patch.
> 
> > After another thought - these functions are quite heavy-weighed anyway,
> > so probably formal parameter checking, something like:
> > if (pkt_in == NULL || pkts_out == NULL || pool_direct == NULL ||
> > 		pool_indirect == NULL || mtu < MIN_MTU)
> > 	return -EINVAL;
> >
> > wouldn't introduce any real slowdown.
> 
> Agreed.
> 
> > About more intense checking - like parsing all extension
> > headers, etc. - I think it would be too much overhead.
> > Again for that there is a special function that user can call directly:
> > rte_ipv6_frag_get_ipv6_fragment_header
> > (though its current implementation also checks only first extension header).
> > So, I think we just need to document that it is a user responsibility to
> > provide not fragmented yet packet, without any pre-fragment headers.
> 
> Makes sense.  Then again, the v6 frag code will need to preserve many of
> the headers anyway, so since we have to read them, maybe it makes
> sense to do the check here anyway.  WDYT?

If we want to make this function fully compliant to what rfc8200 says,
then yes - extra changes is required in current implementation:
1. somehow obtain information about pre-fragment extensions length
2. use info from #1 to put fragment header at proper location.
And extra testing of course.
Probably safer and easier, for that patch just add formal parameter checking.
And if you feel like that - have the hard part as a separate patch.

> 
> >  Konstantin
> >
> >>
> >> >>  	/*
> >> >>  	 * Ensure the IP payload length of all fragments is aligned to a
> >> >>  	 * multiple of 8 bytes as per RFC791 section 2.3.
> >> >> --
> >> >> 2.25.1

Re: [dpdk-dev] [PATCH v3 1/4] ip_frag: ensure minimum v4 fragmentation length

[Aaron Conole]

"Ananyev, Konstantin" <konstantin.ananyev@intel.com> writes:

>> >> >> The IPv4 specification says that each fragment must at least the size of
>> >> >> an IP header plus 8 octets.  When attempting to run ipfrag using a
>> >> >> smaller size, the fragment library will return successful completion,
>> >> >> even though it is a violation of RFC791 (and updates).
>> >> >>
>> >> >> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> >> >> ---
>> >> >>  lib/librte_ip_frag/rte_ipv4_fragmentation.c | 6 ++++++
>> >> >>  1 file changed, 6 insertions(+)
>> >> >>
>> >> >> diff --git a/lib/librte_ip_frag/rte_ipv4_fragmentation.c b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
>> >> >> index 9e9f986cc5..4baaf6355c 100644
>> >> >> --- a/lib/librte_ip_frag/rte_ipv4_fragmentation.c
>> >> >> +++ b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
>> >> >> @@ -76,6 +76,12 @@ rte_ipv4_fragment_packet(struct rte_mbuf *pkt_in,
>> >> >>  	uint16_t fragment_offset, flag_offset, frag_size;
>> >> >>  	uint16_t frag_bytes_remaining;
>> >> >>
>> >> >> +	/*
>> >> >> +	 * Ensure the IP fragmentation size is at least iphdr length + 8 octets
>> >> >> +	 */
>> >> >> +	if (unlikely(mtu_size < (sizeof(struct rte_ipv4_hdr) + 8*sizeof(char))))
>> >> >> +		return -EINVAL;
>> >> >> +
>> >> >
>> >> > Same comment as for ipv6: ipv4 min MTU is 68B.
>> >>
>> >> I can change it.  I suspected that if I went with 68 here and 1280 in
>> >> the v6 code, I would get pushback, but I should have just submitted it
>> >> that way to begin.
>> >>
>> >> > Why do we need extra checking here?
>> >>
>> >> These are error conditions to submit to fragmentation module.  Someone
>> >> needs to do the check - either it is done in the application or the
>> >> library.  If the library doesn't, and the application writer doesn't
>> >> know they must write these checks (it isn't documented anywhere), then
>> >> we get non compliant behavior.  By putting it in the library, we can
>> >> clearly signal the application writer such a case has occurred.
>> >>
>> >> Should we not do error checking?
>> >
>> > It depends I think...
>> > In many data-path functions we skip parameter checking.
>> > These fragment() functions are data-path too.
>> > Agree, it is not stated clearly in these functions formal comments,
>> > as it should be.
>> 
>> I'll add documentation as another patch.
>> 
>> > After another thought - these functions are quite heavy-weighed anyway,
>> > so probably formal parameter checking, something like:
>> > if (pkt_in == NULL || pkts_out == NULL || pool_direct == NULL ||
>> > 		pool_indirect == NULL || mtu < MIN_MTU)
>> > 	return -EINVAL;
>> >
>> > wouldn't introduce any real slowdown.
>> 
>> Agreed.
>> 
>> > About more intense checking - like parsing all extension
>> > headers, etc. - I think it would be too much overhead.
>> > Again for that there is a special function that user can call directly:
>> > rte_ipv6_frag_get_ipv6_fragment_header
>> > (though its current implementation also checks only first extension header).
>> > So, I think we just need to document that it is a user responsibility to
>> > provide not fragmented yet packet, without any pre-fragment headers.
>> 
>> Makes sense.  Then again, the v6 frag code will need to preserve many of
>> the headers anyway, so since we have to read them, maybe it makes
>> sense to do the check here anyway.  WDYT?
>
> If we want to make this function fully compliant to what rfc8200 says,
> then yes - extra changes is required in current implementation:
> 1. somehow obtain information about pre-fragment extensions length
> 2. use info from #1 to put fragment header at proper location.
> And extra testing of course.

I think we should.  I know there are projects relying on it.

> Probably safer and easier, for that patch just add formal parameter checking.
> And if you feel like that - have the hard part as a separate patch.

Okay, I'll resubmit the series with minimal ipv6 unit tests, and then
submit another series which brings the frag header behavior in line.

>> 
>> >  Konstantin
>> >
>> >>
>> >> >>  	/*
>> >> >>  	 * Ensure the IP payload length of all fragments is aligned to a
>> >> >>  	 * multiple of 8 bytes as per RFC791 section 2.3.
>> >> >> --
>> >> >> 2.25.1

[dpdk-dev] [PATCH v4 0/3] ip_frag: add a unit test for fragmentation

[Aaron Conole]

This adds a simple unit test for the ip fragmentation library
and covers fragmenting ipv4 and ipv6.  Additionally, some fixes
are introduced, which are really just sanity rather than real
issues in the field.

v3->v4:
- Drop the frag header detection from ipv6 code
- Full parameter sanity checking in ipv4 and ipv6 code
- Convert from htons/htonl to rte_cpu_to_be_...

v2->v3:
- Remove c99-ism from 4/4

v1->v2:
- Fix patch 4/4 which had a missing assignment for pktid.

Aaron Conole (3):
  ip_frag: ensure minimum v4 fragmentation length
  ip_frag: ensure minimum v6 fragmentation length
  ipfrag: add unit test case

 MAINTAINERS                                 |   1 +
 app/test/meson.build                        |   2 +
 app/test/test_ipfrag.c                      | 276 ++++++++++++++++++++
 lib/librte_ip_frag/rte_ipv4_fragmentation.c |   9 +
 lib/librte_ip_frag/rte_ipv6_fragmentation.c |   9 +
 5 files changed, 297 insertions(+)
 create mode 100644 app/test/test_ipfrag.c

-- 
2.25.1

[dpdk-dev] [PATCH v4 1/3] ip_frag: ensure minimum v4 fragmentation length

[Aaron Conole]

Do a formal parameter check of mtu length, as well as
checking the the various inputs for validity.  If any
aren't acceptable, we bail.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 lib/librte_ip_frag/rte_ipv4_fragmentation.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lib/librte_ip_frag/rte_ipv4_fragmentation.c b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
index 9e9f986cc5..c36678a6d2 100644
--- a/lib/librte_ip_frag/rte_ipv4_fragmentation.c
+++ b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
@@ -76,6 +76,15 @@ rte_ipv4_fragment_packet(struct rte_mbuf *pkt_in,
 	uint16_t fragment_offset, flag_offset, frag_size;
 	uint16_t frag_bytes_remaining;
 
+	/*
+	 * Formal parameter checking.
+	 */
+	if (unlikely(pkt_in == NULL) || unlikely(pkts_out == NULL) ||
+	    unlikely(nb_pkts_out == 0) ||
+	    unlikely(pool_direct == NULL) || unlikely(pool_indirect == NULL) ||
+	    unlikely(mtu_size < 68))
+		return -EINVAL;
+
 	/*
 	 * Ensure the IP payload length of all fragments is aligned to a
 	 * multiple of 8 bytes as per RFC791 section 2.3.
-- 
2.25.1

[dpdk-dev] [PATCH v4 2/3] ip_frag: ensure minimum v6 fragmentation length

[Aaron Conole]

In addition, do a formal parameter check.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 lib/librte_ip_frag/rte_ipv6_fragmentation.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lib/librte_ip_frag/rte_ipv6_fragmentation.c b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
index 43449970e5..ee984aed82 100644
--- a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
+++ b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
@@ -79,6 +79,15 @@ rte_ipv6_fragment_packet(struct rte_mbuf *pkt_in,
 	uint16_t fragment_offset, frag_size;
 	uint64_t frag_bytes_remaining;
 
+	/*
+	 * Formal parameter checking.
+	 */
+	if (unlikely(pkt_in == NULL) || unlikely(pkts_out == NULL) ||
+	    unlikely(nb_pkts_out == 0) ||
+	    unlikely(pool_direct == NULL) || unlikely(pool_indirect == NULL) ||
+	    unlikely(mtu_size < 1280))
+		return -EINVAL;
+
 	/*
 	 * Ensure the IP payload length of all fragments (except the
 	 * the last fragment) are a multiple of 8 bytes per RFC2460.
-- 
2.25.1

[dpdk-dev] [PATCH v4 3/3] ipfrag: add unit test case

[Aaron Conole]

Initial IP fragmentation unit test.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 MAINTAINERS            |   1 +
 app/test/meson.build   |   2 +
 app/test/test_ipfrag.c | 276 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 279 insertions(+)
 create mode 100644 app/test/test_ipfrag.c

diff --git a/MAINTAINERS b/MAINTAINERS
index fe59f0224f..a77c7c17ce 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1228,6 +1228,7 @@ F: app/test/test_crc.c
 IP fragmentation & reassembly
 M: Konstantin Ananyev <konstantin.ananyev@intel.com>
 F: lib/librte_ip_frag/
+F: app/test/test_ipfrag.c
 F: doc/guides/prog_guide/ip_fragment_reassembly_lib.rst
 F: examples/ip_fragmentation/
 F: doc/guides/sample_app_ug/ip_frag.rst
diff --git a/app/test/meson.build b/app/test/meson.build
index 04b59cffa4..4b3c3852a2 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -58,6 +58,7 @@ test_sources = files('commands.c',
 	'test_hash_perf.c',
 	'test_hash_readwrite_lf_perf.c',
 	'test_interrupts.c',
+        'test_ipfrag.c',
 	'test_ipsec.c',
 	'test_ipsec_sad.c',
 	'test_kni.c',
@@ -187,6 +188,7 @@ fast_tests = [
         ['flow_classify_autotest', false],
         ['hash_autotest', true],
         ['interrupt_autotest', true],
+        ['ipfrag_autotest', false],
         ['logs_autotest', true],
         ['lpm_autotest', true],
         ['lpm6_autotest', true],
diff --git a/app/test/test_ipfrag.c b/app/test/test_ipfrag.c
new file mode 100644
index 0000000000..6a13e334d5
--- /dev/null
+++ b/app/test/test_ipfrag.c
@@ -0,0 +1,276 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2020 Red Hat, Inc.
+ */
+
+#include <time.h>
+
+#include <rte_common.h>
+#include <rte_cycles.h>
+#include <rte_hexdump.h>
+#include <rte_ip.h>
+#include <rte_ip_frag.h>
+#include <rte_mbuf.h>
+#include <rte_memcpy.h>
+#include <rte_random.h>
+
+#include "test.h"
+
+#ifndef ARRAY_SIZE
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+#endif
+
+static struct rte_mempool *pkt_pool,
+			  *direct_pool,
+			  *indirect_pool;
+
+static int
+setup_buf_pool(void)
+{
+#define NUM_MBUFS (128)
+#define BURST 32
+
+	if (!pkt_pool)
+		pkt_pool = rte_pktmbuf_pool_create("FRAG_MBUF_POOL",
+						   NUM_MBUFS, BURST, 0,
+						   RTE_MBUF_DEFAULT_BUF_SIZE,
+						   SOCKET_ID_ANY);
+	if (pkt_pool == NULL) {
+		printf("%s: Error creating pkt mempool\n", __func__);
+		goto bad_setup;
+	}
+
+	if (!direct_pool)
+		direct_pool = rte_pktmbuf_pool_create("FRAG_D_MBUF_POOL",
+						      NUM_MBUFS, BURST, 0,
+						      RTE_MBUF_DEFAULT_BUF_SIZE,
+						      SOCKET_ID_ANY);
+	if (!direct_pool) {
+		printf("%s: Error creating direct mempool\n", __func__);
+		goto bad_setup;
+	}
+
+	if (!indirect_pool)
+		indirect_pool = rte_pktmbuf_pool_create("FRAG_I_MBUF_POOL",
+							NUM_MBUFS, BURST, 0,
+							0, SOCKET_ID_ANY);
+	if (!indirect_pool) {
+		printf("%s: Error creating indirect mempool\n", __func__);
+		goto bad_setup;
+	}
+
+	return 0;
+
+bad_setup:
+	if (pkt_pool)
+		rte_mempool_free(pkt_pool);
+
+	if (direct_pool)
+		rte_mempool_free(direct_pool);
+
+	return TEST_FAILED;
+}
+
+static int testsuite_setup(void)
+{
+	if (setup_buf_pool())
+		return TEST_FAILED;
+	return TEST_SUCCESS;
+}
+
+static void testsuite_teardown(void)
+{
+	if (pkt_pool)
+		rte_mempool_free(pkt_pool);
+
+	if (direct_pool)
+		rte_mempool_free(direct_pool);
+
+	if (indirect_pool)
+		rte_mempool_free(indirect_pool);
+
+	pkt_pool = NULL;
+}
+
+static int ut_setup(void)
+{
+	return TEST_SUCCESS;
+}
+
+static void ut_teardown(void)
+{
+}
+
+static int
+v4_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s, int df,
+		      uint8_t ttl, uint8_t proto, uint16_t pktid)
+{
+	/* Create a packet, 2k bytes long */
+	b->data_off = 0;
+	char *data = rte_pktmbuf_mtod(b, char *);
+
+	memset(data, fill, sizeof(struct rte_ipv4_hdr) + s);
+
+	struct rte_ipv4_hdr *hdr = (struct rte_ipv4_hdr *)data;
+
+	hdr->version_ihl = 0x45; /* standard IP header... */
+	hdr->type_of_service = 0;
+	b->pkt_len = s + sizeof(struct rte_ipv4_hdr);
+	b->data_len = b->pkt_len;
+	hdr->total_length = rte_cpu_to_be_32(b->pkt_len);
+	hdr->packet_id = rte_cpu_to_be_16(pktid);
+	hdr->fragment_offset = 0;
+	if (df)
+		hdr->fragment_offset = rte_cpu_to_be_16(0x4000);
+
+	if (!ttl)
+		ttl = 64; /* default to 64 */
+
+	if (!proto)
+		proto = 1; /* icmp */
+
+	hdr->time_to_live = ttl;
+	hdr->next_proto_id = proto;
+	hdr->hdr_checksum = 0;
+	hdr->src_addr = rte_cpu_to_be_32(0x8080808);
+	hdr->dst_addr = rte_cpu_to_be_32(0x8080404);
+
+	return 0;
+}
+
+static int
+v6_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s, uint8_t ttl,
+		      uint8_t proto, uint16_t pktid)
+{
+	/* Create a packet, 2k bytes long */
+	b->data_off = 0;
+	char *data = rte_pktmbuf_mtod(b, char *);
+
+	memset(data, fill, sizeof(struct rte_ipv6_hdr) + s);
+
+	struct rte_ipv6_hdr *hdr = (struct rte_ipv6_hdr *)data;
+	b->pkt_len = s + sizeof(struct rte_ipv6_hdr);
+	b->data_len = b->pkt_len;
+
+	/* basic v6 header */
+	hdr->vtc_flow = rte_cpu_to_be_32(0x60 << 24 | pktid);
+	hdr->payload_len = rte_cpu_to_be_16(b->pkt_len);
+	hdr->proto = proto;
+	hdr->hop_limits = ttl;
+
+	memset(hdr->src_addr, 0x08, sizeof(hdr->src_addr));
+	memset(hdr->dst_addr, 0x04, sizeof(hdr->src_addr));
+
+	return 0;
+}
+
+static inline void
+test_free_fragments(struct rte_mbuf *mb[], uint32_t num)
+{
+	uint32_t i;
+	for (i = 0; i < num; i++)
+		rte_pktmbuf_free(mb[i]);
+}
+
+static int
+test_ip_frag(void)
+{
+	int result = TEST_SUCCESS;
+	size_t i;
+
+	struct test_ip_frags {
+		int     ipv;
+		size_t  mtu_size;
+		size_t  pkt_size;
+		int     set_df;
+		int     ttl;
+		uint8_t proto;
+		int     pkt_id;
+		int     expected_frags;
+	} tests[] = {
+		     {4, 1280, 1400, 0, 64, IPPROTO_ICMP, -1, 2},
+		     {4, 1280, 1400, 0, 64, IPPROTO_ICMP,  0, 2},
+		     {4,  600, 1400, 0, 64, IPPROTO_ICMP, -1, 3},
+		     {4,    4, 1400, 0, 64, IPPROTO_ICMP, -1, -EINVAL},
+		     {4,  600, 1400, 1, 64, IPPROTO_ICMP, -1, -ENOTSUP},
+		     {4,  600, 1400, 0,  0, IPPROTO_ICMP, -1, 3},
+
+		     {6, 1280, 1400, 0, 64, IPPROTO_ICMP, -1, 2},
+		     {6, 1300, 1400, 0, 64, IPPROTO_ICMP, -1, 2},
+		     {6,    4, 1400, 0, 64, IPPROTO_ICMP, -1, -EINVAL},
+		     {6, 1300, 1400, 0,  0, IPPROTO_ICMP, -1, 2},
+	};
+
+	for (i = 0; i < ARRAY_SIZE(tests); i++) {
+		int32_t len;
+		uint16_t pktid = tests[i].pkt_id;
+		struct rte_mbuf *pkts_out[BURST];
+		struct rte_mbuf *b = rte_pktmbuf_alloc(pkt_pool);
+
+		if (!b)
+			return TEST_FAILED; /* Serious error.. abort here */
+
+		if (tests[i].pkt_id == -1)
+			pktid = rte_rand_max(UINT16_MAX);
+
+		if (tests[i].ipv == 4) {
+			if (v4_allocate_packet_of(b, 0x41414141,
+						  tests[i].pkt_size,
+						  tests[i].set_df,
+						  tests[i].ttl,
+						  tests[i].proto,
+						  pktid))
+				result = TEST_FAILED;
+		} else if (tests[i].ipv == 6) {
+			if (v6_allocate_packet_of(b, 0x41414141,
+						  tests[i].pkt_size,
+						  tests[i].ttl,
+						  tests[i].proto,
+						  pktid))
+				result = TEST_FAILED;
+		}
+
+		if (tests[i].ipv == 4)
+			len = rte_ipv4_fragment_packet(b, pkts_out, BURST,
+						       tests[i].mtu_size,
+						       direct_pool,
+						       indirect_pool);
+		else
+			len = rte_ipv6_fragment_packet(b, pkts_out, BURST,
+						       tests[i].mtu_size,
+						       direct_pool,
+						       indirect_pool);
+
+		rte_pktmbuf_free(b);
+
+		if (len > 0)
+			test_free_fragments(pkts_out, len);
+
+		printf("%d: checking %d with %d\n", (int)i, len,
+		       (int)tests[i].expected_frags);
+		RTE_TEST_ASSERT_EQUAL(len, tests[i].expected_frags,
+				      "Failed case %u\n", (unsigned int)i);
+
+	}
+
+	return result;
+}
+
+static struct unit_test_suite ipfrag_testsuite  = {
+	.suite_name = "IP Frag Unit Test Suite",
+	.setup = testsuite_setup,
+	.teardown = testsuite_teardown,
+	.unit_test_cases = {
+		TEST_CASE_ST(ut_setup, ut_teardown,
+			     test_ip_frag),
+
+		TEST_CASES_END() /**< NULL terminate unit test array */
+	}
+};
+
+static int
+test_ipfrag(void)
+{
+	return unit_test_suite_runner(&ipfrag_testsuite);
+}
+
+REGISTER_TEST_COMMAND(ipfrag_autotest, test_ipfrag);
-- 
2.25.1

Re: [dpdk-dev] [PATCH v4 3/3] ipfrag: add unit test case

[Lukasz Wojciechowski]

Hi Aaron,

W dniu 15.04.2020 o 19:25, Aaron Conole pisze:
> Initial IP fragmentation unit test.
>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>   MAINTAINERS            |   1 +
>   app/test/meson.build   |   2 +
>   app/test/test_ipfrag.c | 276 +++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 279 insertions(+)
>   create mode 100644 app/test/test_ipfrag.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fe59f0224f..a77c7c17ce 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1228,6 +1228,7 @@ F: app/test/test_crc.c
>   IP fragmentation & reassembly
>   M: Konstantin Ananyev <konstantin.ananyev@intel.com>
>   F: lib/librte_ip_frag/
> +F: app/test/test_ipfrag.c
>   F: doc/guides/prog_guide/ip_fragment_reassembly_lib.rst
>   F: examples/ip_fragmentation/
>   F: doc/guides/sample_app_ug/ip_frag.rst
> diff --git a/app/test/meson.build b/app/test/meson.build
> index 04b59cffa4..4b3c3852a2 100644
> --- a/app/test/meson.build
> +++ b/app/test/meson.build
> @@ -58,6 +58,7 @@ test_sources = files('commands.c',
>   	'test_hash_perf.c',
>   	'test_hash_readwrite_lf_perf.c',
>   	'test_interrupts.c',
> +        'test_ipfrag.c',
>   	'test_ipsec.c',
>   	'test_ipsec_sad.c',
>   	'test_kni.c',
> @@ -187,6 +188,7 @@ fast_tests = [
>           ['flow_classify_autotest', false],
>           ['hash_autotest', true],
>           ['interrupt_autotest', true],
> +        ['ipfrag_autotest', false],
>           ['logs_autotest', true],
>           ['lpm_autotest', true],
>           ['lpm6_autotest', true],
> diff --git a/app/test/test_ipfrag.c b/app/test/test_ipfrag.c
> new file mode 100644
> index 0000000000..6a13e334d5
> --- /dev/null
> +++ b/app/test/test_ipfrag.c
> @@ -0,0 +1,276 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2020 Red Hat, Inc.
> + */
> +
> +#include <time.h>
> +
> +#include <rte_common.h>
> +#include <rte_cycles.h>
> +#include <rte_hexdump.h>
> +#include <rte_ip.h>
> +#include <rte_ip_frag.h>
> +#include <rte_mbuf.h>
> +#include <rte_memcpy.h>
> +#include <rte_random.h>
> +
> +#include "test.h"
> +
> +#ifndef ARRAY_SIZE
> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> +#endif
> +
> +static struct rte_mempool *pkt_pool,
> +			  *direct_pool,
> +			  *indirect_pool;
> +
> +static int
> +setup_buf_pool(void)
> +{
> +#define NUM_MBUFS (128)
> +#define BURST 32
These defines inside function look like there are local to the function, 
but of courde they are not. And theye are also used even outside the 
function. It just looks akward to me, but of course it works.
And one more question: Why is 128 in brackets?
> +
> +	if (!pkt_pool)
> +		pkt_pool = rte_pktmbuf_pool_create("FRAG_MBUF_POOL",
> +						   NUM_MBUFS, BURST, 0,
> +						   RTE_MBUF_DEFAULT_BUF_SIZE,
> +						   SOCKET_ID_ANY);
> +	if (pkt_pool == NULL) {
> +		printf("%s: Error creating pkt mempool\n", __func__);
> +		goto bad_setup;
> +	}
> +
> +	if (!direct_pool)
> +		direct_pool = rte_pktmbuf_pool_create("FRAG_D_MBUF_POOL",
> +						      NUM_MBUFS, BURST, 0,
> +						      RTE_MBUF_DEFAULT_BUF_SIZE,
> +						      SOCKET_ID_ANY);
> +	if (!direct_pool) {
> +		printf("%s: Error creating direct mempool\n", __func__);
> +		goto bad_setup;
> +	}
> +
> +	if (!indirect_pool)
> +		indirect_pool = rte_pktmbuf_pool_create("FRAG_I_MBUF_POOL",
> +							NUM_MBUFS, BURST, 0,
> +							0, SOCKET_ID_ANY);
> +	if (!indirect_pool) {
> +		printf("%s: Error creating indirect mempool\n", __func__);
> +		goto bad_setup;
> +	}
> +
> +	return 0;
return TEST_SUCCESS;
> +
> +bad_setup:
> +	if (pkt_pool)
> +		rte_mempool_free(pkt_pool);
> +
> +	if (direct_pool)
> +		rte_mempool_free(direct_pool);
> +
Why won't you set pkt_pool and direct_pool to NULL after freeing mbufs?
I know the suitcase is intended to be run just once, but you'll never know.
> +	return TEST_FAILED;
> +}
> +
> +static int testsuite_setup(void)
> +{
> +	if (setup_buf_pool())
> +		return TEST_FAILED;
> +	return TEST_SUCCESS;
or just:
return setup_buf_pool();
returning value != 0 does not mean that test failed. It can be skipped 
or unsupported in current configuration, etc.
> +}
> +
> +static void testsuite_teardown(void)
> +{
> +	if (pkt_pool)
> +		rte_mempool_free(pkt_pool);
> +
> +	if (direct_pool)
> +		rte_mempool_free(direct_pool);
> +
> +	if (indirect_pool)
> +		rte_mempool_free(indirect_pool);
> +
> +	pkt_pool = NULL;
What about zeroing other pointers?
> +}
> +
> +static int ut_setup(void)
> +{
> +	return TEST_SUCCESS;
> +}
> +
> +static void ut_teardown(void)
> +{
> +}
> +
> +static int
> +v4_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s, int df,
> +		      uint8_t ttl, uint8_t proto, uint16_t pktid)
> +{
> +	/* Create a packet, 2k bytes long */
> +	b->data_off = 0;
> +	char *data = rte_pktmbuf_mtod(b, char *);
> +
> +	memset(data, fill, sizeof(struct rte_ipv4_hdr) + s);
Is filling also header necessary. You overwrite all the fields anyway.
> +
> +	struct rte_ipv4_hdr *hdr = (struct rte_ipv4_hdr *)data;
> +
> +	hdr->version_ihl = 0x45; /* standard IP header... */
> +	hdr->type_of_service = 0;
> +	b->pkt_len = s + sizeof(struct rte_ipv4_hdr);
> +	b->data_len = b->pkt_len;
> +	hdr->total_length = rte_cpu_to_be_32(b->pkt_len);
Why rte_cpu_to_be_32 not rte_cpu_to_be_16 ? The struct rte_ipv4_hdr 
defines:  rte_be16_t total_length;
> +	hdr->packet_id = rte_cpu_to_be_16(pktid);
> +	hdr->fragment_offset = 0;
> +	if (df)
> +		hdr->fragment_offset = rte_cpu_to_be_16(0x4000);
> +
> +	if (!ttl)
> +		ttl = 64; /* default to 64 */
> +
> +	if (!proto)
> +		proto = 1; /* icmp */
> +
> +	hdr->time_to_live = ttl;
> +	hdr->next_proto_id = proto;
> +	hdr->hdr_checksum = 0;
> +	hdr->src_addr = rte_cpu_to_be_32(0x8080808);
> +	hdr->dst_addr = rte_cpu_to_be_32(0x8080404);
> +
> +	return 0;
The function return int, but there is only one execution path. Do you 
plan to add some checks? If not maybe consider changing the function to 
void-returning.
> +}
> +
> +static int
> +v6_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s, uint8_t ttl,
> +		      uint8_t proto, uint16_t pktid)
> +{
> +	/* Create a packet, 2k bytes long */
> +	b->data_off = 0;
> +	char *data = rte_pktmbuf_mtod(b, char *);
> +
> +	memset(data, fill, sizeof(struct rte_ipv6_hdr) + s);
Why do you fill also header?
> +
> +	struct rte_ipv6_hdr *hdr = (struct rte_ipv6_hdr *)data;
> +	b->pkt_len = s + sizeof(struct rte_ipv6_hdr);
> +	b->data_len = b->pkt_len;
> +
> +	/* basic v6 header */
> +	hdr->vtc_flow = rte_cpu_to_be_32(0x60 << 24 | pktid);
> +	hdr->payload_len = rte_cpu_to_be_16(b->pkt_len);
> +	hdr->proto = proto;
> +	hdr->hop_limits = ttl;
> +
> +	memset(hdr->src_addr, 0x08, sizeof(hdr->src_addr));
> +	memset(hdr->dst_addr, 0x04, sizeof(hdr->src_addr));
> +
> +	return 0;
Only one patch of execution. Consider changing function signature to void.
> +}
> +
> +static inline void
> +test_free_fragments(struct rte_mbuf *mb[], uint32_t num)
> +{
> +	uint32_t i;
> +	for (i = 0; i < num; i++)
> +		rte_pktmbuf_free(mb[i]);
> +}
> +
> +static int
> +test_ip_frag(void)
> +{
> +	int result = TEST_SUCCESS;
> +	size_t i;
> +
> +	struct test_ip_frags {
> +		int     ipv;
> +		size_t  mtu_size;
> +		size_t  pkt_size;
> +		int     set_df;
> +		int     ttl;
uint8_t ttl will avoid conversions in allocate_packet_of function calls
> +		uint8_t proto;
> +		int     pkt_id;
You can use uint16_t for pkt_id with e.g. UINT16_MAX being a special 
value for randomization
> +		int     expected_frags;
> +	} tests[] = {
> +		     {4, 1280, 1400, 0, 64, IPPROTO_ICMP, -1, 2},
> +		     {4, 1280, 1400, 0, 64, IPPROTO_ICMP,  0, 2},
> +		     {4,  600, 1400, 0, 64, IPPROTO_ICMP, -1, 3},
> +		     {4,    4, 1400, 0, 64, IPPROTO_ICMP, -1, -EINVAL},
> +		     {4,  600, 1400, 1, 64, IPPROTO_ICMP, -1, -ENOTSUP},
> +		     {4,  600, 1400, 0,  0, IPPROTO_ICMP, -1, 3},
> +
> +		     {6, 1280, 1400, 0, 64, IPPROTO_ICMP, -1, 2},
> +		     {6, 1300, 1400, 0, 64, IPPROTO_ICMP, -1, 2},
> +		     {6,    4, 1400, 0, 64, IPPROTO_ICMP, -1, -EINVAL},
> +		     {6, 1300, 1400, 0,  0, IPPROTO_ICMP, -1, 2},
> +	};
> +
> +	for (i = 0; i < ARRAY_SIZE(tests); i++) {
> +		int32_t len;
> +		uint16_t pktid = tests[i].pkt_id;
> +		struct rte_mbuf *pkts_out[BURST];
> +		struct rte_mbuf *b = rte_pktmbuf_alloc(pkt_pool);
> +
> +		if (!b)
> +			return TEST_FAILED; /* Serious error.. abort here */
Please log something here, otherwise if it happens nobody will know why 
the test failed.
> +
> +		if (tests[i].pkt_id == -1)
> +			pktid = rte_rand_max(UINT16_MAX);
> +
> +		if (tests[i].ipv == 4) {
> +			if (v4_allocate_packet_of(b, 0x41414141,
> +						  tests[i].pkt_size,
> +						  tests[i].set_df,
> +						  tests[i].ttl,
> +						  tests[i].proto,
> +						  pktid))
> +				result = TEST_FAILED;
Some log would be appreciated to know during the execution what is the 
cause of failure, in which testcase etc.
Maybe the whole if is not necessary as the allocate_packet_of function 
cannot fail
But if you decide to leave the if, please keep in mind that you continue 
execution of test even without those packet allocated!
> +		} else if (tests[i].ipv == 6) {
> +			if (v6_allocate_packet_of(b, 0x41414141,
> +						  tests[i].pkt_size,
> +						  tests[i].ttl,
> +						  tests[i].proto,
> +						  pktid))
> +				result = TEST_FAILED;
same as above
> +		}
> +
> +		if (tests[i].ipv == 4)
> +			len = rte_ipv4_fragment_packet(b, pkts_out, BURST,
> +						       tests[i].mtu_size,
> +						       direct_pool,
> +						       indirect_pool);
> +		else
Above you use: else if (tests[i].ipv == 6), maybe use same here to keep 
things consistent.
> +			len = rte_ipv6_fragment_packet(b, pkts_out, BURST,
> +						       tests[i].mtu_size,
> +						       direct_pool,
> +						       indirect_pool);
> +
> +		rte_pktmbuf_free(b);
> +
> +		if (len > 0)
> +			test_free_fragments(pkts_out, len);
> +
> +		printf("%d: checking %d with %d\n", (int)i, len,
> +		       (int)tests[i].expected_frags);

You don't need to convert variables to ints. The i is size_t type, so 
you can use %z in format message and the expected frags is already an int.

It would be also nice to be more verbose here. The current message does 
not tell much about what failed and why. I just received the following 
during the test:

  + ------------------------------------------------------- +
  + Test Suite : IP Frag Unit Test Suite
  + ------------------------------------------------------- +
0: checking 2 with 2
1: checking 2 with 2
2: checking 3 with 3
3: checking 1 with -22
  + TestCase [ 0] : test_ip_frag failed
  + ------------------------------------------------------- +

> +		RTE_TEST_ASSERT_EQUAL(len, tests[i].expected_frags,
> +				      "Failed case %u\n", (unsigned int)i);

You can use %z format for size_t type parameter.

And as you can see in the execution above, the assert didn't produce any 
log, that's because there are no log levels configured.
So please add these following lines in the test_ipfrag to enable logs:

static int
test_ipfrag(void)
{
+    rte_log_set_global_level(RTE_LOG_DEBUG);
+    rte_log_set_level(RTE_LOGTYPE_EAL, RTE_LOG_DEBUG);
+
     return unit_test_suite_runner(&ipfrag_testsuite);
}

So you'll get something like this:
  + ------------------------------------------------------- +
0: checking 2 with 2
1: checking 2 with 2
2: checking 3 with 3
3: checking 1 with -22
EAL: Test assert test_ip_frag line 253 failed: Failed case 3

  + TestCase [ 0] : test_ip_frag failed

> +
> +	}
> +
> +	return result;
> +}
> +
> +static struct unit_test_suite ipfrag_testsuite  = {
> +	.suite_name = "IP Frag Unit Test Suite",
> +	.setup = testsuite_setup,
> +	.teardown = testsuite_teardown,
> +	.unit_test_cases = {
> +		TEST_CASE_ST(ut_setup, ut_teardown,
> +			     test_ip_frag),
> +
> +		TEST_CASES_END() /**< NULL terminate unit test array */
> +	}
> +};
> +
> +static int
> +test_ipfrag(void)
> +{
> +	return unit_test_suite_runner(&ipfrag_testsuite);
> +}
> +
> +REGISTER_TEST_COMMAND(ipfrag_autotest, test_ipfrag);

And don't worry the tests pass with the other patches applied.

Best regards

-- 

Lukasz Wojciechowski
Principal Software Engineer

Samsung R&D Institute Poland
Samsung Electronics
Office +48 22 377 88 25
l.wojciechow@partner.samsung.com

Re: [dpdk-dev] [PATCH v4 3/3] ipfrag: add unit test case

[Aaron Conole]

Lukasz Wojciechowski <l.wojciechow@partner.samsung.com> writes:

> Hi Aaron,
>
> W dniu 15.04.2020 o 19:25, Aaron Conole pisze:
>> Initial IP fragmentation unit test.
>>
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> ---

Thanks for the review, Lukasz!

>>   MAINTAINERS            |   1 +
>>   app/test/meson.build   |   2 +
>>   app/test/test_ipfrag.c | 276 +++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 279 insertions(+)
>>   create mode 100644 app/test/test_ipfrag.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index fe59f0224f..a77c7c17ce 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1228,6 +1228,7 @@ F: app/test/test_crc.c
>>   IP fragmentation & reassembly
>>   M: Konstantin Ananyev <konstantin.ananyev@intel.com>
>>   F: lib/librte_ip_frag/
>> +F: app/test/test_ipfrag.c
>>   F: doc/guides/prog_guide/ip_fragment_reassembly_lib.rst
>>   F: examples/ip_fragmentation/
>>   F: doc/guides/sample_app_ug/ip_frag.rst
>> diff --git a/app/test/meson.build b/app/test/meson.build
>> index 04b59cffa4..4b3c3852a2 100644
>> --- a/app/test/meson.build
>> +++ b/app/test/meson.build
>> @@ -58,6 +58,7 @@ test_sources = files('commands.c',
>>   	'test_hash_perf.c',
>>   	'test_hash_readwrite_lf_perf.c',
>>   	'test_interrupts.c',
>> +        'test_ipfrag.c',
>>   	'test_ipsec.c',
>>   	'test_ipsec_sad.c',
>>   	'test_kni.c',
>> @@ -187,6 +188,7 @@ fast_tests = [
>>           ['flow_classify_autotest', false],
>>           ['hash_autotest', true],
>>           ['interrupt_autotest', true],
>> +        ['ipfrag_autotest', false],
>>           ['logs_autotest', true],
>>           ['lpm_autotest', true],
>>           ['lpm6_autotest', true],
>> diff --git a/app/test/test_ipfrag.c b/app/test/test_ipfrag.c
>> new file mode 100644
>> index 0000000000..6a13e334d5
>> --- /dev/null
>> +++ b/app/test/test_ipfrag.c
>> @@ -0,0 +1,276 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause
>> + * Copyright(c) 2020 Red Hat, Inc.
>> + */
>> +
>> +#include <time.h>
>> +
>> +#include <rte_common.h>
>> +#include <rte_cycles.h>
>> +#include <rte_hexdump.h>
>> +#include <rte_ip.h>
>> +#include <rte_ip_frag.h>
>> +#include <rte_mbuf.h>
>> +#include <rte_memcpy.h>
>> +#include <rte_random.h>
>> +
>> +#include "test.h"
>> +
>> +#ifndef ARRAY_SIZE
>> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>> +#endif
>> +
>> +static struct rte_mempool *pkt_pool,
>> +			  *direct_pool,
>> +			  *indirect_pool;
>> +
>> +static int
>> +setup_buf_pool(void)
>> +{
>> +#define NUM_MBUFS (128)
>> +#define BURST 32
> These defines inside function look like there are local to the function, 
> but of courde they are not. And theye are also used even outside the 
> function. It just looks akward to me, but of course it works.

I moved them to the top of the block.

> And one more question: Why is 128 in brackets?

Leftover from a pre-post version

>> +
>> +	if (!pkt_pool)
>> +		pkt_pool = rte_pktmbuf_pool_create("FRAG_MBUF_POOL",
>> +						   NUM_MBUFS, BURST, 0,
>> +						   RTE_MBUF_DEFAULT_BUF_SIZE,
>> +						   SOCKET_ID_ANY);
>> +	if (pkt_pool == NULL) {
>> +		printf("%s: Error creating pkt mempool\n", __func__);
>> +		goto bad_setup;
>> +	}
>> +
>> +	if (!direct_pool)
>> +		direct_pool = rte_pktmbuf_pool_create("FRAG_D_MBUF_POOL",
>> +						      NUM_MBUFS, BURST, 0,
>> +						      RTE_MBUF_DEFAULT_BUF_SIZE,
>> +						      SOCKET_ID_ANY);
>> +	if (!direct_pool) {
>> +		printf("%s: Error creating direct mempool\n", __func__);
>> +		goto bad_setup;
>> +	}
>> +
>> +	if (!indirect_pool)
>> +		indirect_pool = rte_pktmbuf_pool_create("FRAG_I_MBUF_POOL",
>> +							NUM_MBUFS, BURST, 0,
>> +							0, SOCKET_ID_ANY);
>> +	if (!indirect_pool) {
>> +		printf("%s: Error creating indirect mempool\n", __func__);
>> +		goto bad_setup;
>> +	}
>> +
>> +	return 0;
> return TEST_SUCCESS;

Fixed.

>> +
>> +bad_setup:
>> +	if (pkt_pool)
>> +		rte_mempool_free(pkt_pool);
>> +
>> +	if (direct_pool)
>> +		rte_mempool_free(direct_pool);
>> +
> Why won't you set pkt_pool and direct_pool to NULL after freeing mbufs?
> I know the suitcase is intended to be run just once, but you'll never know.

Fixed.

>> +	return TEST_FAILED;
>> +}
>> +
>> +static int testsuite_setup(void)
>> +{
>> +	if (setup_buf_pool())
>> +		return TEST_FAILED;
>> +	return TEST_SUCCESS;
> or just:
> return setup_buf_pool();

Done.

> returning value != 0 does not mean that test failed. It can be skipped 
> or unsupported in current configuration, etc.
>> +}
>> +
>> +static void testsuite_teardown(void)
>> +{
>> +	if (pkt_pool)
>> +		rte_mempool_free(pkt_pool);
>> +
>> +	if (direct_pool)
>> +		rte_mempool_free(direct_pool);
>> +
>> +	if (indirect_pool)
>> +		rte_mempool_free(indirect_pool);
>> +
>> +	pkt_pool = NULL;
> What about zeroing other pointers?

Done.

>> +}
>> +
>> +static int ut_setup(void)
>> +{
>> +	return TEST_SUCCESS;
>> +}
>> +
>> +static void ut_teardown(void)
>> +{
>> +}
>> +
>> +static int
>> +v4_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s, int df,
>> +		      uint8_t ttl, uint8_t proto, uint16_t pktid)
>> +{
>> +	/* Create a packet, 2k bytes long */
>> +	b->data_off = 0;
>> +	char *data = rte_pktmbuf_mtod(b, char *);
>> +
>> +	memset(data, fill, sizeof(struct rte_ipv4_hdr) + s);
> Is filling also header necessary. You overwrite all the fields anyway.

It's from a pre-posted version when I was debugging.  As you point out,
it doesn't really matter too much.  I will cut it in here and the v6 as
well.

>> +
>> +	struct rte_ipv4_hdr *hdr = (struct rte_ipv4_hdr *)data;
>> +
>> +	hdr->version_ihl = 0x45; /* standard IP header... */
>> +	hdr->type_of_service = 0;
>> +	b->pkt_len = s + sizeof(struct rte_ipv4_hdr);
>> +	b->data_len = b->pkt_len;
>> +	hdr->total_length = rte_cpu_to_be_32(b->pkt_len);
> Why rte_cpu_to_be_32 not rte_cpu_to_be_16 ? The struct rte_ipv4_hdr 
> defines:  rte_be16_t total_length;

That was a mistake when converting from htons.

>> +	hdr->packet_id = rte_cpu_to_be_16(pktid);
>> +	hdr->fragment_offset = 0;
>> +	if (df)
>> +		hdr->fragment_offset = rte_cpu_to_be_16(0x4000);
>> +
>> +	if (!ttl)
>> +		ttl = 64; /* default to 64 */
>> +
>> +	if (!proto)
>> +		proto = 1; /* icmp */
>> +
>> +	hdr->time_to_live = ttl;
>> +	hdr->next_proto_id = proto;
>> +	hdr->hdr_checksum = 0;
>> +	hdr->src_addr = rte_cpu_to_be_32(0x8080808);
>> +	hdr->dst_addr = rte_cpu_to_be_32(0x8080404);
>> +
>> +	return 0;
> The function return int, but there is only one execution path. Do you 
> plan to add some checks? If not maybe consider changing the function to 
> void-returning.

Done.

>> +}
>> +
>> +static int
>> +v6_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s, uint8_t ttl,
>> +		      uint8_t proto, uint16_t pktid)
>> +{
>> +	/* Create a packet, 2k bytes long */
>> +	b->data_off = 0;
>> +	char *data = rte_pktmbuf_mtod(b, char *);
>> +
>> +	memset(data, fill, sizeof(struct rte_ipv6_hdr) + s);
> Why do you fill also header?
>> +
>> +	struct rte_ipv6_hdr *hdr = (struct rte_ipv6_hdr *)data;
>> +	b->pkt_len = s + sizeof(struct rte_ipv6_hdr);
>> +	b->data_len = b->pkt_len;
>> +
>> +	/* basic v6 header */
>> +	hdr->vtc_flow = rte_cpu_to_be_32(0x60 << 24 | pktid);
>> +	hdr->payload_len = rte_cpu_to_be_16(b->pkt_len);
>> +	hdr->proto = proto;
>> +	hdr->hop_limits = ttl;
>> +
>> +	memset(hdr->src_addr, 0x08, sizeof(hdr->src_addr));
>> +	memset(hdr->dst_addr, 0x04, sizeof(hdr->src_addr));
>> +
>> +	return 0;
> Only one patch of execution. Consider changing function signature to void.
>> +}
>> +
>> +static inline void
>> +test_free_fragments(struct rte_mbuf *mb[], uint32_t num)
>> +{
>> +	uint32_t i;
>> +	for (i = 0; i < num; i++)
>> +		rte_pktmbuf_free(mb[i]);
>> +}
>> +
>> +static int
>> +test_ip_frag(void)
>> +{
>> +	int result = TEST_SUCCESS;
>> +	size_t i;
>> +
>> +	struct test_ip_frags {
>> +		int     ipv;
>> +		size_t  mtu_size;
>> +		size_t  pkt_size;
>> +		int     set_df;
>> +		int     ttl;
> uint8_t ttl will avoid conversions in allocate_packet_of function calls

Okay.  Done.

>> +		uint8_t proto;
>> +		int     pkt_id;
> You can use uint16_t for pkt_id with e.g. UINT16_MAX being a special 
> value for randomization

Done.

>> +		int     expected_frags;
>> +	} tests[] = {
>> +		     {4, 1280, 1400, 0, 64, IPPROTO_ICMP, -1, 2},
>> +		     {4, 1280, 1400, 0, 64, IPPROTO_ICMP,  0, 2},
>> +		     {4,  600, 1400, 0, 64, IPPROTO_ICMP, -1, 3},
>> +		     {4,    4, 1400, 0, 64, IPPROTO_ICMP, -1, -EINVAL},
>> +		     {4,  600, 1400, 1, 64, IPPROTO_ICMP, -1, -ENOTSUP},
>> +		     {4,  600, 1400, 0,  0, IPPROTO_ICMP, -1, 3},
>> +
>> +		     {6, 1280, 1400, 0, 64, IPPROTO_ICMP, -1, 2},
>> +		     {6, 1300, 1400, 0, 64, IPPROTO_ICMP, -1, 2},
>> +		     {6,    4, 1400, 0, 64, IPPROTO_ICMP, -1, -EINVAL},
>> +		     {6, 1300, 1400, 0,  0, IPPROTO_ICMP, -1, 2},
>> +	};
>> +
>> +	for (i = 0; i < ARRAY_SIZE(tests); i++) {
>> +		int32_t len;
>> +		uint16_t pktid = tests[i].pkt_id;
>> +		struct rte_mbuf *pkts_out[BURST];
>> +		struct rte_mbuf *b = rte_pktmbuf_alloc(pkt_pool);
>> +
>> +		if (!b)
>> +			return TEST_FAILED; /* Serious error.. abort here */
> Please log something here, otherwise if it happens nobody will know why 
> the test failed.

Changed to a test assert.

>> +
>> +		if (tests[i].pkt_id == -1)
>> +			pktid = rte_rand_max(UINT16_MAX);
>> +
>> +		if (tests[i].ipv == 4) {
>> +			if (v4_allocate_packet_of(b, 0x41414141,
>> +						  tests[i].pkt_size,
>> +						  tests[i].set_df,
>> +						  tests[i].ttl,
>> +						  tests[i].proto,
>> +						  pktid))
>> +				result = TEST_FAILED;
> Some log would be appreciated to know during the execution what is the 
> cause of failure, in which testcase etc.
> Maybe the whole if is not necessary as the allocate_packet_of function 
> cannot fail
> But if you decide to leave the if, please keep in mind that you continue 
> execution of test even without those packet allocated!

Clipped.

>> +		} else if (tests[i].ipv == 6) {
>> +			if (v6_allocate_packet_of(b, 0x41414141,
>> +						  tests[i].pkt_size,
>> +						  tests[i].ttl,
>> +						  tests[i].proto,
>> +						  pktid))
>> +				result = TEST_FAILED;
> same as above
>> +		}
>> +
>> +		if (tests[i].ipv == 4)
>> +			len = rte_ipv4_fragment_packet(b, pkts_out, BURST,
>> +						       tests[i].mtu_size,
>> +						       direct_pool,
>> +						       indirect_pool);
>> +		else
> Above you use: else if (tests[i].ipv == 6), maybe use same here to keep 
> things consistent.

Okay, done.

>> +			len = rte_ipv6_fragment_packet(b, pkts_out, BURST,
>> +						       tests[i].mtu_size,
>> +						       direct_pool,
>> +						       indirect_pool);
>> +
>> +		rte_pktmbuf_free(b);
>> +
>> +		if (len > 0)
>> +			test_free_fragments(pkts_out, len);
>> +
>> +		printf("%d: checking %d with %d\n", (int)i, len,
>> +		       (int)tests[i].expected_frags);
>
> You don't need to convert variables to ints. The i is size_t type, so 
> you can use %z in format message and the expected frags is already an int.

Done.

> It would be also nice to be more verbose here. The current message does 
> not tell much about what failed and why. I just received the following 
> during the test:
>
>   + ------------------------------------------------------- +
>   + Test Suite : IP Frag Unit Test Suite
>   + ------------------------------------------------------- +
> 0: checking 2 with 2
> 1: checking 2 with 2
> 2: checking 3 with 3
> 3: checking 1 with -22
>   + TestCase [ 0] : test_ip_frag failed
>   + ------------------------------------------------------- +
>
>> +		RTE_TEST_ASSERT_EQUAL(len, tests[i].expected_frags,
>> +				      "Failed case %u\n", (unsigned int)i);
>
> You can use %z format for size_t type parameter.
>
> And as you can see in the execution above, the assert didn't produce any 
> log, that's because there are no log levels configured.
> So please add these following lines in the test_ipfrag to enable logs:

Done.

> static int
> test_ipfrag(void)
> {
> +    rte_log_set_global_level(RTE_LOG_DEBUG);
> +    rte_log_set_level(RTE_LOGTYPE_EAL, RTE_LOG_DEBUG);
> +
>      return unit_test_suite_runner(&ipfrag_testsuite);
> }
>
> So you'll get something like this:
>   + ------------------------------------------------------- +
> 0: checking 2 with 2
> 1: checking 2 with 2
> 2: checking 3 with 3
> 3: checking 1 with -22
> EAL: Test assert test_ip_frag line 253 failed: Failed case 3
>
>   + TestCase [ 0] : test_ip_frag failed
>
>> +
>> +	}
>> +
>> +	return result;
>> +}
>> +
>> +static struct unit_test_suite ipfrag_testsuite  = {
>> +	.suite_name = "IP Frag Unit Test Suite",
>> +	.setup = testsuite_setup,
>> +	.teardown = testsuite_teardown,
>> +	.unit_test_cases = {
>> +		TEST_CASE_ST(ut_setup, ut_teardown,
>> +			     test_ip_frag),
>> +
>> +		TEST_CASES_END() /**< NULL terminate unit test array */
>> +	}
>> +};
>> +
>> +static int
>> +test_ipfrag(void)
>> +{
>> +	return unit_test_suite_runner(&ipfrag_testsuite);
>> +}
>> +
>> +REGISTER_TEST_COMMAND(ipfrag_autotest, test_ipfrag);
>
> And don't worry the tests pass with the other patches applied.

:-)

> Best regards

Re: [dpdk-dev] [PATCH v4 1/3] ip_frag: ensure minimum v4 fragmentation length

[Lukasz Wojciechowski]

W dniu 15.04.2020 o 19:25, Aaron Conole pisze:
> Do a formal parameter check of mtu length, as well as
> checking the the various inputs for validity.  If any
> aren't acceptable, we bail.
>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>   lib/librte_ip_frag/rte_ipv4_fragmentation.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/lib/librte_ip_frag/rte_ipv4_fragmentation.c b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
> index 9e9f986cc5..c36678a6d2 100644
> --- a/lib/librte_ip_frag/rte_ipv4_fragmentation.c
> +++ b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
> @@ -76,6 +76,15 @@ rte_ipv4_fragment_packet(struct rte_mbuf *pkt_in,
>   	uint16_t fragment_offset, flag_offset, frag_size;
>   	uint16_t frag_bytes_remaining;
>   
> +	/*
> +	 * Formal parameter checking.
> +	 */
> +	if (unlikely(pkt_in == NULL) || unlikely(pkts_out == NULL) ||
> +	    unlikely(nb_pkts_out == 0) ||
> +	    unlikely(pool_direct == NULL) || unlikely(pool_indirect == NULL) ||
> +	    unlikely(mtu_size < 68))
> +		return -EINVAL;
> +
>   	/*
>   	 * Ensure the IP payload length of all fragments is aligned to a
>   	 * multiple of 8 bytes as per RFC791 section 2.3.

Acked-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>

-- 

Lukasz Wojciechowski
Principal Software Engineer

Samsung R&D Institute Poland
Samsung Electronics
Office +48 22 377 88 25
l.wojciechow@partner.samsung.com

Re: [dpdk-dev] [PATCH v4 2/3] ip_frag: ensure minimum v6 fragmentation length

[Lukasz Wojciechowski]

W dniu 15.04.2020 o 19:25, Aaron Conole pisze:
> In addition, do a formal parameter check.
>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>   lib/librte_ip_frag/rte_ipv6_fragmentation.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/lib/librte_ip_frag/rte_ipv6_fragmentation.c b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> index 43449970e5..ee984aed82 100644
> --- a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> +++ b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> @@ -79,6 +79,15 @@ rte_ipv6_fragment_packet(struct rte_mbuf *pkt_in,
>   	uint16_t fragment_offset, frag_size;
>   	uint64_t frag_bytes_remaining;
>   
> +	/*
> +	 * Formal parameter checking.
> +	 */
> +	if (unlikely(pkt_in == NULL) || unlikely(pkts_out == NULL) ||
> +	    unlikely(nb_pkts_out == 0) ||
> +	    unlikely(pool_direct == NULL) || unlikely(pool_indirect == NULL) ||
> +	    unlikely(mtu_size < 1280))
> +		return -EINVAL;
> +
>   	/*
>   	 * Ensure the IP payload length of all fragments (except the
>   	 * the last fragment) are a multiple of 8 bytes per RFC2460.

Acked-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>


-- 

Lukasz Wojciechowski
Principal Software Engineer

Samsung R&D Institute Poland
Samsung Electronics
Office +48 22 377 88 25
l.wojciechow@partner.samsung.com

[dpdk-dev] [PATCH v5 0/3] ip_frag: add a unit test for fragmentation

[Aaron Conole]

This adds a simple unit test for the ip fragmentation library
and covers fragmenting ipv4 and ipv6.  Additionally, some fixes
are introduced, which are really just sanity rather than real
issues in the field.

v4->v5:
- Address coding style comments in patch 3/3

v3->v4:
- Drop the frag header detection from ipv6 code
- Full parameter sanity checking in ipv4 and ipv6 code
- Convert from htons/htonl to rte_cpu_to_be_...

v2->v3:
- Remove c99-ism from 4/4

v1->v2:
- Fix patch 4/4 which had a missing assignment for pktid.

Aaron Conole (3):
  ip_frag: ensure minimum v4 fragmentation length
  ip_frag: ensure minimum v6 fragmentation length
  ipfrag: add unit test case

 MAINTAINERS                                 |   1 +
 app/test/meson.build                        |   2 +
 app/test/test_ipfrag.c                      | 272 ++++++++++++++++++++
 lib/librte_ip_frag/rte_ipv4_fragmentation.c |   9 +
 lib/librte_ip_frag/rte_ipv6_fragmentation.c |   9 +
 5 files changed, 293 insertions(+)
 create mode 100644 app/test/test_ipfrag.c

-- 
2.25.1

[dpdk-dev] [PATCH v5 1/3] ip_frag: ensure minimum v4 fragmentation length

[Aaron Conole]

Do a formal parameter check of mtu length, as well as
checking the the various inputs for validity.  If any
aren't acceptable, we bail.

Acked-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 lib/librte_ip_frag/rte_ipv4_fragmentation.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lib/librte_ip_frag/rte_ipv4_fragmentation.c b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
index 9e9f986cc5..c36678a6d2 100644
--- a/lib/librte_ip_frag/rte_ipv4_fragmentation.c
+++ b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
@@ -76,6 +76,15 @@ rte_ipv4_fragment_packet(struct rte_mbuf *pkt_in,
 	uint16_t fragment_offset, flag_offset, frag_size;
 	uint16_t frag_bytes_remaining;
 
+	/*
+	 * Formal parameter checking.
+	 */
+	if (unlikely(pkt_in == NULL) || unlikely(pkts_out == NULL) ||
+	    unlikely(nb_pkts_out == 0) ||
+	    unlikely(pool_direct == NULL) || unlikely(pool_indirect == NULL) ||
+	    unlikely(mtu_size < 68))
+		return -EINVAL;
+
 	/*
 	 * Ensure the IP payload length of all fragments is aligned to a
 	 * multiple of 8 bytes as per RFC791 section 2.3.
-- 
2.25.1

[dpdk-dev] [PATCH v5 2/3] ip_frag: ensure minimum v6 fragmentation length

[Aaron Conole]

In addition, do a formal parameter check.

Acked-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 lib/librte_ip_frag/rte_ipv6_fragmentation.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lib/librte_ip_frag/rte_ipv6_fragmentation.c b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
index 43449970e5..ee984aed82 100644
--- a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
+++ b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
@@ -79,6 +79,15 @@ rte_ipv6_fragment_packet(struct rte_mbuf *pkt_in,
 	uint16_t fragment_offset, frag_size;
 	uint64_t frag_bytes_remaining;
 
+	/*
+	 * Formal parameter checking.
+	 */
+	if (unlikely(pkt_in == NULL) || unlikely(pkts_out == NULL) ||
+	    unlikely(nb_pkts_out == 0) ||
+	    unlikely(pool_direct == NULL) || unlikely(pool_indirect == NULL) ||
+	    unlikely(mtu_size < 1280))
+		return -EINVAL;
+
 	/*
 	 * Ensure the IP payload length of all fragments (except the
 	 * the last fragment) are a multiple of 8 bytes per RFC2460.
-- 
2.25.1

[dpdk-dev] [PATCH v5 3/3] ipfrag: add unit test case

[Aaron Conole]

Initial IP fragmentation unit test.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 MAINTAINERS            |   1 +
 app/test/meson.build   |   2 +
 app/test/test_ipfrag.c | 272 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 275 insertions(+)
 create mode 100644 app/test/test_ipfrag.c

diff --git a/MAINTAINERS b/MAINTAINERS
index fe59f0224f..a77c7c17ce 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1228,6 +1228,7 @@ F: app/test/test_crc.c
 IP fragmentation & reassembly
 M: Konstantin Ananyev <konstantin.ananyev@intel.com>
 F: lib/librte_ip_frag/
+F: app/test/test_ipfrag.c
 F: doc/guides/prog_guide/ip_fragment_reassembly_lib.rst
 F: examples/ip_fragmentation/
 F: doc/guides/sample_app_ug/ip_frag.rst
diff --git a/app/test/meson.build b/app/test/meson.build
index 04b59cffa4..4b3c3852a2 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -58,6 +58,7 @@ test_sources = files('commands.c',
 	'test_hash_perf.c',
 	'test_hash_readwrite_lf_perf.c',
 	'test_interrupts.c',
+        'test_ipfrag.c',
 	'test_ipsec.c',
 	'test_ipsec_sad.c',
 	'test_kni.c',
@@ -187,6 +188,7 @@ fast_tests = [
         ['flow_classify_autotest', false],
         ['hash_autotest', true],
         ['interrupt_autotest', true],
+        ['ipfrag_autotest', false],
         ['logs_autotest', true],
         ['lpm_autotest', true],
         ['lpm6_autotest', true],
diff --git a/app/test/test_ipfrag.c b/app/test/test_ipfrag.c
new file mode 100644
index 0000000000..6a80581f0e
--- /dev/null
+++ b/app/test/test_ipfrag.c
@@ -0,0 +1,272 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2020 Red Hat, Inc.
+ */
+
+#include <time.h>
+
+#include <rte_common.h>
+#include <rte_cycles.h>
+#include <rte_hexdump.h>
+#include <rte_ip.h>
+#include <rte_ip_frag.h>
+#include <rte_mbuf.h>
+#include <rte_memcpy.h>
+#include <rte_random.h>
+
+#include "test.h"
+
+#define NUM_MBUFS 128
+#define BURST 32
+
+static struct rte_mempool *pkt_pool,
+			  *direct_pool,
+			  *indirect_pool;
+
+static int
+setup_buf_pool(void)
+{
+	if (!pkt_pool)
+		pkt_pool = rte_pktmbuf_pool_create("FRAG_MBUF_POOL",
+						   NUM_MBUFS, BURST, 0,
+						   RTE_MBUF_DEFAULT_BUF_SIZE,
+						   SOCKET_ID_ANY);
+	if (pkt_pool == NULL) {
+		printf("%s: Error creating pkt mempool\n", __func__);
+		goto bad_setup;
+	}
+
+	if (!direct_pool)
+		direct_pool = rte_pktmbuf_pool_create("FRAG_D_MBUF_POOL",
+						      NUM_MBUFS, BURST, 0,
+						      RTE_MBUF_DEFAULT_BUF_SIZE,
+						      SOCKET_ID_ANY);
+	if (!direct_pool) {
+		printf("%s: Error creating direct mempool\n", __func__);
+		goto bad_setup;
+	}
+
+	if (!indirect_pool)
+		indirect_pool = rte_pktmbuf_pool_create("FRAG_I_MBUF_POOL",
+							NUM_MBUFS, BURST, 0,
+							0, SOCKET_ID_ANY);
+	if (!indirect_pool) {
+		printf("%s: Error creating indirect mempool\n", __func__);
+		goto bad_setup;
+	}
+
+	return TEST_SUCCESS;
+
+bad_setup:
+	if (pkt_pool)
+		rte_mempool_free(pkt_pool);
+	pkt_pool = NULL;
+
+	if (direct_pool)
+		rte_mempool_free(direct_pool);
+	direct_pool = NULL;
+
+	return TEST_FAILED;
+}
+
+static int testsuite_setup(void)
+{
+	return setup_buf_pool();
+}
+
+static void testsuite_teardown(void)
+{
+	if (pkt_pool)
+		rte_mempool_free(pkt_pool);
+
+	if (direct_pool)
+		rte_mempool_free(direct_pool);
+
+	if (indirect_pool)
+		rte_mempool_free(indirect_pool);
+
+	pkt_pool = NULL;
+	direct_pool = NULL;
+	indirect_pool = NULL;
+}
+
+static int ut_setup(void)
+{
+	return TEST_SUCCESS;
+}
+
+static void ut_teardown(void)
+{
+}
+
+static void
+v4_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s, int df,
+		      uint8_t ttl, uint8_t proto, uint16_t pktid)
+{
+	/* Create a packet, 2k bytes long */
+	b->data_off = 0;
+	char *data = rte_pktmbuf_mtod(b, char *);
+
+	memset(data, fill, sizeof(struct rte_ipv4_hdr) + s);
+
+	struct rte_ipv4_hdr *hdr = (struct rte_ipv4_hdr *)data;
+
+	hdr->version_ihl = 0x45; /* standard IP header... */
+	hdr->type_of_service = 0;
+	b->pkt_len = s + sizeof(struct rte_ipv4_hdr);
+	b->data_len = b->pkt_len;
+	hdr->total_length = rte_cpu_to_be_16(b->pkt_len);
+	hdr->packet_id = rte_cpu_to_be_16(pktid);
+	hdr->fragment_offset = 0;
+	if (df)
+		hdr->fragment_offset = rte_cpu_to_be_16(0x4000);
+
+	if (!ttl)
+		ttl = 64; /* default to 64 */
+
+	if (!proto)
+		proto = 1; /* icmp */
+
+	hdr->time_to_live = ttl;
+	hdr->next_proto_id = proto;
+	hdr->hdr_checksum = 0;
+	hdr->src_addr = rte_cpu_to_be_32(0x8080808);
+	hdr->dst_addr = rte_cpu_to_be_32(0x8080404);
+}
+
+static void
+v6_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s, uint8_t ttl,
+		      uint8_t proto, uint16_t pktid)
+{
+	/* Create a packet, 2k bytes long */
+	b->data_off = 0;
+	char *data = rte_pktmbuf_mtod(b, char *);
+
+	memset(data, fill, sizeof(struct rte_ipv6_hdr) + s);
+
+	struct rte_ipv6_hdr *hdr = (struct rte_ipv6_hdr *)data;
+	b->pkt_len = s + sizeof(struct rte_ipv6_hdr);
+	b->data_len = b->pkt_len;
+
+	/* basic v6 header */
+	hdr->vtc_flow = rte_cpu_to_be_32(0x60 << 24 | pktid);
+	hdr->payload_len = rte_cpu_to_be_16(b->pkt_len);
+	hdr->proto = proto;
+	hdr->hop_limits = ttl;
+
+	memset(hdr->src_addr, 0x08, sizeof(hdr->src_addr));
+	memset(hdr->dst_addr, 0x04, sizeof(hdr->src_addr));
+}
+
+static inline void
+test_free_fragments(struct rte_mbuf *mb[], uint32_t num)
+{
+	uint32_t i;
+	for (i = 0; i < num; i++)
+		rte_pktmbuf_free(mb[i]);
+}
+
+static int
+test_ip_frag(void)
+{
+	static const uint16_t RND_ID = UINT16_MAX;
+	int result = TEST_SUCCESS;
+	size_t i;
+
+	struct test_ip_frags {
+		int      ipv;
+		size_t   mtu_size;
+		size_t   pkt_size;
+		int      set_df;
+		uint8_t  ttl;
+		uint8_t  proto;
+		uint16_t pkt_id;
+		int      expected_frags;
+	} tests[] = {
+		     {4, 1280, 1400, 0, 64, IPPROTO_ICMP, RND_ID, 2},
+		     {4, 1280, 1400, 0, 64, IPPROTO_ICMP, 0,      2},
+		     {4,  600, 1400, 0, 64, IPPROTO_ICMP, RND_ID, 3},
+		     {4,    4, 1400, 0, 64, IPPROTO_ICMP, RND_ID, -EINVAL},
+		     {4,  600, 1400, 1, 64, IPPROTO_ICMP, RND_ID, -ENOTSUP},
+		     {4,  600, 1400, 0,  0, IPPROTO_ICMP, RND_ID, 3},
+
+		     {6, 1280, 1400, 0, 64, IPPROTO_ICMP, RND_ID, 2},
+		     {6, 1300, 1400, 0, 64, IPPROTO_ICMP, RND_ID, 2},
+		     {6,    4, 1400, 0, 64, IPPROTO_ICMP, RND_ID, -EINVAL},
+		     {6, 1300, 1400, 0,  0, IPPROTO_ICMP, RND_ID, 2},
+	};
+
+	for (i = 0; i < RTE_DIM(tests); i++) {
+		int32_t len = 0;
+		uint16_t pktid = tests[i].pkt_id;
+		struct rte_mbuf *pkts_out[BURST];
+		struct rte_mbuf *b = rte_pktmbuf_alloc(pkt_pool);
+
+		RTE_TEST_ASSERT_NOT_EQUAL(b, NULL,
+					  "Failed to allocate pkt.");
+
+		if (tests[i].pkt_id == RND_ID)
+			pktid = rte_rand_max(UINT16_MAX);
+
+		if (tests[i].ipv == 4) {
+			v4_allocate_packet_of(b, 0x41414141,
+					      tests[i].pkt_size,
+					      tests[i].set_df,
+					      tests[i].ttl,
+					      tests[i].proto,
+					      pktid);
+		} else if (tests[i].ipv == 6) {
+			v6_allocate_packet_of(b, 0x41414141,
+					      tests[i].pkt_size,
+					      tests[i].ttl,
+					      tests[i].proto,
+					      pktid);
+		}
+
+		if (tests[i].ipv == 4)
+			len = rte_ipv4_fragment_packet(b, pkts_out, BURST,
+						       tests[i].mtu_size,
+						       direct_pool,
+						       indirect_pool);
+		else if (tests[i].ipv == 6)
+			len = rte_ipv6_fragment_packet(b, pkts_out, BURST,
+						       tests[i].mtu_size,
+						       direct_pool,
+						       indirect_pool);
+
+		rte_pktmbuf_free(b);
+
+		if (len > 0)
+			test_free_fragments(pkts_out, len);
+
+		printf("%zd: checking %d with %d\n", i, len,
+		       tests[i].expected_frags);
+		RTE_TEST_ASSERT_EQUAL(len, tests[i].expected_frags,
+				      "Failed case %zd.\n", i);
+
+	}
+
+	return result;
+}
+
+static struct unit_test_suite ipfrag_testsuite  = {
+	.suite_name = "IP Frag Unit Test Suite",
+	.setup = testsuite_setup,
+	.teardown = testsuite_teardown,
+	.unit_test_cases = {
+		TEST_CASE_ST(ut_setup, ut_teardown,
+			     test_ip_frag),
+
+		TEST_CASES_END() /**< NULL terminate unit test array */
+	}
+};
+
+static int
+test_ipfrag(void)
+{
+	rte_log_set_global_level(RTE_LOG_DEBUG);
+	rte_log_set_level(RTE_LOGTYPE_EAL, RTE_LOG_DEBUG);
+
+	return unit_test_suite_runner(&ipfrag_testsuite);
+}
+
+REGISTER_TEST_COMMAND(ipfrag_autotest, test_ipfrag);
-- 
2.25.1

Re: [dpdk-dev] [PATCH v5 3/3] ipfrag: add unit test case

[Lukasz Wojciechowski]

W dniu 17.04.2020 o 15:14, Aaron Conole pisze:
> Initial IP fragmentation unit test.
>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>   MAINTAINERS            |   1 +
>   app/test/meson.build   |   2 +
>   app/test/test_ipfrag.c | 272 +++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 275 insertions(+)
>   create mode 100644 app/test/test_ipfrag.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fe59f0224f..a77c7c17ce 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1228,6 +1228,7 @@ F: app/test/test_crc.c
>   IP fragmentation & reassembly
>   M: Konstantin Ananyev <konstantin.ananyev@intel.com>
>   F: lib/librte_ip_frag/
> +F: app/test/test_ipfrag.c
>   F: doc/guides/prog_guide/ip_fragment_reassembly_lib.rst
>   F: examples/ip_fragmentation/
>   F: doc/guides/sample_app_ug/ip_frag.rst
> diff --git a/app/test/meson.build b/app/test/meson.build
> index 04b59cffa4..4b3c3852a2 100644
> --- a/app/test/meson.build
> +++ b/app/test/meson.build
> @@ -58,6 +58,7 @@ test_sources = files('commands.c',
>   	'test_hash_perf.c',
>   	'test_hash_readwrite_lf_perf.c',
>   	'test_interrupts.c',
> +        'test_ipfrag.c',
>   	'test_ipsec.c',
>   	'test_ipsec_sad.c',
>   	'test_kni.c',
> @@ -187,6 +188,7 @@ fast_tests = [
>           ['flow_classify_autotest', false],
>           ['hash_autotest', true],
>           ['interrupt_autotest', true],
> +        ['ipfrag_autotest', false],
>           ['logs_autotest', true],
>           ['lpm_autotest', true],
>           ['lpm6_autotest', true],
> diff --git a/app/test/test_ipfrag.c b/app/test/test_ipfrag.c
> new file mode 100644
> index 0000000000..6a80581f0e
> --- /dev/null
> +++ b/app/test/test_ipfrag.c
> @@ -0,0 +1,272 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2020 Red Hat, Inc.
> + */
> +
> +#include <time.h>
> +
> +#include <rte_common.h>
> +#include <rte_cycles.h>
> +#include <rte_hexdump.h>
> +#include <rte_ip.h>
> +#include <rte_ip_frag.h>
> +#include <rte_mbuf.h>
> +#include <rte_memcpy.h>
> +#include <rte_random.h>
> +
> +#include "test.h"
> +
> +#define NUM_MBUFS 128
> +#define BURST 32
> +
> +static struct rte_mempool *pkt_pool,
> +			  *direct_pool,
> +			  *indirect_pool;
> +
> +static int
> +setup_buf_pool(void)
> +{
> +	if (!pkt_pool)
> +		pkt_pool = rte_pktmbuf_pool_create("FRAG_MBUF_POOL",
> +						   NUM_MBUFS, BURST, 0,
> +						   RTE_MBUF_DEFAULT_BUF_SIZE,
> +						   SOCKET_ID_ANY);
> +	if (pkt_pool == NULL) {
> +		printf("%s: Error creating pkt mempool\n", __func__);
> +		goto bad_setup;
> +	}
> +
> +	if (!direct_pool)
> +		direct_pool = rte_pktmbuf_pool_create("FRAG_D_MBUF_POOL",
> +						      NUM_MBUFS, BURST, 0,
> +						      RTE_MBUF_DEFAULT_BUF_SIZE,
> +						      SOCKET_ID_ANY);
> +	if (!direct_pool) {
> +		printf("%s: Error creating direct mempool\n", __func__);
> +		goto bad_setup;
> +	}
> +
> +	if (!indirect_pool)
> +		indirect_pool = rte_pktmbuf_pool_create("FRAG_I_MBUF_POOL",
> +							NUM_MBUFS, BURST, 0,
> +							0, SOCKET_ID_ANY);
> +	if (!indirect_pool) {
> +		printf("%s: Error creating indirect mempool\n", __func__);
> +		goto bad_setup;
> +	}
> +
> +	return TEST_SUCCESS;
> +
> +bad_setup:
> +	if (pkt_pool)
> +		rte_mempool_free(pkt_pool);
> +	pkt_pool = NULL;
> +
> +	if (direct_pool)
> +		rte_mempool_free(direct_pool);
> +	direct_pool = NULL;
> +
> +	return TEST_FAILED;
> +}
> +
> +static int testsuite_setup(void)
> +{
> +	return setup_buf_pool();
> +}
> +
> +static void testsuite_teardown(void)
> +{
> +	if (pkt_pool)
> +		rte_mempool_free(pkt_pool);
> +
> +	if (direct_pool)
> +		rte_mempool_free(direct_pool);
> +
> +	if (indirect_pool)
> +		rte_mempool_free(indirect_pool);
> +
> +	pkt_pool = NULL;
> +	direct_pool = NULL;
> +	indirect_pool = NULL;
> +}
> +
> +static int ut_setup(void)
> +{
> +	return TEST_SUCCESS;
> +}
> +
> +static void ut_teardown(void)
> +{
> +}
> +
> +static void
> +v4_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s, int df,
> +		      uint8_t ttl, uint8_t proto, uint16_t pktid)
> +{
> +	/* Create a packet, 2k bytes long */
> +	b->data_off = 0;
> +	char *data = rte_pktmbuf_mtod(b, char *);
> +
> +	memset(data, fill, sizeof(struct rte_ipv4_hdr) + s);
> +
> +	struct rte_ipv4_hdr *hdr = (struct rte_ipv4_hdr *)data;
> +
> +	hdr->version_ihl = 0x45; /* standard IP header... */
> +	hdr->type_of_service = 0;
> +	b->pkt_len = s + sizeof(struct rte_ipv4_hdr);
> +	b->data_len = b->pkt_len;
> +	hdr->total_length = rte_cpu_to_be_16(b->pkt_len);
> +	hdr->packet_id = rte_cpu_to_be_16(pktid);
> +	hdr->fragment_offset = 0;
> +	if (df)
> +		hdr->fragment_offset = rte_cpu_to_be_16(0x4000);
> +
> +	if (!ttl)
> +		ttl = 64; /* default to 64 */
> +
> +	if (!proto)
> +		proto = 1; /* icmp */
> +
> +	hdr->time_to_live = ttl;
> +	hdr->next_proto_id = proto;
> +	hdr->hdr_checksum = 0;
> +	hdr->src_addr = rte_cpu_to_be_32(0x8080808);
> +	hdr->dst_addr = rte_cpu_to_be_32(0x8080404);
> +}
> +
> +static void
> +v6_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s, uint8_t ttl,
> +		      uint8_t proto, uint16_t pktid)
> +{
> +	/* Create a packet, 2k bytes long */
> +	b->data_off = 0;
> +	char *data = rte_pktmbuf_mtod(b, char *);
> +
> +	memset(data, fill, sizeof(struct rte_ipv6_hdr) + s);
> +
> +	struct rte_ipv6_hdr *hdr = (struct rte_ipv6_hdr *)data;
> +	b->pkt_len = s + sizeof(struct rte_ipv6_hdr);
> +	b->data_len = b->pkt_len;
> +
> +	/* basic v6 header */
> +	hdr->vtc_flow = rte_cpu_to_be_32(0x60 << 24 | pktid);
> +	hdr->payload_len = rte_cpu_to_be_16(b->pkt_len);
> +	hdr->proto = proto;
> +	hdr->hop_limits = ttl;
> +
> +	memset(hdr->src_addr, 0x08, sizeof(hdr->src_addr));
> +	memset(hdr->dst_addr, 0x04, sizeof(hdr->src_addr));
> +}
> +
> +static inline void
> +test_free_fragments(struct rte_mbuf *mb[], uint32_t num)
> +{
> +	uint32_t i;
> +	for (i = 0; i < num; i++)
> +		rte_pktmbuf_free(mb[i]);
> +}
> +
> +static int
> +test_ip_frag(void)
> +{
> +	static const uint16_t RND_ID = UINT16_MAX;
> +	int result = TEST_SUCCESS;
> +	size_t i;
> +
> +	struct test_ip_frags {
> +		int      ipv;
> +		size_t   mtu_size;
> +		size_t   pkt_size;
> +		int      set_df;
> +		uint8_t  ttl;
> +		uint8_t  proto;
> +		uint16_t pkt_id;
> +		int      expected_frags;
> +	} tests[] = {
> +		     {4, 1280, 1400, 0, 64, IPPROTO_ICMP, RND_ID, 2},
> +		     {4, 1280, 1400, 0, 64, IPPROTO_ICMP, 0,      2},
> +		     {4,  600, 1400, 0, 64, IPPROTO_ICMP, RND_ID, 3},
> +		     {4,    4, 1400, 0, 64, IPPROTO_ICMP, RND_ID, -EINVAL},
> +		     {4,  600, 1400, 1, 64, IPPROTO_ICMP, RND_ID, -ENOTSUP},
> +		     {4,  600, 1400, 0,  0, IPPROTO_ICMP, RND_ID, 3},
> +
> +		     {6, 1280, 1400, 0, 64, IPPROTO_ICMP, RND_ID, 2},
> +		     {6, 1300, 1400, 0, 64, IPPROTO_ICMP, RND_ID, 2},
> +		     {6,    4, 1400, 0, 64, IPPROTO_ICMP, RND_ID, -EINVAL},
> +		     {6, 1300, 1400, 0,  0, IPPROTO_ICMP, RND_ID, 2},
> +	};
> +
> +	for (i = 0; i < RTE_DIM(tests); i++) {
> +		int32_t len = 0;
> +		uint16_t pktid = tests[i].pkt_id;
> +		struct rte_mbuf *pkts_out[BURST];
> +		struct rte_mbuf *b = rte_pktmbuf_alloc(pkt_pool);
> +
> +		RTE_TEST_ASSERT_NOT_EQUAL(b, NULL,
> +					  "Failed to allocate pkt.");
> +
> +		if (tests[i].pkt_id == RND_ID)
> +			pktid = rte_rand_max(UINT16_MAX);
> +
> +		if (tests[i].ipv == 4) {
> +			v4_allocate_packet_of(b, 0x41414141,
> +					      tests[i].pkt_size,
> +					      tests[i].set_df,
> +					      tests[i].ttl,
> +					      tests[i].proto,
> +					      pktid);
> +		} else if (tests[i].ipv == 6) {
> +			v6_allocate_packet_of(b, 0x41414141,
> +					      tests[i].pkt_size,
> +					      tests[i].ttl,
> +					      tests[i].proto,
> +					      pktid);
> +		}
> +
> +		if (tests[i].ipv == 4)
> +			len = rte_ipv4_fragment_packet(b, pkts_out, BURST,
> +						       tests[i].mtu_size,
> +						       direct_pool,
> +						       indirect_pool);
> +		else if (tests[i].ipv == 6)
> +			len = rte_ipv6_fragment_packet(b, pkts_out, BURST,
> +						       tests[i].mtu_size,
> +						       direct_pool,
> +						       indirect_pool);
> +
> +		rte_pktmbuf_free(b);
> +
> +		if (len > 0)
> +			test_free_fragments(pkts_out, len);
> +
> +		printf("%zd: checking %d with %d\n", i, len,
> +		       tests[i].expected_frags);
> +		RTE_TEST_ASSERT_EQUAL(len, tests[i].expected_frags,
> +				      "Failed case %zd.\n", i);
> +
> +	}
> +
> +	return result;
> +}
> +
> +static struct unit_test_suite ipfrag_testsuite  = {
> +	.suite_name = "IP Frag Unit Test Suite",
> +	.setup = testsuite_setup,
> +	.teardown = testsuite_teardown,
> +	.unit_test_cases = {
> +		TEST_CASE_ST(ut_setup, ut_teardown,
> +			     test_ip_frag),
> +
> +		TEST_CASES_END() /**< NULL terminate unit test array */
> +	}
> +};
> +
> +static int
> +test_ipfrag(void)
> +{
> +	rte_log_set_global_level(RTE_LOG_DEBUG);
> +	rte_log_set_level(RTE_LOGTYPE_EAL, RTE_LOG_DEBUG);
> +
> +	return unit_test_suite_runner(&ipfrag_testsuite);
> +}
> +
> +REGISTER_TEST_COMMAND(ipfrag_autotest, test_ipfrag);

Perfect!

Reviewed-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>

Tested-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>

-- 

Lukasz Wojciechowski
Principal Software Engineer

Samsung R&D Institute Poland
Samsung Electronics
Office +48 22 377 88 25
l.wojciechow@partner.samsung.com

Re: [dpdk-dev] [PATCH v5 1/3] ip_frag: ensure minimum v4 fragmentation length

[Ananyev, Konstantin]

 
> Do a formal parameter check of mtu length, as well as
> checking the the various inputs for validity.  If any
> aren't acceptable, we bail.
> 
> Acked-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>  lib/librte_ip_frag/rte_ipv4_fragmentation.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/lib/librte_ip_frag/rte_ipv4_fragmentation.c b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
> index 9e9f986cc5..c36678a6d2 100644
> --- a/lib/librte_ip_frag/rte_ipv4_fragmentation.c
> +++ b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
> @@ -76,6 +76,15 @@ rte_ipv4_fragment_packet(struct rte_mbuf *pkt_in,
>  	uint16_t fragment_offset, flag_offset, frag_size;
>  	uint16_t frag_bytes_remaining;
> 
> +	/*
> +	 * Formal parameter checking.
> +	 */
> +	if (unlikely(pkt_in == NULL) || unlikely(pkts_out == NULL) ||
> +	    unlikely(nb_pkts_out == 0) ||
> +	    unlikely(pool_direct == NULL) || unlikely(pool_indirect == NULL) ||
> +	    unlikely(mtu_size < 68))

It is better not to use hard-coded constant values.
I think we have a macro for it at lib/librte_net/rte_ether.h:
#define RTE_ETHER_MIN_MTU 68 /**< Minimum MTU for IPv4 packets, see RFC 791. */

Apart from that:
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>


> +		return -EINVAL;
> +
>  	/*
>  	 * Ensure the IP payload length of all fragments is aligned to a
>  	 * multiple of 8 bytes as per RFC791 section 2.3.
> --
> 2.25.1

Re: [dpdk-dev] [PATCH v5 2/3] ip_frag: ensure minimum v6 fragmentation length

[Ananyev, Konstantin]

> 
> In addition, do a formal parameter check.
> 
> Acked-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>  lib/librte_ip_frag/rte_ipv6_fragmentation.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/lib/librte_ip_frag/rte_ipv6_fragmentation.c b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> index 43449970e5..ee984aed82 100644
> --- a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> +++ b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> @@ -79,6 +79,15 @@ rte_ipv6_fragment_packet(struct rte_mbuf *pkt_in,
>  	uint16_t fragment_offset, frag_size;
>  	uint64_t frag_bytes_remaining;
> 
> +	/*
> +	 * Formal parameter checking.
> +	 */
> +	if (unlikely(pkt_in == NULL) || unlikely(pkts_out == NULL) ||
> +	    unlikely(nb_pkts_out == 0) ||
> +	    unlikely(pool_direct == NULL) || unlikely(pool_indirect == NULL) ||
> +	    unlikely(mtu_size < 1280))

Same as for ipv4 - LGTM in general, but please avoid hard-coded constants for MTU values.
Here I couldn't find an existing macro ro min ipv6 mtu, so probably worth to add a new one
in librte_net/.

With that nit fixed:
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> +		return -EINVAL;
> +
>  	/*
>  	 * Ensure the IP payload length of all fragments (except the
>  	 * the last fragment) are a multiple of 8 bytes per RFC2460.
> --
> 2.25.1

Re: [dpdk-dev] [PATCH v5 1/3] ip_frag: ensure minimum v4 fragmentation length

[Aaron Conole]

"Ananyev, Konstantin" <konstantin.ananyev@intel.com> writes:

>  
>> Do a formal parameter check of mtu length, as well as
>> checking the the various inputs for validity.  If any
>> aren't acceptable, we bail.
>> 
>> Acked-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> ---
>>  lib/librte_ip_frag/rte_ipv4_fragmentation.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>> 
>> diff --git a/lib/librte_ip_frag/rte_ipv4_fragmentation.c b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
>> index 9e9f986cc5..c36678a6d2 100644
>> --- a/lib/librte_ip_frag/rte_ipv4_fragmentation.c
>> +++ b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
>> @@ -76,6 +76,15 @@ rte_ipv4_fragment_packet(struct rte_mbuf *pkt_in,
>>  	uint16_t fragment_offset, flag_offset, frag_size;
>>  	uint16_t frag_bytes_remaining;
>> 
>> +	/*
>> +	 * Formal parameter checking.
>> +	 */
>> +	if (unlikely(pkt_in == NULL) || unlikely(pkts_out == NULL) ||
>> +	    unlikely(nb_pkts_out == 0) ||
>> +	    unlikely(pool_direct == NULL) || unlikely(pool_indirect == NULL) ||
>> +	    unlikely(mtu_size < 68))
>
> It is better not to use hard-coded constant values.
> I think we have a macro for it at lib/librte_net/rte_ether.h:
> #define RTE_ETHER_MIN_MTU 68 /**< Minimum MTU for IPv4 packets, see RFC 791. */

Done.

> Apart from that:
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>
>
>> +		return -EINVAL;
>> +
>>  	/*
>>  	 * Ensure the IP payload length of all fragments is aligned to a
>>  	 * multiple of 8 bytes as per RFC791 section 2.3.
>> --
>> 2.25.1

Re: [dpdk-dev] [PATCH v5 2/3] ip_frag: ensure minimum v6 fragmentation length

[Aaron Conole]

"Ananyev, Konstantin" <konstantin.ananyev@intel.com> writes:

>> 
>> In addition, do a formal parameter check.
>> 
>> Acked-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> ---
>>  lib/librte_ip_frag/rte_ipv6_fragmentation.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>> 
>> diff --git a/lib/librte_ip_frag/rte_ipv6_fragmentation.c b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
>> index 43449970e5..ee984aed82 100644
>> --- a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
>> +++ b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
>> @@ -79,6 +79,15 @@ rte_ipv6_fragment_packet(struct rte_mbuf *pkt_in,
>>  	uint16_t fragment_offset, frag_size;
>>  	uint64_t frag_bytes_remaining;
>> 
>> +	/*
>> +	 * Formal parameter checking.
>> +	 */
>> +	if (unlikely(pkt_in == NULL) || unlikely(pkts_out == NULL) ||
>> +	    unlikely(nb_pkts_out == 0) ||
>> +	    unlikely(pool_direct == NULL) || unlikely(pool_indirect == NULL) ||
>> +	    unlikely(mtu_size < 1280))
>
> Same as for ipv4 - LGTM in general, but please avoid hard-coded constants for MTU values.
> Here I couldn't find an existing macro ro min ipv6 mtu, so probably worth to add a new one
> in librte_net/.

I plan to add it in rte_ip.h as RTE_IPV6_MIN_MTU since it seems to fit
there.  I don't think it looks right to add RTE_ETHER_IPV6_MIN_MTU in
the rte_ether.h file (but if you think it looks better I will change to
that).

> With that nit fixed:
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Thanks!

>> +		return -EINVAL;
>> +
>>  	/*
>>  	 * Ensure the IP payload length of all fragments (except the
>>  	 * the last fragment) are a multiple of 8 bytes per RFC2460.
>> --
>> 2.25.1

Re: [dpdk-dev] [PATCH v5 2/3] ip_frag: ensure minimum v6 fragmentation length

[Ananyev, Konstantin]

> >> In addition, do a formal parameter check.
> >>
> >> Acked-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> >> Signed-off-by: Aaron Conole <aconole@redhat.com>
> >> ---
> >>  lib/librte_ip_frag/rte_ipv6_fragmentation.c | 9 +++++++++
> >>  1 file changed, 9 insertions(+)
> >>
> >> diff --git a/lib/librte_ip_frag/rte_ipv6_fragmentation.c b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> >> index 43449970e5..ee984aed82 100644
> >> --- a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> >> +++ b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> >> @@ -79,6 +79,15 @@ rte_ipv6_fragment_packet(struct rte_mbuf *pkt_in,
> >>  	uint16_t fragment_offset, frag_size;
> >>  	uint64_t frag_bytes_remaining;
> >>
> >> +	/*
> >> +	 * Formal parameter checking.
> >> +	 */
> >> +	if (unlikely(pkt_in == NULL) || unlikely(pkts_out == NULL) ||
> >> +	    unlikely(nb_pkts_out == 0) ||
> >> +	    unlikely(pool_direct == NULL) || unlikely(pool_indirect == NULL) ||
> >> +	    unlikely(mtu_size < 1280))
> >
> > Same as for ipv4 - LGTM in general, but please avoid hard-coded constants for MTU values.
> > Here I couldn't find an existing macro ro min ipv6 mtu, so probably worth to add a new one
> > in librte_net/.
> 
> I plan to add it in rte_ip.h as RTE_IPV6_MIN_MTU since it seems to fit
> there.

Sounds ok to me.
Thanks
Konstantin

  I don't think it looks right to add RTE_ETHER_IPV6_MIN_MTU in
> the rte_ether.h file (but if you think it looks better I will change to
> that).
> 
> > With that nit fixed:
> > Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> 
> Thanks!
> 
> >> +		return -EINVAL;
> >> +
> >>  	/*
> >>  	 * Ensure the IP payload length of all fragments (except the
> >>  	 * the last fragment) are a multiple of 8 bytes per RFC2460.
> >> --
> >> 2.25.1

Re: [dpdk-dev] [PATCH v5 3/3] ipfrag: add unit test case

[Burakov, Anatoly]

On 17-Apr-20 2:14 PM, Aaron Conole wrote:
> Initial IP fragmentation unit test.
> 
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---

<snip>

> +	if (!pkt_pool)
> +		pkt_pool = rte_pktmbuf_pool_create("FRAG_MBUF_POOL",
> +						   NUM_MBUFS, BURST, 0,
> +						   RTE_MBUF_DEFAULT_BUF_SIZE,
> +						   SOCKET_ID_ANY);
> +	if (pkt_pool == NULL) {
> +		printf("%s: Error creating pkt mempool\n", __func__);
> +		goto bad_setup;
> +	}
> +
> +	if (!direct_pool)
> +		direct_pool = rte_pktmbuf_pool_create("FRAG_D_MBUF_POOL",
> +						      NUM_MBUFS, BURST, 0,
> +						      RTE_MBUF_DEFAULT_BUF_SIZE,
> +						      SOCKET_ID_ANY);
> +	if (!direct_pool) {
> +		printf("%s: Error creating direct mempool\n", __func__);
> +		goto bad_setup;
> +	}
> +
> +	if (!indirect_pool)
> +		indirect_pool = rte_pktmbuf_pool_create("FRAG_I_MBUF_POOL",
> +							NUM_MBUFS, BURST, 0,
> +							0, SOCKET_ID_ANY);
> +	if (!indirect_pool) {
> +		printf("%s: Error creating indirect mempool\n", __func__);
> +		goto bad_setup;
> +	}

Nitpicking, but i believe the coding style guide discourages using 
boolean syntax for anything other than boolean checks, and it is better 
to use a more explicit `if (x == NULL)`.

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v5 3/3] ipfrag: add unit test case

[Aaron Conole]

"Burakov, Anatoly" <anatoly.burakov@intel.com> writes:

> On 17-Apr-20 2:14 PM, Aaron Conole wrote:
>> Initial IP fragmentation unit test.
>>
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> ---
>
> <snip>
>
>> +	if (!pkt_pool)
>> +		pkt_pool = rte_pktmbuf_pool_create("FRAG_MBUF_POOL",
>> +						   NUM_MBUFS, BURST, 0,
>> +						   RTE_MBUF_DEFAULT_BUF_SIZE,
>> +						   SOCKET_ID_ANY);
>> +	if (pkt_pool == NULL) {
>> +		printf("%s: Error creating pkt mempool\n", __func__);
>> +		goto bad_setup;
>> +	}
>> +
>> +	if (!direct_pool)
>> +		direct_pool = rte_pktmbuf_pool_create("FRAG_D_MBUF_POOL",
>> +						      NUM_MBUFS, BURST, 0,
>> +						      RTE_MBUF_DEFAULT_BUF_SIZE,
>> +						      SOCKET_ID_ANY);
>> +	if (!direct_pool) {
>> +		printf("%s: Error creating direct mempool\n", __func__);
>> +		goto bad_setup;
>> +	}
>> +
>> +	if (!indirect_pool)
>> +		indirect_pool = rte_pktmbuf_pool_create("FRAG_I_MBUF_POOL",
>> +							NUM_MBUFS, BURST, 0,
>> +							0, SOCKET_ID_ANY);
>> +	if (!indirect_pool) {
>> +		printf("%s: Error creating indirect mempool\n", __func__);
>> +		goto bad_setup;
>> +	}
>
> Nitpicking, but i believe the coding style guide discourages using
> boolean syntax for anything other than boolean checks, and it is
> better to use a more explicit `if (x == NULL)`.

I see, it does.  Looking at the code-base, I see it mixed all over, some
places using 'if (!ptr)' and others 'if (ptr == NULL)'. Actually, even
in the flow_filtering.rst doc, it implies that if (!ptr) is acceptable.

Since I'm spinning a v6 with the constants, I'll fold this change in -
maybe it makes sense to clean it up everywhere to help mitigate the
confusion (for example, I most recently did work in the eal and the !ptr
is all over there).  WDYT?

[dpdk-dev] [PATCH v6 0/3] ip_frag: add a unit test for fragmentation

[Aaron Conole]

This adds a simple unit test for the ip fragmentation library
and covers fragmenting ipv4 and ipv6.  Additionally, some fixes
are introduced, which are really just sanity rather than real
issues in the field.

v5->v6:
- Coding style fixes
- Use named defines instead of bare MTU decimal values.

v4->v5:
- Address coding style comments in patch 3/3

v3->v4:
- Drop the frag header detection from ipv6 code
- Full parameter sanity checking in ipv4 and ipv6 code
- Convert from htons/htonl to rte_cpu_to_be_...

v2->v3:
- Remove c99-ism from 4/4

v1->v2:
- Fix patch 4/4 which had a missing assignment for pktid.


Aaron Conole (3):
  ip_frag: ensure minimum v4 fragmentation length
  ip_frag: ensure minimum v6 fragmentation length
  ipfrag: add unit test case

 MAINTAINERS                                 |   1 +
 app/test/meson.build                        |   2 +
 app/test/test_ipfrag.c                      | 262 ++++++++++++++++++++
 lib/librte_ip_frag/rte_ipv4_fragmentation.c |  10 +
 lib/librte_ip_frag/rte_ipv6_fragmentation.c |   9 +
 lib/librte_net/rte_ip.h                     |   2 +
 6 files changed, 286 insertions(+)
 create mode 100644 app/test/test_ipfrag.c

-- 
2.25.1

[dpdk-dev] [PATCH v6 1/3] ip_frag: ensure minimum v4 fragmentation length

[Aaron Conole]

Do a formal parameter check of mtu length, as well as
checking the the various inputs for validity.  If any
aren't acceptable, we bail.

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
Acked-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 lib/librte_ip_frag/rte_ipv4_fragmentation.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/lib/librte_ip_frag/rte_ipv4_fragmentation.c b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
index 9e9f986cc5..e9de335ae2 100644
--- a/lib/librte_ip_frag/rte_ipv4_fragmentation.c
+++ b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
@@ -8,6 +8,7 @@
 #include <rte_memcpy.h>
 #include <rte_mempool.h>
 #include <rte_debug.h>
+#include <rte_ether.h>
 
 #include "ip_frag_common.h"
 
@@ -76,6 +77,15 @@ rte_ipv4_fragment_packet(struct rte_mbuf *pkt_in,
 	uint16_t fragment_offset, flag_offset, frag_size;
 	uint16_t frag_bytes_remaining;
 
+	/*
+	 * Formal parameter checking.
+	 */
+	if (unlikely(pkt_in == NULL) || unlikely(pkts_out == NULL) ||
+	    unlikely(nb_pkts_out == 0) ||
+	    unlikely(pool_direct == NULL) || unlikely(pool_indirect == NULL) ||
+	    unlikely(mtu_size < RTE_ETHER_MIN_MTU))
+		return -EINVAL;
+
 	/*
 	 * Ensure the IP payload length of all fragments is aligned to a
 	 * multiple of 8 bytes as per RFC791 section 2.3.
-- 
2.25.1

[dpdk-dev] [PATCH v6 2/3] ip_frag: ensure minimum v6 fragmentation length

[Aaron Conole]

In addition, do a formal parameter check.

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
Acked-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 lib/librte_ip_frag/rte_ipv6_fragmentation.c | 9 +++++++++
 lib/librte_net/rte_ip.h                     | 2 ++
 2 files changed, 11 insertions(+)

diff --git a/lib/librte_ip_frag/rte_ipv6_fragmentation.c b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
index 43449970e5..5d67336f2d 100644
--- a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
+++ b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
@@ -79,6 +79,15 @@ rte_ipv6_fragment_packet(struct rte_mbuf *pkt_in,
 	uint16_t fragment_offset, frag_size;
 	uint64_t frag_bytes_remaining;
 
+	/*
+	 * Formal parameter checking.
+	 */
+	if (unlikely(pkt_in == NULL) || unlikely(pkts_out == NULL) ||
+	    unlikely(nb_pkts_out == 0) ||
+	    unlikely(pool_direct == NULL) || unlikely(pool_indirect == NULL) ||
+	    unlikely(mtu_size < RTE_IPV6_MIN_MTU))
+		return -EINVAL;
+
 	/*
 	 * Ensure the IP payload length of all fragments (except the
 	 * the last fragment) are a multiple of 8 bytes per RFC2460.
diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
index 1ceb7b7931..d50edec572 100644
--- a/lib/librte_net/rte_ip.h
+++ b/lib/librte_net/rte_ip.h
@@ -371,6 +371,8 @@ struct rte_ipv6_hdr {
 #define RTE_IPV6_HDR_ECN_MASK	(0x03 << RTE_IPV6_HDR_TC_SHIFT)
 #define RTE_IPV6_HDR_ECN_CE	RTE_IPV6_HDR_ECN_MASK
 
+#define RTE_IPV6_MIN_MTU 1280 /**< Minimum MTU for IPv6, see RFC 8200. */
+
 /**
  * Process the pseudo-header checksum of an IPv6 header.
  *
-- 
2.25.1

[dpdk-dev] [PATCH v6 3/3] ipfrag: add unit test case

[Aaron Conole]

Initial IP fragmentation unit test.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 MAINTAINERS            |   1 +
 app/test/meson.build   |   2 +
 app/test/test_ipfrag.c | 262 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 265 insertions(+)
 create mode 100644 app/test/test_ipfrag.c

diff --git a/MAINTAINERS b/MAINTAINERS
index fe59f0224f..a77c7c17ce 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1228,6 +1228,7 @@ F: app/test/test_crc.c
 IP fragmentation & reassembly
 M: Konstantin Ananyev <konstantin.ananyev@intel.com>
 F: lib/librte_ip_frag/
+F: app/test/test_ipfrag.c
 F: doc/guides/prog_guide/ip_fragment_reassembly_lib.rst
 F: examples/ip_fragmentation/
 F: doc/guides/sample_app_ug/ip_frag.rst
diff --git a/app/test/meson.build b/app/test/meson.build
index 04b59cffa4..4b3c3852a2 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -58,6 +58,7 @@ test_sources = files('commands.c',
 	'test_hash_perf.c',
 	'test_hash_readwrite_lf_perf.c',
 	'test_interrupts.c',
+        'test_ipfrag.c',
 	'test_ipsec.c',
 	'test_ipsec_sad.c',
 	'test_kni.c',
@@ -187,6 +188,7 @@ fast_tests = [
         ['flow_classify_autotest', false],
         ['hash_autotest', true],
         ['interrupt_autotest', true],
+        ['ipfrag_autotest', false],
         ['logs_autotest', true],
         ['lpm_autotest', true],
         ['lpm6_autotest', true],
diff --git a/app/test/test_ipfrag.c b/app/test/test_ipfrag.c
new file mode 100644
index 0000000000..da8c212f92
--- /dev/null
+++ b/app/test/test_ipfrag.c
@@ -0,0 +1,262 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2020 Red Hat, Inc.
+ */
+
+#include <time.h>
+
+#include <rte_common.h>
+#include <rte_cycles.h>
+#include <rte_hexdump.h>
+#include <rte_ip.h>
+#include <rte_ip_frag.h>
+#include <rte_mbuf.h>
+#include <rte_memcpy.h>
+#include <rte_random.h>
+
+#include "test.h"
+
+#define NUM_MBUFS 128
+#define BURST 32
+
+static struct rte_mempool *pkt_pool,
+			  *direct_pool,
+			  *indirect_pool;
+
+static int
+setup_buf_pool(void)
+{
+	pkt_pool = rte_pktmbuf_pool_create("FRAG_MBUF_POOL",
+					   NUM_MBUFS, BURST, 0,
+					   RTE_MBUF_DEFAULT_BUF_SIZE,
+					   SOCKET_ID_ANY);
+	if (pkt_pool == NULL) {
+		printf("%s: Error creating pkt mempool\n", __func__);
+		goto bad_setup;
+	}
+
+	direct_pool = rte_pktmbuf_pool_create("FRAG_D_MBUF_POOL",
+					      NUM_MBUFS, BURST, 0,
+					      RTE_MBUF_DEFAULT_BUF_SIZE,
+					      SOCKET_ID_ANY);
+	if (direct_pool == NULL) {
+		printf("%s: Error creating direct mempool\n", __func__);
+		goto bad_setup;
+	}
+
+	indirect_pool = rte_pktmbuf_pool_create("FRAG_I_MBUF_POOL",
+						NUM_MBUFS, BURST, 0,
+						0, SOCKET_ID_ANY);
+	if (indirect_pool == NULL) {
+		printf("%s: Error creating indirect mempool\n", __func__);
+		goto bad_setup;
+	}
+
+	return TEST_SUCCESS;
+
+bad_setup:
+	rte_mempool_free(pkt_pool);
+	pkt_pool = NULL;
+
+	rte_mempool_free(direct_pool);
+	direct_pool = NULL;
+
+	return TEST_FAILED;
+}
+
+static int testsuite_setup(void)
+{
+	return setup_buf_pool();
+}
+
+static void testsuite_teardown(void)
+{
+	rte_mempool_free(pkt_pool);
+	rte_mempool_free(direct_pool);
+	rte_mempool_free(indirect_pool);
+
+	pkt_pool = NULL;
+	direct_pool = NULL;
+	indirect_pool = NULL;
+}
+
+static int ut_setup(void)
+{
+	return TEST_SUCCESS;
+}
+
+static void ut_teardown(void)
+{
+}
+
+static void
+v4_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s, int df,
+		      uint8_t ttl, uint8_t proto, uint16_t pktid)
+{
+	/* Create a packet, 2k bytes long */
+	b->data_off = 0;
+	char *data = rte_pktmbuf_mtod(b, char *);
+
+	memset(data, fill, sizeof(struct rte_ipv4_hdr) + s);
+
+	struct rte_ipv4_hdr *hdr = (struct rte_ipv4_hdr *)data;
+
+	hdr->version_ihl = 0x45; /* standard IP header... */
+	hdr->type_of_service = 0;
+	b->pkt_len = s + sizeof(struct rte_ipv4_hdr);
+	b->data_len = b->pkt_len;
+	hdr->total_length = rte_cpu_to_be_16(b->pkt_len);
+	hdr->packet_id = rte_cpu_to_be_16(pktid);
+	hdr->fragment_offset = 0;
+	if (df)
+		hdr->fragment_offset = rte_cpu_to_be_16(0x4000);
+
+	if (!ttl)
+		ttl = 64; /* default to 64 */
+
+	if (!proto)
+		proto = 1; /* icmp */
+
+	hdr->time_to_live = ttl;
+	hdr->next_proto_id = proto;
+	hdr->hdr_checksum = 0;
+	hdr->src_addr = rte_cpu_to_be_32(0x8080808);
+	hdr->dst_addr = rte_cpu_to_be_32(0x8080404);
+}
+
+static void
+v6_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s, uint8_t ttl,
+		      uint8_t proto, uint16_t pktid)
+{
+	/* Create a packet, 2k bytes long */
+	b->data_off = 0;
+	char *data = rte_pktmbuf_mtod(b, char *);
+
+	memset(data, fill, sizeof(struct rte_ipv6_hdr) + s);
+
+	struct rte_ipv6_hdr *hdr = (struct rte_ipv6_hdr *)data;
+	b->pkt_len = s + sizeof(struct rte_ipv6_hdr);
+	b->data_len = b->pkt_len;
+
+	/* basic v6 header */
+	hdr->vtc_flow = rte_cpu_to_be_32(0x60 << 24 | pktid);
+	hdr->payload_len = rte_cpu_to_be_16(b->pkt_len);
+	hdr->proto = proto;
+	hdr->hop_limits = ttl;
+
+	memset(hdr->src_addr, 0x08, sizeof(hdr->src_addr));
+	memset(hdr->dst_addr, 0x04, sizeof(hdr->src_addr));
+}
+
+static inline void
+test_free_fragments(struct rte_mbuf *mb[], uint32_t num)
+{
+	uint32_t i;
+	for (i = 0; i < num; i++)
+		rte_pktmbuf_free(mb[i]);
+}
+
+static int
+test_ip_frag(void)
+{
+	static const uint16_t RND_ID = UINT16_MAX;
+	int result = TEST_SUCCESS;
+	size_t i;
+
+	struct test_ip_frags {
+		int      ipv;
+		size_t   mtu_size;
+		size_t   pkt_size;
+		int      set_df;
+		uint8_t  ttl;
+		uint8_t  proto;
+		uint16_t pkt_id;
+		int      expected_frags;
+	} tests[] = {
+		     {4, 1280, 1400, 0, 64, IPPROTO_ICMP, RND_ID, 2},
+		     {4, 1280, 1400, 0, 64, IPPROTO_ICMP, 0,      2},
+		     {4,  600, 1400, 0, 64, IPPROTO_ICMP, RND_ID, 3},
+		     {4,    4, 1400, 0, 64, IPPROTO_ICMP, RND_ID, -EINVAL},
+		     {4,  600, 1400, 1, 64, IPPROTO_ICMP, RND_ID, -ENOTSUP},
+		     {4,  600, 1400, 0,  0, IPPROTO_ICMP, RND_ID, 3},
+
+		     {6, 1280, 1400, 0, 64, IPPROTO_ICMP, RND_ID, 2},
+		     {6, 1300, 1400, 0, 64, IPPROTO_ICMP, RND_ID, 2},
+		     {6,    4, 1400, 0, 64, IPPROTO_ICMP, RND_ID, -EINVAL},
+		     {6, 1300, 1400, 0,  0, IPPROTO_ICMP, RND_ID, 2},
+	};
+
+	for (i = 0; i < RTE_DIM(tests); i++) {
+		int32_t len = 0;
+		uint16_t pktid = tests[i].pkt_id;
+		struct rte_mbuf *pkts_out[BURST];
+		struct rte_mbuf *b = rte_pktmbuf_alloc(pkt_pool);
+
+		RTE_TEST_ASSERT_NOT_EQUAL(b, NULL,
+					  "Failed to allocate pkt.");
+
+		if (tests[i].pkt_id == RND_ID)
+			pktid = rte_rand_max(UINT16_MAX);
+
+		if (tests[i].ipv == 4) {
+			v4_allocate_packet_of(b, 0x41414141,
+					      tests[i].pkt_size,
+					      tests[i].set_df,
+					      tests[i].ttl,
+					      tests[i].proto,
+					      pktid);
+		} else if (tests[i].ipv == 6) {
+			v6_allocate_packet_of(b, 0x41414141,
+					      tests[i].pkt_size,
+					      tests[i].ttl,
+					      tests[i].proto,
+					      pktid);
+		}
+
+		if (tests[i].ipv == 4)
+			len = rte_ipv4_fragment_packet(b, pkts_out, BURST,
+						       tests[i].mtu_size,
+						       direct_pool,
+						       indirect_pool);
+		else if (tests[i].ipv == 6)
+			len = rte_ipv6_fragment_packet(b, pkts_out, BURST,
+						       tests[i].mtu_size,
+						       direct_pool,
+						       indirect_pool);
+
+		rte_pktmbuf_free(b);
+
+		if (len > 0)
+			test_free_fragments(pkts_out, len);
+
+		printf("%zd: checking %d with %d\n", i, len,
+		       tests[i].expected_frags);
+		RTE_TEST_ASSERT_EQUAL(len, tests[i].expected_frags,
+				      "Failed case %zd.\n", i);
+
+	}
+
+	return result;
+}
+
+static struct unit_test_suite ipfrag_testsuite  = {
+	.suite_name = "IP Frag Unit Test Suite",
+	.setup = testsuite_setup,
+	.teardown = testsuite_teardown,
+	.unit_test_cases = {
+		TEST_CASE_ST(ut_setup, ut_teardown,
+			     test_ip_frag),
+
+		TEST_CASES_END() /**< NULL terminate unit test array */
+	}
+};
+
+static int
+test_ipfrag(void)
+{
+	rte_log_set_global_level(RTE_LOG_DEBUG);
+	rte_log_set_level(RTE_LOGTYPE_EAL, RTE_LOG_DEBUG);
+
+	return unit_test_suite_runner(&ipfrag_testsuite);
+}
+
+REGISTER_TEST_COMMAND(ipfrag_autotest, test_ipfrag);
-- 
2.25.1

Re: [dpdk-dev] [PATCH v6 3/3] ipfrag: add unit test case

[Lukasz Wojciechowski]

W dniu 20.04.2020 o 21:25, Aaron Conole pisze:
> Initial IP fragmentation unit test.
>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>   MAINTAINERS            |   1 +
>   app/test/meson.build   |   2 +
>   app/test/test_ipfrag.c | 262 +++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 265 insertions(+)
>   create mode 100644 app/test/test_ipfrag.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fe59f0224f..a77c7c17ce 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1228,6 +1228,7 @@ F: app/test/test_crc.c
>   IP fragmentation & reassembly
>   M: Konstantin Ananyev <konstantin.ananyev@intel.com>
>   F: lib/librte_ip_frag/
> +F: app/test/test_ipfrag.c
>   F: doc/guides/prog_guide/ip_fragment_reassembly_lib.rst
>   F: examples/ip_fragmentation/
>   F: doc/guides/sample_app_ug/ip_frag.rst
> diff --git a/app/test/meson.build b/app/test/meson.build
> index 04b59cffa4..4b3c3852a2 100644
> --- a/app/test/meson.build
> +++ b/app/test/meson.build
> @@ -58,6 +58,7 @@ test_sources = files('commands.c',
>   	'test_hash_perf.c',
>   	'test_hash_readwrite_lf_perf.c',
>   	'test_interrupts.c',
> +        'test_ipfrag.c',
>   	'test_ipsec.c',
>   	'test_ipsec_sad.c',
>   	'test_kni.c',
> @@ -187,6 +188,7 @@ fast_tests = [
>           ['flow_classify_autotest', false],
>           ['hash_autotest', true],
>           ['interrupt_autotest', true],
> +        ['ipfrag_autotest', false],
>           ['logs_autotest', true],
>           ['lpm_autotest', true],
>           ['lpm6_autotest', true],
> diff --git a/app/test/test_ipfrag.c b/app/test/test_ipfrag.c
> new file mode 100644
> index 0000000000..da8c212f92
> --- /dev/null
> +++ b/app/test/test_ipfrag.c
> @@ -0,0 +1,262 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2020 Red Hat, Inc.
> + */
> +
> +#include <time.h>
> +
> +#include <rte_common.h>
> +#include <rte_cycles.h>
> +#include <rte_hexdump.h>
> +#include <rte_ip.h>
> +#include <rte_ip_frag.h>
> +#include <rte_mbuf.h>
> +#include <rte_memcpy.h>
> +#include <rte_random.h>
> +
> +#include "test.h"
> +
> +#define NUM_MBUFS 128
> +#define BURST 32
> +
> +static struct rte_mempool *pkt_pool,
> +			  *direct_pool,
> +			  *indirect_pool;
> +
> +static int
> +setup_buf_pool(void)
> +{
> +	pkt_pool = rte_pktmbuf_pool_create("FRAG_MBUF_POOL",
> +					   NUM_MBUFS, BURST, 0,
> +					   RTE_MBUF_DEFAULT_BUF_SIZE,
> +					   SOCKET_ID_ANY);
> +	if (pkt_pool == NULL) {
> +		printf("%s: Error creating pkt mempool\n", __func__);
> +		goto bad_setup;
> +	}
> +
> +	direct_pool = rte_pktmbuf_pool_create("FRAG_D_MBUF_POOL",
> +					      NUM_MBUFS, BURST, 0,
> +					      RTE_MBUF_DEFAULT_BUF_SIZE,
> +					      SOCKET_ID_ANY);
> +	if (direct_pool == NULL) {
> +		printf("%s: Error creating direct mempool\n", __func__);
> +		goto bad_setup;
> +	}
> +
> +	indirect_pool = rte_pktmbuf_pool_create("FRAG_I_MBUF_POOL",
> +						NUM_MBUFS, BURST, 0,
> +						0, SOCKET_ID_ANY);
> +	if (indirect_pool == NULL) {
> +		printf("%s: Error creating indirect mempool\n", __func__);
> +		goto bad_setup;
> +	}
> +
> +	return TEST_SUCCESS;
> +
> +bad_setup:
> +	rte_mempool_free(pkt_pool);
> +	pkt_pool = NULL;
> +
> +	rte_mempool_free(direct_pool);
> +	direct_pool = NULL;
> +
> +	return TEST_FAILED;
> +}
> +
> +static int testsuite_setup(void)
> +{
> +	return setup_buf_pool();
> +}
> +
> +static void testsuite_teardown(void)
> +{
> +	rte_mempool_free(pkt_pool);
> +	rte_mempool_free(direct_pool);
> +	rte_mempool_free(indirect_pool);
> +
> +	pkt_pool = NULL;
> +	direct_pool = NULL;
> +	indirect_pool = NULL;
> +}
> +
> +static int ut_setup(void)
> +{
> +	return TEST_SUCCESS;
> +}
> +
> +static void ut_teardown(void)
> +{
> +}
> +
> +static void
> +v4_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s, int df,
> +		      uint8_t ttl, uint8_t proto, uint16_t pktid)
> +{
> +	/* Create a packet, 2k bytes long */
> +	b->data_off = 0;
> +	char *data = rte_pktmbuf_mtod(b, char *);
> +
> +	memset(data, fill, sizeof(struct rte_ipv4_hdr) + s);
> +
> +	struct rte_ipv4_hdr *hdr = (struct rte_ipv4_hdr *)data;
> +
> +	hdr->version_ihl = 0x45; /* standard IP header... */
> +	hdr->type_of_service = 0;
> +	b->pkt_len = s + sizeof(struct rte_ipv4_hdr);
> +	b->data_len = b->pkt_len;
> +	hdr->total_length = rte_cpu_to_be_16(b->pkt_len);
> +	hdr->packet_id = rte_cpu_to_be_16(pktid);
> +	hdr->fragment_offset = 0;
> +	if (df)
> +		hdr->fragment_offset = rte_cpu_to_be_16(0x4000);
> +
> +	if (!ttl)
> +		ttl = 64; /* default to 64 */
> +
> +	if (!proto)
> +		proto = 1; /* icmp */
> +
> +	hdr->time_to_live = ttl;
> +	hdr->next_proto_id = proto;
> +	hdr->hdr_checksum = 0;
> +	hdr->src_addr = rte_cpu_to_be_32(0x8080808);
> +	hdr->dst_addr = rte_cpu_to_be_32(0x8080404);
> +}
> +
> +static void
> +v6_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s, uint8_t ttl,
> +		      uint8_t proto, uint16_t pktid)
> +{
> +	/* Create a packet, 2k bytes long */
> +	b->data_off = 0;
> +	char *data = rte_pktmbuf_mtod(b, char *);
> +
> +	memset(data, fill, sizeof(struct rte_ipv6_hdr) + s);
> +
> +	struct rte_ipv6_hdr *hdr = (struct rte_ipv6_hdr *)data;
> +	b->pkt_len = s + sizeof(struct rte_ipv6_hdr);
> +	b->data_len = b->pkt_len;
> +
> +	/* basic v6 header */
> +	hdr->vtc_flow = rte_cpu_to_be_32(0x60 << 24 | pktid);
> +	hdr->payload_len = rte_cpu_to_be_16(b->pkt_len);
> +	hdr->proto = proto;
> +	hdr->hop_limits = ttl;
> +
> +	memset(hdr->src_addr, 0x08, sizeof(hdr->src_addr));
> +	memset(hdr->dst_addr, 0x04, sizeof(hdr->src_addr));
> +}
> +
> +static inline void
> +test_free_fragments(struct rte_mbuf *mb[], uint32_t num)
> +{
> +	uint32_t i;
> +	for (i = 0; i < num; i++)
> +		rte_pktmbuf_free(mb[i]);
> +}
> +
> +static int
> +test_ip_frag(void)
> +{
> +	static const uint16_t RND_ID = UINT16_MAX;
> +	int result = TEST_SUCCESS;
> +	size_t i;
> +
> +	struct test_ip_frags {
> +		int      ipv;
> +		size_t   mtu_size;
> +		size_t   pkt_size;
> +		int      set_df;
> +		uint8_t  ttl;
> +		uint8_t  proto;
> +		uint16_t pkt_id;
> +		int      expected_frags;
> +	} tests[] = {
> +		     {4, 1280, 1400, 0, 64, IPPROTO_ICMP, RND_ID, 2},
> +		     {4, 1280, 1400, 0, 64, IPPROTO_ICMP, 0,      2},
> +		     {4,  600, 1400, 0, 64, IPPROTO_ICMP, RND_ID, 3},
> +		     {4,    4, 1400, 0, 64, IPPROTO_ICMP, RND_ID, -EINVAL},
> +		     {4,  600, 1400, 1, 64, IPPROTO_ICMP, RND_ID, -ENOTSUP},
> +		     {4,  600, 1400, 0,  0, IPPROTO_ICMP, RND_ID, 3},
> +
> +		     {6, 1280, 1400, 0, 64, IPPROTO_ICMP, RND_ID, 2},
> +		     {6, 1300, 1400, 0, 64, IPPROTO_ICMP, RND_ID, 2},
> +		     {6,    4, 1400, 0, 64, IPPROTO_ICMP, RND_ID, -EINVAL},
> +		     {6, 1300, 1400, 0,  0, IPPROTO_ICMP, RND_ID, 2},
> +	};
> +
> +	for (i = 0; i < RTE_DIM(tests); i++) {
> +		int32_t len = 0;
> +		uint16_t pktid = tests[i].pkt_id;
> +		struct rte_mbuf *pkts_out[BURST];
> +		struct rte_mbuf *b = rte_pktmbuf_alloc(pkt_pool);
> +
> +		RTE_TEST_ASSERT_NOT_EQUAL(b, NULL,
> +					  "Failed to allocate pkt.");
> +
> +		if (tests[i].pkt_id == RND_ID)
> +			pktid = rte_rand_max(UINT16_MAX);
> +
> +		if (tests[i].ipv == 4) {
> +			v4_allocate_packet_of(b, 0x41414141,
> +					      tests[i].pkt_size,
> +					      tests[i].set_df,
> +					      tests[i].ttl,
> +					      tests[i].proto,
> +					      pktid);
> +		} else if (tests[i].ipv == 6) {
> +			v6_allocate_packet_of(b, 0x41414141,
> +					      tests[i].pkt_size,
> +					      tests[i].ttl,
> +					      tests[i].proto,
> +					      pktid);
> +		}
> +
> +		if (tests[i].ipv == 4)
> +			len = rte_ipv4_fragment_packet(b, pkts_out, BURST,
> +						       tests[i].mtu_size,
> +						       direct_pool,
> +						       indirect_pool);
> +		else if (tests[i].ipv == 6)
> +			len = rte_ipv6_fragment_packet(b, pkts_out, BURST,
> +						       tests[i].mtu_size,
> +						       direct_pool,
> +						       indirect_pool);
> +
> +		rte_pktmbuf_free(b);
> +
> +		if (len > 0)
> +			test_free_fragments(pkts_out, len);
> +
> +		printf("%zd: checking %d with %d\n", i, len,
> +		       tests[i].expected_frags);
> +		RTE_TEST_ASSERT_EQUAL(len, tests[i].expected_frags,
> +				      "Failed case %zd.\n", i);
> +
> +	}
> +
> +	return result;
> +}
> +
> +static struct unit_test_suite ipfrag_testsuite  = {
> +	.suite_name = "IP Frag Unit Test Suite",
> +	.setup = testsuite_setup,
> +	.teardown = testsuite_teardown,
> +	.unit_test_cases = {
> +		TEST_CASE_ST(ut_setup, ut_teardown,
> +			     test_ip_frag),
> +
> +		TEST_CASES_END() /**< NULL terminate unit test array */
> +	}
> +};
> +
> +static int
> +test_ipfrag(void)
> +{
> +	rte_log_set_global_level(RTE_LOG_DEBUG);
> +	rte_log_set_level(RTE_LOGTYPE_EAL, RTE_LOG_DEBUG);
> +
> +	return unit_test_suite_runner(&ipfrag_testsuite);
> +}
> +
> +REGISTER_TEST_COMMAND(ipfrag_autotest, test_ipfrag);

Stiil works and lloks good to me.

Tested-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
Reviewed-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>

-- 

Lukasz Wojciechowski
Principal Software Engineer

Samsung R&D Institute Poland
Samsung Electronics
Office +48 22 377 88 25
l.wojciechow@partner.samsung.com

Re: [dpdk-dev] [PATCH v6 1/3] ip_frag: ensure minimum v4 fragmentation length

[Lukasz Wojciechowski]

W dniu 20.04.2020 o 21:25, Aaron Conole pisze:
> Do a formal parameter check of mtu length, as well as
> checking the the various inputs for validity.  If any
> aren't acceptable, we bail.
>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> Acked-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>   lib/librte_ip_frag/rte_ipv4_fragmentation.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/lib/librte_ip_frag/rte_ipv4_fragmentation.c b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
> index 9e9f986cc5..e9de335ae2 100644
> --- a/lib/librte_ip_frag/rte_ipv4_fragmentation.c
> +++ b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
> @@ -8,6 +8,7 @@
>   #include <rte_memcpy.h>
>   #include <rte_mempool.h>
>   #include <rte_debug.h>
> +#include <rte_ether.h>
>   
>   #include "ip_frag_common.h"
>   
> @@ -76,6 +77,15 @@ rte_ipv4_fragment_packet(struct rte_mbuf *pkt_in,
>   	uint16_t fragment_offset, flag_offset, frag_size;
>   	uint16_t frag_bytes_remaining;
>   
> +	/*
> +	 * Formal parameter checking.
> +	 */
> +	if (unlikely(pkt_in == NULL) || unlikely(pkts_out == NULL) ||
> +	    unlikely(nb_pkts_out == 0) ||
> +	    unlikely(pool_direct == NULL) || unlikely(pool_indirect == NULL) ||
> +	    unlikely(mtu_size < RTE_ETHER_MIN_MTU))
> +		return -EINVAL;
> +
>   	/*
>   	 * Ensure the IP payload length of all fragments is aligned to a
>   	 * multiple of 8 bytes as per RFC791 section 2.3.
Acked-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>

-- 

Lukasz Wojciechowski
Principal Software Engineer

Samsung R&D Institute Poland
Samsung Electronics
Office +48 22 377 88 25
l.wojciechow@partner.samsung.com

Re: [dpdk-dev] [PATCH v6 2/3] ip_frag: ensure minimum v6 fragmentation length

[Lukasz Wojciechowski]

W dniu 20.04.2020 o 21:25, Aaron Conole pisze:
> In addition, do a formal parameter check.
>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> Acked-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>   lib/librte_ip_frag/rte_ipv6_fragmentation.c | 9 +++++++++
>   lib/librte_net/rte_ip.h                     | 2 ++
>   2 files changed, 11 insertions(+)
>
> diff --git a/lib/librte_ip_frag/rte_ipv6_fragmentation.c b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> index 43449970e5..5d67336f2d 100644
> --- a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> +++ b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> @@ -79,6 +79,15 @@ rte_ipv6_fragment_packet(struct rte_mbuf *pkt_in,
>   	uint16_t fragment_offset, frag_size;
>   	uint64_t frag_bytes_remaining;
>   
> +	/*
> +	 * Formal parameter checking.
> +	 */
> +	if (unlikely(pkt_in == NULL) || unlikely(pkts_out == NULL) ||
> +	    unlikely(nb_pkts_out == 0) ||
> +	    unlikely(pool_direct == NULL) || unlikely(pool_indirect == NULL) ||
> +	    unlikely(mtu_size < RTE_IPV6_MIN_MTU))
> +		return -EINVAL;
> +
>   	/*
>   	 * Ensure the IP payload length of all fragments (except the
>   	 * the last fragment) are a multiple of 8 bytes per RFC2460.
> diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
> index 1ceb7b7931..d50edec572 100644
> --- a/lib/librte_net/rte_ip.h
> +++ b/lib/librte_net/rte_ip.h
> @@ -371,6 +371,8 @@ struct rte_ipv6_hdr {
>   #define RTE_IPV6_HDR_ECN_MASK	(0x03 << RTE_IPV6_HDR_TC_SHIFT)
>   #define RTE_IPV6_HDR_ECN_CE	RTE_IPV6_HDR_ECN_MASK
>   
> +#define RTE_IPV6_MIN_MTU 1280 /**< Minimum MTU for IPv6, see RFC 8200. */
> +
>   /**
>    * Process the pseudo-header checksum of an IPv6 header.
>    *
Acked-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>

-- 

Lukasz Wojciechowski
Principal Software Engineer

Samsung R&D Institute Poland
Samsung Electronics
Office +48 22 377 88 25
l.wojciechow@partner.samsung.com

Re: [dpdk-dev] [PATCH v5 3/3] ipfrag: add unit test case

[Thomas Monjalon]

20/04/2020 19:34, Aaron Conole:
> "Burakov, Anatoly" <anatoly.burakov@intel.com> writes:
> > Nitpicking, but i believe the coding style guide discourages using
> > boolean syntax for anything other than boolean checks, and it is
> > better to use a more explicit `if (x == NULL)`.
> 
> I see, it does.  Looking at the code-base, I see it mixed all over, some
> places using 'if (!ptr)' and others 'if (ptr == NULL)'. Actually, even
> in the flow_filtering.rst doc, it implies that if (!ptr) is acceptable.
> 
> Since I'm spinning a v6 with the constants, I'll fold this change in -
> maybe it makes sense to clean it up everywhere to help mitigate the
> confusion (for example, I most recently did work in the eal and the !ptr
> is all over there).  WDYT?

In general I agree cleanups are good, avoiding confusion.
About changing the whole codebase, just for styling I am not sure.
Please let's start with documentation fixes and discuss whether to move forward.

Re: [dpdk-dev] [PATCH v6 0/3] ip_frag: add a unit test for fragmentation

[Thomas Monjalon]

> Aaron Conole (3):
>   ip_frag: ensure minimum v4 fragmentation length
>   ip_frag: ensure minimum v6 fragmentation length
>   ipfrag: add unit test case

Applied, thanks