[v1,3/6] app/test: fix freeing mbufs in distributor tests
diff mbox series

Message ID 20200915193449.13310-4-l.wojciechow@partner.samsung.com
State Superseded
Delegated to: David Marchand
Headers show
Series
  • fix distributor synchronization issues
Related show

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Lukasz Wojciechowski Sept. 15, 2020, 7:34 p.m. UTC
Sanity tests with mbuf alloc and shutdown tests assume that
mbufs passed to worker cores are freed in handlers.
Such packets should not be returned to the distributor's main
core. The only packets that should be returned are the packets
send after completion of the tests in quit_workers function.

This patch fixes freeing mbufs, stops returning them
to distributor's core and cleans up unused variables.

Fixes: c0de0eb82e40 ("distributor: switch over to new API")
Cc: david.hunt@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 app/test/test_distributor.c | 37 +++++++++++--------------------------
 1 file changed, 11 insertions(+), 26 deletions(-)

Comments

David Hunt Sept. 17, 2020, 12:34 p.m. UTC | #1
Hi Lukasz,

On 15/9/2020 8:34 PM, Lukasz Wojciechowski wrote:
> Sanity tests with mbuf alloc and shutdown tests assume that
> mbufs passed to worker cores are freed in handlers.
> Such packets should not be returned to the distributor's main
> core. The only packets that should be returned are the packets
> send after completion of the tests in quit_workers function.
>
> This patch fixes freeing mbufs, stops returning them
> to distributor's core and cleans up unused variables.
>
> Fixes: c0de0eb82e40 ("distributor: switch over to new API")
> Cc: david.hunt@intel.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> ---
>   app/test/test_distributor.c | 37 +++++++++++--------------------------
>   1 file changed, 11 insertions(+), 26 deletions(-)
>
> diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
> index 0e49e3714..da13a9a3f 100644
> --- a/app/test/test_distributor.c
> +++ b/app/test/test_distributor.c
> @@ -277,24 +277,21 @@ handle_work_with_free_mbufs(void *arg)
>   	struct rte_mbuf *buf[8] __rte_cache_aligned;
>   	struct worker_params *wp = arg;
>   	struct rte_distributor *d = wp->dist;
> -	unsigned int count = 0;
>   	unsigned int i;
>   	unsigned int num = 0;
>   	unsigned int id = __atomic_fetch_add(&worker_idx, 1, __ATOMIC_RELAXED);
>   
>   	for (i = 0; i < 8; i++)
>   		buf[i] = NULL;
> -	num = rte_distributor_get_pkt(d, id, buf, buf, num);
> +	num = rte_distributor_get_pkt(d, id, buf, buf, 0);
>   	while (!quit) {
> -		count += num;
>   		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
>   				__ATOMIC_ACQ_REL);
>   		for (i = 0; i < num; i++)
>   			rte_pktmbuf_free(buf[i]);
>   		num = rte_distributor_get_pkt(d,
> -				id, buf, buf, num);
> +				id, buf, buf, 0);
>   	}
> -	count += num;
>   	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
>   			__ATOMIC_ACQ_REL);
>   	rte_distributor_return_pkt(d, id, buf, num);
> @@ -322,7 +319,6 @@ sanity_test_with_mbuf_alloc(struct worker_params *wp, struct rte_mempool *p)
>   			rte_distributor_process(d, NULL, 0);
>   		for (j = 0; j < BURST; j++) {
>   			bufs[j]->hash.usr = (i+j) << 1;
> -			rte_mbuf_refcnt_set(bufs[j], 1);
>   		}
>   
>   		rte_distributor_process(d, bufs, BURST);
> @@ -346,20 +342,15 @@ sanity_test_with_mbuf_alloc(struct worker_params *wp, struct rte_mempool *p)
>   static int
>   handle_work_for_shutdown_test(void *arg)
>   {
> -	struct rte_mbuf *pkt = NULL;
>   	struct rte_mbuf *buf[8] __rte_cache_aligned;
>   	struct worker_params *wp = arg;
>   	struct rte_distributor *d = wp->dist;
> -	unsigned int count = 0;
>   	unsigned int num = 0;
> -	unsigned int total = 0;
>   	unsigned int i;
> -	unsigned int returned = 0;
>   	unsigned int zero_id = 0;
>   	const unsigned int id = __atomic_fetch_add(&worker_idx, 1,
>   			__ATOMIC_RELAXED);
> -
> -	num = rte_distributor_get_pkt(d, id, buf, buf, num);
> +	num = rte_distributor_get_pkt(d, id, buf, buf, 0);
>   
>   	zero_id = __atomic_load_n(&zero_idx, __ATOMIC_ACQUIRE);
>   	if (id == zero_id && num > 0) {
> @@ -371,13 +362,12 @@ handle_work_for_shutdown_test(void *arg)
>   	/* wait for quit single globally, or for worker zero, wait
>   	 * for zero_quit */
>   	while (!quit && !(id == zero_id && zero_quit)) {
> -		count += num;
>   		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
>   				__ATOMIC_ACQ_REL);
>   		for (i = 0; i < num; i++)
>   			rte_pktmbuf_free(buf[i]);
>   		num = rte_distributor_get_pkt(d,
> -				id, buf, buf, num);
> +				id, buf, buf, 0);
>   
>   		zero_id = __atomic_load_n(&zero_idx, __ATOMIC_ACQUIRE);
>   		if (id == zero_id && num > 0) {
> @@ -385,12 +375,7 @@ handle_work_for_shutdown_test(void *arg)
>   				__ATOMIC_ACQUIRE);
>   			__atomic_store_n(&zero_idx, zero_id, __ATOMIC_RELEASE);
>   		}
> -
> -		total += num;
>   	}
> -	count += num;
> -	returned = rte_distributor_return_pkt(d, id, buf, num);
> -
>   	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
>   			__ATOMIC_ACQ_REL);
>   	if (id == zero_id) {
> @@ -400,20 +385,20 @@ handle_work_for_shutdown_test(void *arg)
>   		while (zero_quit)
>   			usleep(100);
>   
> +		for (i = 0; i < num; i++)
> +			rte_pktmbuf_free(buf[i]);
>   		num = rte_distributor_get_pkt(d,
> -				id, buf, buf, num);
> +				id, buf, buf, 0);
>   
>   		while (!quit) {
> -			count += num;
> -			rte_pktmbuf_free(pkt);
> -			num = rte_distributor_get_pkt(d, id, buf, buf, num);
>   			__atomic_fetch_add(&worker_stats[id].handled_packets,
>   					num, __ATOMIC_ACQ_REL);
> +			for (i = 0; i < num; i++)
> +				rte_pktmbuf_free(buf[i]);
> +			num = rte_distributor_get_pkt(d, id, buf, buf, 0);
>   		}
> -		returned = rte_distributor_return_pkt(d,
> -				id, buf, num);
> -		printf("Num returned = %d\n", returned);
>   	}
> +	rte_distributor_return_pkt(d, id, buf, num);
>   	return 0;
>   }
>   

Nice cleanup, Thanks.

Acked-by: David Hunt <david.hunt@intel.com>
David Marchand Sept. 22, 2020, 12:42 p.m. UTC | #2
Hello Lukasz, David,


On Tue, Sep 15, 2020 at 9:35 PM Lukasz Wojciechowski
<l.wojciechow@partner.samsung.com> wrote:
> diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
> index 0e49e3714..da13a9a3f 100644
> --- a/app/test/test_distributor.c
> +++ b/app/test/test_distributor.c
> @@ -277,24 +277,21 @@ handle_work_with_free_mbufs(void *arg)
>         struct rte_mbuf *buf[8] __rte_cache_aligned;
>         struct worker_params *wp = arg;
>         struct rte_distributor *d = wp->dist;
> -       unsigned int count = 0;
>         unsigned int i;
>         unsigned int num = 0;
>         unsigned int id = __atomic_fetch_add(&worker_idx, 1, __ATOMIC_RELAXED);
>
>         for (i = 0; i < 8; i++)
>                 buf[i] = NULL;
> -       num = rte_distributor_get_pkt(d, id, buf, buf, num);
> +       num = rte_distributor_get_pkt(d, id, buf, buf, 0);

For my understanding, we pass an array even if we return 0 packet. Is
this necessary?


>         while (!quit) {
> -               count += num;
>                 __atomic_fetch_add(&worker_stats[id].handled_packets, num,
>                                 __ATOMIC_ACQ_REL);
>                 for (i = 0; i < num; i++)
>                         rte_pktmbuf_free(buf[i]);
>                 num = rte_distributor_get_pkt(d,
> -                               id, buf, buf, num);
> +                               id, buf, buf, 0);

Here, it gives the impression we have some potential use-after-free on
buf[] content.
And trying to pass NULL, I can see the distributor library
dereferences oldpkt[] without checking retcount != 0.
Lukasz Wojciechowski Sept. 23, 2020, 1:55 a.m. UTC | #3
Hello David,

W dniu 22.09.2020 o 14:42, David Marchand pisze:
> Hello Lukasz, David,
>
>
> On Tue, Sep 15, 2020 at 9:35 PM Lukasz Wojciechowski
> <l.wojciechow@partner.samsung.com> wrote:
>> diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
>> index 0e49e3714..da13a9a3f 100644
>> --- a/app/test/test_distributor.c
>> +++ b/app/test/test_distributor.c
>> @@ -277,24 +277,21 @@ handle_work_with_free_mbufs(void *arg)
>>          struct rte_mbuf *buf[8] __rte_cache_aligned;
>>          struct worker_params *wp = arg;
>>          struct rte_distributor *d = wp->dist;
>> -       unsigned int count = 0;
>>          unsigned int i;
>>          unsigned int num = 0;
>>          unsigned int id = __atomic_fetch_add(&worker_idx, 1, __ATOMIC_RELAXED);
>>
>>          for (i = 0; i < 8; i++)
>>                  buf[i] = NULL;
>> -       num = rte_distributor_get_pkt(d, id, buf, buf, num);
>> +       num = rte_distributor_get_pkt(d, id, buf, buf, 0);
> For my understanding, we pass an array even if we return 0 packet. Is
> this necessary?

The short answer is: yes.

That's because in case of using old legacy API (single distributor), it 
is required to pass a pointer to mbuf (might be NULL however). The new 
burst API functions call the old API dereferencing the first element of 
the array passed. So there must be a valid array containing at least 1 elem.

I pushed the v2 version of the patchset, which contains 2 new patches. 
Patch #7 fixes this issue in librte_distributor by passing NULL mbuf 
pointer to legacy API if number of return buffers is zero.

>
>
>>          while (!quit) {
>> -               count += num;
>>                  __atomic_fetch_add(&worker_stats[id].handled_packets, num,
>>                                  __ATOMIC_ACQ_REL);
>>                  for (i = 0; i < num; i++)
>>                          rte_pktmbuf_free(buf[i]);
>>                  num = rte_distributor_get_pkt(d,
>> -                               id, buf, buf, num);
>> +                               id, buf, buf, 0);
> Here, it gives the impression we have some potential use-after-free on
> buf[] content.
Nice catch! I missed it.
I fixed it in v2 by assigning NULL values to the bufs, so they won't be 
used after free.
> And trying to pass NULL, I can see the distributor library
> dereferences oldpkt[] without checking retcount != 0.

That's fixed in new patch v2 7/8


Best regards

Lukasz

>
>

Patch
diff mbox series

diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
index 0e49e3714..da13a9a3f 100644
--- a/app/test/test_distributor.c
+++ b/app/test/test_distributor.c
@@ -277,24 +277,21 @@  handle_work_with_free_mbufs(void *arg)
 	struct rte_mbuf *buf[8] __rte_cache_aligned;
 	struct worker_params *wp = arg;
 	struct rte_distributor *d = wp->dist;
-	unsigned int count = 0;
 	unsigned int i;
 	unsigned int num = 0;
 	unsigned int id = __atomic_fetch_add(&worker_idx, 1, __ATOMIC_RELAXED);
 
 	for (i = 0; i < 8; i++)
 		buf[i] = NULL;
-	num = rte_distributor_get_pkt(d, id, buf, buf, num);
+	num = rte_distributor_get_pkt(d, id, buf, buf, 0);
 	while (!quit) {
-		count += num;
 		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
 				__ATOMIC_ACQ_REL);
 		for (i = 0; i < num; i++)
 			rte_pktmbuf_free(buf[i]);
 		num = rte_distributor_get_pkt(d,
-				id, buf, buf, num);
+				id, buf, buf, 0);
 	}
-	count += num;
 	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
 			__ATOMIC_ACQ_REL);
 	rte_distributor_return_pkt(d, id, buf, num);
@@ -322,7 +319,6 @@  sanity_test_with_mbuf_alloc(struct worker_params *wp, struct rte_mempool *p)
 			rte_distributor_process(d, NULL, 0);
 		for (j = 0; j < BURST; j++) {
 			bufs[j]->hash.usr = (i+j) << 1;
-			rte_mbuf_refcnt_set(bufs[j], 1);
 		}
 
 		rte_distributor_process(d, bufs, BURST);
@@ -346,20 +342,15 @@  sanity_test_with_mbuf_alloc(struct worker_params *wp, struct rte_mempool *p)
 static int
 handle_work_for_shutdown_test(void *arg)
 {
-	struct rte_mbuf *pkt = NULL;
 	struct rte_mbuf *buf[8] __rte_cache_aligned;
 	struct worker_params *wp = arg;
 	struct rte_distributor *d = wp->dist;
-	unsigned int count = 0;
 	unsigned int num = 0;
-	unsigned int total = 0;
 	unsigned int i;
-	unsigned int returned = 0;
 	unsigned int zero_id = 0;
 	const unsigned int id = __atomic_fetch_add(&worker_idx, 1,
 			__ATOMIC_RELAXED);
-
-	num = rte_distributor_get_pkt(d, id, buf, buf, num);
+	num = rte_distributor_get_pkt(d, id, buf, buf, 0);
 
 	zero_id = __atomic_load_n(&zero_idx, __ATOMIC_ACQUIRE);
 	if (id == zero_id && num > 0) {
@@ -371,13 +362,12 @@  handle_work_for_shutdown_test(void *arg)
 	/* wait for quit single globally, or for worker zero, wait
 	 * for zero_quit */
 	while (!quit && !(id == zero_id && zero_quit)) {
-		count += num;
 		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
 				__ATOMIC_ACQ_REL);
 		for (i = 0; i < num; i++)
 			rte_pktmbuf_free(buf[i]);
 		num = rte_distributor_get_pkt(d,
-				id, buf, buf, num);
+				id, buf, buf, 0);
 
 		zero_id = __atomic_load_n(&zero_idx, __ATOMIC_ACQUIRE);
 		if (id == zero_id && num > 0) {
@@ -385,12 +375,7 @@  handle_work_for_shutdown_test(void *arg)
 				__ATOMIC_ACQUIRE);
 			__atomic_store_n(&zero_idx, zero_id, __ATOMIC_RELEASE);
 		}
-
-		total += num;
 	}
-	count += num;
-	returned = rte_distributor_return_pkt(d, id, buf, num);
-
 	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
 			__ATOMIC_ACQ_REL);
 	if (id == zero_id) {
@@ -400,20 +385,20 @@  handle_work_for_shutdown_test(void *arg)
 		while (zero_quit)
 			usleep(100);
 
+		for (i = 0; i < num; i++)
+			rte_pktmbuf_free(buf[i]);
 		num = rte_distributor_get_pkt(d,
-				id, buf, buf, num);
+				id, buf, buf, 0);
 
 		while (!quit) {
-			count += num;
-			rte_pktmbuf_free(pkt);
-			num = rte_distributor_get_pkt(d, id, buf, buf, num);
 			__atomic_fetch_add(&worker_stats[id].handled_packets,
 					num, __ATOMIC_ACQ_REL);
+			for (i = 0; i < num; i++)
+				rte_pktmbuf_free(buf[i]);
+			num = rte_distributor_get_pkt(d, id, buf, buf, 0);
 		}
-		returned = rte_distributor_return_pkt(d,
-				id, buf, num);
-		printf("Num returned = %d\n", returned);
 	}
+	rte_distributor_return_pkt(d, id, buf, num);
 	return 0;
 }