[PATCH 1/2] reorder: fix ready buffers not being nulled out

[PATCH 1/2] reorder: fix ready buffers not being nulled out

[Pavan Nikhilesh]

The ready buffers should be set to NULL when drained else it might result
in double free (mempool put) when rte_reorder_free is called.

Fixes: b70b56032bff ("reorder: new library")

Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
---
 lib/librte_reorder/rte_reorder.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_reorder/rte_reorder.c b/lib/librte_reorder/rte_reorder.c
index 010dff6..302eba6 100644
--- a/lib/librte_reorder/rte_reorder.c
+++ b/lib/librte_reorder/rte_reorder.c
@@ -392,6 +392,7 @@ rte_reorder_drain(struct rte_reorder_buffer *b, struct rte_mbuf **mbufs,
 	/* Try to fetch requested number of mbufs from ready buffer */
 	while ((drain_cnt < max_mbufs) && (ready_buf->tail != ready_buf->head)) {
 		mbufs[drain_cnt++] = ready_buf->entries[ready_buf->tail];
+		ready_buf->entries[ready_buf->tail] = NULL;
 		ready_buf->tail = (ready_buf->tail + 1) & ready_buf->mask;
 	}
 
-- 
2.7.4

[PATCH 2/2] test/reorder: fix reorder drain test

[Pavan Nikhilesh]

The reorder drain test fails due to mempool corruption caused by freeing
packet buffer twice.

Fixes: d0c9b58d7156 ("app/test: new reorder unit test")

Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
---
 test/test/test_reorder.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/test/test/test_reorder.c b/test/test/test_reorder.c
index 4ec22ac..51c2dcd 100644
--- a/test/test/test_reorder.c
+++ b/test/test/test_reorder.c
@@ -70,13 +70,15 @@ test_reorder_create(void)
 	TEST_ASSERT((b == NULL) && (rte_errno == EINVAL),
 			"No error on create() with NULL name");
 
-	b = rte_reorder_create("PKT", rte_socket_id(), REORDER_BUFFER_SIZE_INVALID);
+	b = rte_reorder_create("PKT", rte_socket_id(),
+			REORDER_BUFFER_SIZE_INVALID);
 	TEST_ASSERT((b == NULL) && (rte_errno == EINVAL),
 			"No error on create() with invalid buffer size param.");
 
 	b = rte_reorder_create("PKT_RO1", rte_socket_id(), REORDER_BUFFER_SIZE);
 	TEST_ASSERT_EQUAL(b, test_params->b,
-			"New reorder instance created with already existing name");
+			"New reorder instance created with already existing"
+			" name");
 
 	return 0;
 }
@@ -88,8 +90,8 @@ test_reorder_init(void)
 	unsigned int size;
 	/*
 	 * The minimum memory area size that should be passed to library is,
-	 * sizeof(struct rte_reorder_buffer) + (2 * size * sizeof(struct rte_mbuf *));
-	 * Otherwise error will be thrown
+	 * sizeof(struct rte_reorder_buffer) + (2 * size *
+	 * sizeof(struct rte_mbuf *)); Otherwise error will be thrown
 	 */
 
 	size = 100;
@@ -189,15 +191,16 @@ test_reorder_insert(void)
 	for (i = 0; i < size; i++) {
 		ret = rte_reorder_insert(b, bufs[i]);
 		if (ret != 0) {
-			printf("%s:%d: Error inserting packet with seqn less than size\n",
+			printf("%s:%d: Error inserting packet with seqn less"
+					" than size\n",
 					__func__, __LINE__);
 			ret = -1;
 			goto exit;
 		}
 	}
 
-	/* early packet - should move mbufs to ready buf and move sequence window
-	 * reorder_seq = 4
+	/* early packet - should move mbufs to ready buf and move sequence
+	 * window reorder_seq = 4
 	 * RB[] = {0, 1, 2, 3}
 	 * OB[] = {4, NULL, NULL, NULL}
 	 */
@@ -208,12 +211,14 @@ test_reorder_insert(void)
 		ret = -1;
 		goto exit;
 	}
+	i++;
 
 	/* early packet from current sequence window - full ready buffer */
 	bufs[5]->seqn = 2 * size;
 	ret = rte_reorder_insert(b, bufs[5]);
 	if (!((ret == -1) && (rte_errno == ENOSPC))) {
-		printf("%s:%d: No error inserting early packet with full ready buffer\n",
+		printf("%s:%d: No error inserting early packet with full ready"
+				" buffer\n",
 				__func__, __LINE__);
 		ret = -1;
 		goto exit;
@@ -231,7 +236,7 @@ test_reorder_insert(void)
 
 	ret = 0;
 exit:
-	rte_mempool_put_bulk(p, (void *)bufs, num_bufs);
+	rte_mempool_put_bulk(p, (void **)&bufs[i], num_bufs - i);
 	rte_reorder_free(b);
 	return ret;
 }
@@ -360,6 +365,13 @@ test_setup(void)
 	return 0;
 }
 
+static void
+test_destroy(void)
+{
+	rte_mempool_free(test_params->p);
+	test_params->p = NULL;
+}
+
 static struct unit_test_suite reorder_test_suite  = {
 
 	.setup = test_setup,
@@ -372,7 +384,8 @@ static struct unit_test_suite reorder_test_suite  = {
 		TEST_CASE(test_reorder_insert),
 		TEST_CASE(test_reorder_drain),
 		TEST_CASES_END()
-	}
+	},
+	.teardown = test_destroy
 };
 
 static int
-- 
2.7.4

Re: [PATCH 1/2] reorder: fix ready buffers not being nulled out

[Thomas Monjalon]

12/09/2017 17:06, Pavan Nikhilesh:
> The ready buffers should be set to NULL when drained else it might result
> in double free (mempool put) when rte_reorder_free is called.
> 
> Fixes: b70b56032bff ("reorder: new library")
> 
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>

Anyone to review, please?

Re: [PATCH 2/2] test/reorder: fix reorder drain test

[Bruce Richardson]

On Tue, Sep 12, 2017 at 08:36:04PM +0530, Pavan Nikhilesh wrote:
> The reorder drain test fails due to mempool corruption caused by freeing
> packet buffer twice.
> 
> Fixes: d0c9b58d7156 ("app/test: new reorder unit test")
> 
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
> ---
>  test/test/test_reorder.c | 33 +++++++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/test/test/test_reorder.c b/test/test/test_reorder.c
> index 4ec22ac..51c2dcd 100644
> --- a/test/test/test_reorder.c
> +++ b/test/test/test_reorder.c
> @@ -70,13 +70,15 @@ test_reorder_create(void)
>  	TEST_ASSERT((b == NULL) && (rte_errno == EINVAL),
>  			"No error on create() with NULL name");
>  
> -	b = rte_reorder_create("PKT", rte_socket_id(), REORDER_BUFFER_SIZE_INVALID);
> +	b = rte_reorder_create("PKT", rte_socket_id(),
> +			REORDER_BUFFER_SIZE_INVALID);
>  	TEST_ASSERT((b == NULL) && (rte_errno == EINVAL),
>  			"No error on create() with invalid buffer size param.");
>  
>  	b = rte_reorder_create("PKT_RO1", rte_socket_id(), REORDER_BUFFER_SIZE);
>  	TEST_ASSERT_EQUAL(b, test_params->b,
> -			"New reorder instance created with already existing name");
> +			"New reorder instance created with already existing"
> +			" name");
>  
>  	return 0;
>  }

These changes are just cosmetic and so shouldn't really be included in
this patch. Ideally, cosmetic changes should be made only when you are
already touching the affected lines anyway due to some other change.
Also, it's bad practice to split literal strings across lines, even if
the line ends up crossing the 80-char threshold. Being able to grep
error strings is more important than keeping lines short.

This applies to many of the other changes in this patch. It makes it hard to
review for the actual functional changes.

Regards,
/Bruce

Re: [PATCH 1/2] reorder: fix ready buffers not being nulled out

[Bruce Richardson]

On Tue, Sep 12, 2017 at 08:36:03PM +0530, Pavan Nikhilesh wrote:
> The ready buffers should be set to NULL when drained else it might
> result in double free (mempool put) when rte_reorder_free is called.
> 
> Fixes: b70b56032bff ("reorder: new library")
> 
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com> ---
> lib/librte_reorder/rte_reorder.c | 1 + 1 file changed, 1 insertion(+)
> 
Rather than having an addition write for each entry going through the
reorder library, it should be possible to change free function so that
it only frees entries based on the index values.

In fact, a better solution to having reorder_free just blindly free the
mbufs would be to have reorder_free hand them back to the application,
or allow reorder_free to fail if the reorder buffer is non-empty. Making
such a change would be an ABI break, though.

/Bruce