| Message ID | 52cb119f-621c-4eb8-d2fd-1428a26a46b0@samsung.com |
|---|---|
| State | Not Applicable |
| Headers | show |
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! >> >>> *** 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? > 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.
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.
Daniele Di Proietto <diproiettod@vmware.com> writes: > 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". 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?
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]); } > >> *** 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. 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 */