[dpdk-dev] [PATCH v1 3/6] app/test: fix freeing mbufs in distributor tests

Lukasz Wojciechowski l.wojciechow at partner.samsung.com
Wed Sep 23 03:55:20 CEST 2020


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 at 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

>
>
-- 
Lukasz Wojciechowski
Principal Software Engineer

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



More information about the dev mailing list