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

Aaron Conole aconole at redhat.com
Mon Nov 28 18:55:10 UTC 2016


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.

> *** 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; but we
should probably keep a separate copy of the elements to release, so that
the area is still free for dpdk to play with (complying with c99 5.1.2.2.1
item 2, bullet 4), but that we can release the memory (since 5.1.2.2.2
makes no guarantee that memory will be released if I'm reading it
correctly).

Is this (completely untested) patch acceptable?

---

diff --git a/lib/dpdk.c b/lib/dpdk.c
index 49a589a..2343588 100644
--- a/lib/dpdk.c
+++ b/lib/dpdk.c
@@ -250,7 +250,7 @@ get_dpdk_args(const struct smap *ovs_other_config, char ***argv,
     return i + extra_argc;
 }
 
-static char **dpdk_argv;
+static char **dpdk_argv, **dpdk_argv_release;
 static int dpdk_argc;
 
 static void
@@ -258,10 +258,11 @@ deferred_argv_release(void)
 {
     int result;
     for (result = 0; result < dpdk_argc; ++result) {
-        free(dpdk_argv[result]);
+        free(dpdk_argv_release[result]);
     }
 
     free(dpdk_argv);
+    free(dpdk_argv_release);
 }
 
 static void
@@ -366,6 +367,11 @@ dpdk_init__(const struct smap *ovs_other_config)
         ds_destroy(&eal_args);
     }
 
+    dpdk_argv_release = grow_argv(&dpdk_argv_release, 0, argc);
+    for (argc_tmp = 0; argc_tmp < argc; ++argc_tmp) {
+        dpdk_argv_release[argc_tmp] = argv[argc_tmp];
+    }
+
     /* Make sure things are initialized ... */
     result = rte_eal_init(argc, argv);
     if (result < 0) {


More information about the dev mailing list