[ovs-dev] [PATCH] dpdk: Fix abort on double free.
Aaron Conole
aconole at redhat.com
Tue Dec 6 15:25:01 UTC 2016
Daniele Di Proietto <diproiettod at vmware.com> writes:
> On 29/11/2016 07:57, "Aaron Conole" <aconole at redhat.com> wrote:
>
>>Hi Ilya,
>>
>>Ilya Maximets <i.maximets at samsung.com> writes:
>>
>>> On 28.11.2016 21:55, Aaron Conole wrote:
>>>> Ilya Maximets <i.maximets at 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 at redhat.com>
>>>>> Fixes: bab694097133 ("netdev-dpdk: Convert initialization from
>>>>> cmdline to db")
>>>>> Signed-off-by: Ilya Maximets <i.maximets at 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".
With Ilya's incremental, it won't - we will drop the static variables
which would still hold references, as well. So valgrind will report it
as leaked for sure.
> 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.
I'm okay if you want to take Ilya's patches for this, since I can't
truly test for this case (my environment hasn't reproduced the report).
Alternately, I can put in some printf() calls and prove it out that way,
then submit my patch formally. I would prefer if OvS would release the
memory, since it isn't required.
On further thought, though, I actually think it should be released
immediately, and you can see my original proposal did that, and was told
to change:
https://mail.openvswitch.org/pipermail/ovs-dev/2015-December/306540.html
So, I'd much prefer a patch which frees immediately after calling
dpdk_init. Nothing in the dpdk API documentation makes me believe that
it expects to use the memory.
Does anyone object to me submitting a formal patch which does an
immediate free of the memory passed to 'dpdk_init()' as the 'argv'
parameter?
More information about the dev
mailing list