| Message ID | 1466433172-30787-1-git-send-email-u9012063@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Mon, Jun 20, 2016 at 07:32:52AM -0700, William Tu wrote: > Testcase 2050, ovn -- 3 HVs, 1 LS, 3 lports/HV, reports possible leak: > nln_notifier_create (netlink-notifier.c:131) > name_table_init (route-table.c:319) > route_table_init (route-table.c:110) > dp_initialize (dpif.c:126) > dp_unregister_provider (dpif.c:218) > dpif_dummy_override (dpif-netdev.c:4309) > dpif_dummy_register (dpif-netdev.c:4329) > dummy_enable (dummy.c:46) > parse_options (ovs-vswitchd.c:205) > main (ovs-vswitchd.c:76) > 'name_notifier' could be overwritten without being free'd. > > Tested-at: https://travis-ci.org/williamtu/ovs-travis/builds/138910851 > Signed-off-by: William Tu <u9012063@gmail.com> > --- > lib/route-table.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/lib/route-table.c b/lib/route-table.c > index 58e7f62..cf01c34 100644 > --- a/lib/route-table.c > +++ b/lib/route-table.c > @@ -316,6 +316,7 @@ route_table_fallback_lookup(const struct in6_addr *ip6_dst OVS_UNUSED, > static void > name_table_init(void) > { > + free(name_notifier); > name_notifier = rtnetlink_notifier_create(name_table_change, NULL); > } That doesn't seem right. I could not reproduce the valgrind problem by running TESTSUITEFLAGS=2050 make check-valgrind. But this has several problems. Fist, it's not clear code. Second, if name_notifier was not NULL, it could release memory still in use, and cause other potential bugs and leaks as well. Third: it's not even necessary. This looks much more like a false positive from valgrind. Unless we are calling name_table_init twice, which I can't see how. Can you look into the real bug here, maybe WARN whenever name_table_init when name_notifier is not NULL? If this is really a false positive and you really want to get rid of it, you could just do: static void name_table_init(void) { - name_notifier = rtnetlink_notifier_create(name_table_change, NULL); + if (name_notifier == NULL) { + name_notifier = rtnetlink_notifier_create(name_table_change, NULL); + } } Cascardo. > > -- > 2.5.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
Hi Cascardo, Thanks for your feedback. I did a couple of more tests and I think it should be valgrind's false positive. Even for testcase 1 (TESTSUITEFLAGS='1'), my valgrind complains about this case as "possible lost." On the other hand, I do check and make sure that we only called the name_table_init() once. Although we never free the 'name_notifier', since it's a static variable, valgrind should reports "still reachable" instead of "possible lost" when ovs-vswitchd exits. Regards, William On Mon, Jun 20, 2016 at 12:08 PM, Thadeu Lima de Souza Cascardo <cascardo@redhat.com> wrote: > On Mon, Jun 20, 2016 at 07:32:52AM -0700, William Tu wrote: >> Testcase 2050, ovn -- 3 HVs, 1 LS, 3 lports/HV, reports possible leak: >> nln_notifier_create (netlink-notifier.c:131) >> name_table_init (route-table.c:319) >> route_table_init (route-table.c:110) >> dp_initialize (dpif.c:126) >> dp_unregister_provider (dpif.c:218) >> dpif_dummy_override (dpif-netdev.c:4309) >> dpif_dummy_register (dpif-netdev.c:4329) >> dummy_enable (dummy.c:46) >> parse_options (ovs-vswitchd.c:205) >> main (ovs-vswitchd.c:76) >> 'name_notifier' could be overwritten without being free'd. >> >> Tested-at: https://travis-ci.org/williamtu/ovs-travis/builds/138910851 >> Signed-off-by: William Tu <u9012063@gmail.com> >> --- >> lib/route-table.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/lib/route-table.c b/lib/route-table.c >> index 58e7f62..cf01c34 100644 >> --- a/lib/route-table.c >> +++ b/lib/route-table.c >> @@ -316,6 +316,7 @@ route_table_fallback_lookup(const struct in6_addr *ip6_dst OVS_UNUSED, >> static void >> name_table_init(void) >> { >> + free(name_notifier); >> name_notifier = rtnetlink_notifier_create(name_table_change, NULL); >> } > > That doesn't seem right. I could not reproduce the valgrind problem by running > TESTSUITEFLAGS=2050 make check-valgrind. > > But this has several problems. Fist, it's not clear code. Second, if > name_notifier was not NULL, it could release memory still in use, and cause > other potential bugs and leaks as well. > > Third: it's not even necessary. This looks much more like a false positive from > valgrind. Unless we are calling name_table_init twice, which I can't see how. > Can you look into the real bug here, maybe WARN whenever name_table_init when > name_notifier is not NULL? > > If this is really a false positive and you really want to get rid of it, you > could just do: > > static void > name_table_init(void) > { > - name_notifier = rtnetlink_notifier_create(name_table_change, NULL); > + if (name_notifier == NULL) { > + name_notifier = rtnetlink_notifier_create(name_table_change, NULL); > + } > } > > Cascardo. > >> >> -- >> 2.5.0 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> http://openvswitch.org/mailman/listinfo/dev
Hi Ben, Thanks, this indeed fixes the "possible" leaks. Regards, William On Fri, Jul 1, 2016 at 8:24 PM, Ben Pfaff <blp@ovn.org> wrote: > Hi William, please try this patch as a substitute for yours. It should > ensure that pointers to nln_notifiers are to the beginning of the > structs instead of to the middle, meaning that valgrind does not > consider them "possible" leaks. > > diff --git a/lib/netlink-notifier.c b/lib/netlink-notifier.c > index 0867952..3de6e15 100644 > --- a/lib/netlink-notifier.c > +++ b/lib/netlink-notifier.c > @@ -46,9 +46,9 @@ struct nln { > }; > > struct nln_notifier { > + struct ovs_list node; > struct nln *nln; /* Parent nln. */ > > - struct ovs_list node; > int multicast_group; /* Multicast group we listen on. */ > nln_notify_func *cb; > void *aux; > > On Sat, Jun 25, 2016 at 12:30:46PM -0700, William Tu wrote: >> Hi Cascardo, >> >> Thanks for your feedback. >> I did a couple of more tests and I think it should be valgrind's false >> positive. Even for testcase 1 (TESTSUITEFLAGS='1'), my valgrind >> complains about this case as "possible lost." >> >> On the other hand, I do check and make sure that we only called the >> name_table_init() once. Although we never free the 'name_notifier', >> since it's a static variable, valgrind should reports "still >> reachable" instead of "possible lost" when ovs-vswitchd exits. >> >> Regards, >> William >> >> >> On Mon, Jun 20, 2016 at 12:08 PM, Thadeu Lima de Souza Cascardo >> <cascardo@redhat.com> wrote: >> > On Mon, Jun 20, 2016 at 07:32:52AM -0700, William Tu wrote: >> >> Testcase 2050, ovn -- 3 HVs, 1 LS, 3 lports/HV, reports possible leak: >> >> nln_notifier_create (netlink-notifier.c:131) >> >> name_table_init (route-table.c:319) >> >> route_table_init (route-table.c:110) >> >> dp_initialize (dpif.c:126) >> >> dp_unregister_provider (dpif.c:218) >> >> dpif_dummy_override (dpif-netdev.c:4309) >> >> dpif_dummy_register (dpif-netdev.c:4329) >> >> dummy_enable (dummy.c:46) >> >> parse_options (ovs-vswitchd.c:205) >> >> main (ovs-vswitchd.c:76) >> >> 'name_notifier' could be overwritten without being free'd. >> >> >> >> Tested-at: https://travis-ci.org/williamtu/ovs-travis/builds/138910851 >> >> Signed-off-by: William Tu <u9012063@gmail.com> >> >> --- >> >> lib/route-table.c | 1 + >> >> 1 file changed, 1 insertion(+) >> >> >> >> diff --git a/lib/route-table.c b/lib/route-table.c >> >> index 58e7f62..cf01c34 100644 >> >> --- a/lib/route-table.c >> >> +++ b/lib/route-table.c >> >> @@ -316,6 +316,7 @@ route_table_fallback_lookup(const struct in6_addr *ip6_dst OVS_UNUSED, >> >> static void >> >> name_table_init(void) >> >> { >> >> + free(name_notifier); >> >> name_notifier = rtnetlink_notifier_create(name_table_change, NULL); >> >> } >> > >> > That doesn't seem right. I could not reproduce the valgrind problem by running >> > TESTSUITEFLAGS=2050 make check-valgrind. >> > >> > But this has several problems. Fist, it's not clear code. Second, if >> > name_notifier was not NULL, it could release memory still in use, and cause >> > other potential bugs and leaks as well. >> > >> > Third: it's not even necessary. This looks much more like a false positive from >> > valgrind. Unless we are calling name_table_init twice, which I can't see how. >> > Can you look into the real bug here, maybe WARN whenever name_table_init when >> > name_notifier is not NULL? >> > >> > If this is really a false positive and you really want to get rid of it, you >> > could just do: >> > >> > static void >> > name_table_init(void) >> > { >> > - name_notifier = rtnetlink_notifier_create(name_table_change, NULL); >> > + if (name_notifier == NULL) { >> > + name_notifier = rtnetlink_notifier_create(name_table_change, NULL); >> > + } >> > } >> > >> > Cascardo. >> > >> >> >> >> -- >> >> 2.5.0 >> >> >> >> _______________________________________________ >> >> dev mailing list >> >> dev@openvswitch.org >> >> http://openvswitch.org/mailman/listinfo/dev >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> http://openvswitch.org/mailman/listinfo/dev
diff --git a/lib/route-table.c b/lib/route-table.c index 58e7f62..cf01c34 100644 --- a/lib/route-table.c +++ b/lib/route-table.c @@ -316,6 +316,7 @@ route_table_fallback_lookup(const struct in6_addr *ip6_dst OVS_UNUSED, static void name_table_init(void) { + free(name_notifier); name_notifier = rtnetlink_notifier_create(name_table_change, NULL); }
Testcase 2050, ovn -- 3 HVs, 1 LS, 3 lports/HV, reports possible leak: nln_notifier_create (netlink-notifier.c:131) name_table_init (route-table.c:319) route_table_init (route-table.c:110) dp_initialize (dpif.c:126) dp_unregister_provider (dpif.c:218) dpif_dummy_override (dpif-netdev.c:4309) dpif_dummy_register (dpif-netdev.c:4329) dummy_enable (dummy.c:46) parse_options (ovs-vswitchd.c:205) main (ovs-vswitchd.c:76) 'name_notifier' could be overwritten without being free'd. Tested-at: https://travis-ci.org/williamtu/ovs-travis/builds/138910851 Signed-off-by: William Tu <u9012063@gmail.com> --- lib/route-table.c | 1 + 1 file changed, 1 insertion(+)