[v1,3/6] app/test: fix freeing mbufs in distributor tests
Checks
Commit Message
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
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>
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.
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
>
>
@@ -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;
}