Re: [dpdk-dev] [PATCH] dpdk: Fix abort on double free.

[Daniele Di Proietto <diproiettod@vmware.com>]

On 29/11/2016 07:57, "Aaron Conole" <aconole@redhat.com> wrote:

>Hi Ilya,
>
>Ilya Maximets <i.maximets@samsung.com> writes:
>
>> On 28.11.2016 21:55, Aaron Conole wrote:
>>> Ilya Maximets <i.maximets@samsung.com> writes:
>>> 
>>>> According to DPDK API (lib/librte_eal/common/include/rte_eal.h):
>>>>
>>>> 	"After the call to rte_eal_init(), all arguments argv[x]
>>>> 	 with x < ret may be modified and should not be accessed
>>>> 	 by the application."
>>>>
>>>> This means, that OVS must not free the arguments passed to DPDK.
>>>> In real world, 'rte_eal_init()' replaces the last argument in
>>>> 'dpdk_argv' with the first one by doing this:
>>> 
>>> Thanks for spotting this error, Ilya.
>>> 
>>>> 	# eal_parse_args() from lib/librte_eal/linuxapp/eal/eal.c
>>>>
>>>> 	char *prgname = argv[0];
>>>> 	...
>>>> 	if (optind >= 0)
>>>> 		argv[optind-1] = prgname;
>>>>
>>>> This leads to double free inside 'deferred_argv_release()' and
>>>> possible ABORT at exit:
>>> 
>>> I haven't seen this, which is both shocking and scary - the commit which
>>> does this copy is almost 4 years old;  did you have to do anything
>>> specific for this behavior to occur?  Did something change in DPDK
>>> recently that exposed this behavior?  Just wondering how you reproduced
>>> it.
>>
>> Abort was caught up accidentally. I'm able to reproduce it on my a
>> little unusual testing system (ARMv8 + Fedora 21 + clang 3.5) without
>> any specific manipulations. The bug exists always but it's hard
>> for libc to detect double free here because there are many other
>> frees/allocations at exit time. I've used following patch to confirm
>> the issue if it wasn't detected by libc:
>
>Well, it's at least good that you can observe it consistently.  Did you
>try my provided patch to see if that works as well?
>
>> diff --git a/lib/dpdk.c b/lib/dpdk.c
>> index 49a589a..65d2d28 100644
>> --- a/lib/dpdk.c
>> +++ b/lib/dpdk.c
>> @@ -258,6 +258,8 @@ deferred_argv_release(void)
>>  {
>>      int result;
>>      for (result = 0; result < dpdk_argc; ++result) {
>> +        VLOG_INFO("DPDK ARGV release: %2d: 0x%" PRIx64 " (%s)",
>> +                  result, (intptr_t)dpdk_argv[result], dpdk_argv[result]);
>>          free(dpdk_argv[result]);
>>      }
>>  
>
>It's quite glaring after studying the code.  Really good catch!

I agree, thanks for spotting this

>
>>> 
>>>> *** Error in `ovs-vswitchd': double free or corruption (fasttop) <...> ***
>>>> 	Program received signal SIGABRT, Aborted.
>>>>
>>>> 	#0  raise () from /lib64/libc.so.6
>>>> 	#1  abort () from /lib64/libc.so.6
>>>> 	#2  __libc_message () from /lib64/libc.so.6
>>>> 	#3  free () from /lib64/libc.so.6
>>>> 	#4  deferred_argv_release () at lib/dpdk.c:261
>>>> 	#5  __run_exit_handlers () from /lib64/libc.so.6
>>>> 	#6  exit () from /lib64/libc.so.6
>>>> 	#7  __libc_start_main () from /lib64/libc.so.6
>>>> 	#8  _start ()
>>>>
>>>> Fix that by not calling free for the memory passed to DPDK.
>>>>
>>>> CC: Aaron Conole <aconole@redhat.com>
>>>> Fixes: bab694097133 ("netdev-dpdk: Convert initialization from
>>>> cmdline to db")
>>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>>>> ---
>>> 
>>> We need to free the memory - I think that is not a question;
>>
>> Actually, it is. According to DPDK API (see above) 'rte_eal_init()'
>> takes the ownership of 'argv'. This means that we must not free
>> or use this memory.
>
>Apologies for the ranty-wall of text below.
>
>DPDK *cannot* take ownership of freeing this memory, unless 1) it expects a
>completely separate array from argv/argc than the one passed during
>program execution and initialization, or 2) it expects the hosted
>environment to give it the responsibility of cleaning this up.  It
>explicitly claims that the argv/argc is what comes from main(), and
>therefore should obey the restrictions and privileges afforded those
>variables.
>
>In fact, I don't even see anywhere that dpdk preserves argv, *at all*.
>Looking through the history very quickly (admittedly just back to commit
>af75078fece3615088e561357c1e97603e43a5fe in dpdk) confirms that dpdk
>hasn't stored the arguments anywhere to do any processing.
>
>DPDK api guide does NOT state that it takes possession - and that matches
>with what happens in the code, BUT I will agree the statement
>
>  'all arguments argv[x] with x < ret may be modified and should not be
>  accessed by the application'
>
>is a bit ambiguous.  I think it's trying to say that the application should do
>its getopt()s parsing before calling the dpdk init routine, because DPDK libs
>will change the array.  I don't see a reason for modifying the array in
>the code (the `argv[optind-1] = progname`), but if the dpdk library wants
>to do that, it is free to do so according to C99 5.1.2.2.1;  I think
>it's best we always free what we allocate, which is why I suggested the
>side array patch which stores additional pointers to the data to be
>free'd up at exit.
>
>I am not sure which is more appropriate, since this is an exit condition,
>after all.  The memory will get free()d up eventually by the
>environments on which OvS runs.  It doesn't _feel_ correct to leave the
>memory dangling, since we can free it.
>
>Anyone else have thoughts on this?

I don't think it's a big deal to leak memory that has to be used until the process
terminates.  There are other examples of this in OvS, such as 'timewarp_seq' in
lib/timeval.c.  They should be reported by valgrind as "still reachable".

That said, at some point we might want to have 100% leak free valgrind runs, so
I think it's be better to free everything we allocate, so I would prefer Aaron's
solution.  I don't think DPDK should expect the arguments to be available in exit
handlers, i.e. after main() returns.

I don't feel strongly about it though, since, if I'm not mistaken, valgrind doesn't
support DPDK yet.

>
>> Some thoughts:
>> DPDK internally doesn't free this memory, but it's not the reason to
>> touch it from the outside. Actually, DPDK API change required here to
>> support freeing of this resources if needed. But until there is no
>> 'rte_eal_uninit()' such API change isn't actually useful.
>>
>> Also, I forget to remove the variables. So, the following incremental
>> to my original patch required:
>>
>> ------------------------------------
>> diff --git a/lib/dpdk.c b/lib/dpdk.c
>> index 2014946..4201149 100644
>> --- a/lib/dpdk.c
>> +++ b/lib/dpdk.c
>> @@ -250,9 +250,6 @@ get_dpdk_args(const struct smap *ovs_other_config, char ***argv,
>>      return i + extra_argc;
>>  }
>>  
>> -static char **dpdk_argv;
>> -static int dpdk_argc;
>> -
>>  static void
>>  dpdk_init__(const struct smap *ovs_other_config)
>>  {
>> @@ -370,9 +367,6 @@ dpdk_init__(const struct smap *ovs_other_config)
>>          }
>>      }
>>  
>> -    dpdk_argv = argv;
>> -    dpdk_argc = argc;
>> -
>>      rte_memzone_dump(stdout);
>>  
>>      /* We are called from the main thread here */
>> ------------------------------------
>>
>> Best regards, Ilya Maximets.