| Message ID | 20200117222702.12950-1-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] dpif: Fix leak and usage of uninitialized dp_extra_info. | expand |
On 2020-01-18 12:27 AM, Ilya Maximets wrote: > 'dpif_probe_feature'/'revalidate' doesn't free the 'dp_extra_info' > string. Also, all the implementations of dpif_flow_get() should > initialize the value to avoid printing/freeing of random memory. > > 30 bytes in 1 blocks are definitely lost in loss record 323 of 889 > at 0x483AD19: realloc (vg_replace_malloc.c:836) > by 0xDDAD89: xrealloc (util.c:149) > by 0xCE1609: ds_reserve (dynamic-string.c:63) > by 0xCE1A90: ds_put_format_valist (dynamic-string.c:161) > by 0xCE19B9: ds_put_format (dynamic-string.c:142) > by 0xCCCEA9: dp_netdev_flow_to_dpif_flow (dpif-netdev.c:3170) > by 0xCCD2DD: dpif_netdev_flow_get (dpif-netdev.c:3278) > by 0xCCEA0A: dpif_netdev_operate (dpif-netdev.c:3868) > by 0xCDF81B: dpif_operate (dpif.c:1361) > by 0xCDEE93: dpif_flow_get (dpif.c:1002) > by 0xCDECF9: dpif_probe_feature (dpif.c:962) > by 0xC635D2: check_recirc (ofproto-dpif.c:896) > by 0xC65C02: check_support (ofproto-dpif.c:1567) > by 0xC63274: open_dpif_backer (ofproto-dpif.c:818) > by 0xC65E3E: construct (ofproto-dpif.c:1605) > by 0xC4D436: ofproto_create (ofproto.c:549) > by 0xC3931A: bridge_reconfigure (bridge.c:877) > by 0xC3FEAC: bridge_run (bridge.c:3324) > by 0xC4551D: main (ovs-vswitchd.c:127) > > CC: Emma Finn <emma.finn@intel.com> > Fixes: 0e8f5c6a38d0 ("dpif-netdev: Modified ovs-appctl dpctl/dump-flows command") > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- > lib/dpif-netlink.c | 1 + > lib/dpif.c | 1 + > lib/dpif.h | 7 ++++--- > lib/netdev-offload-dpdk.c | 1 + > lib/netdev-offload-tc.c | 1 + > ofproto/ofproto-dpif-upcall.c | 3 +++ > 6 files changed, 11 insertions(+), 3 deletions(-) > > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c > index d865c0bdb..5b5c96d72 100644 > --- a/lib/dpif-netlink.c > +++ b/lib/dpif-netlink.c > @@ -1590,6 +1590,7 @@ dpif_netlink_flow_to_dpif_flow(struct dpif_flow *dpif_flow, > dpif_netlink_flow_get_stats(datapath_flow, &dpif_flow->stats); > dpif_flow->attrs.offloaded = false; > dpif_flow->attrs.dp_layer = "ovs"; > + dpif_flow->attrs.dp_extra_info = NULL; > } > > /* The design is such that all threads are working together on the first dump > diff --git a/lib/dpif.c b/lib/dpif.c > index 9d9c716c1..6cbcdfb2e 100644 > --- a/lib/dpif.c > +++ b/lib/dpif.c > @@ -966,6 +966,7 @@ dpif_probe_feature(struct dpif *dpif, const char *name, > && ovs_u128_equals(*ufid, flow.ufid)))) { > enable_feature = true; > } > + free(flow.attrs.dp_extra_info); > > error = dpif_flow_del(dpif, key->data, key->size, ufid, > NON_PMD_CORE_ID, NULL); > diff --git a/lib/dpif.h b/lib/dpif.h > index 59d82dce3..286a0e2d5 100644 > --- a/lib/dpif.h > +++ b/lib/dpif.h > @@ -741,11 +741,12 @@ struct dpif_execute { > * 'buffer' must point to an initialized buffer, with a recommended size of > * DPIF_FLOW_BUFSIZE bytes. > * > - * On success, 'flow' will be populated with the mask, actions and stats for > - * the datapath flow corresponding to 'key'. The mask and actions may point > + * On success, 'flow' will be populated with the mask, actions, stats and attrs > + * for the datapath flow corresponding to 'key'. The mask and actions may point > * within '*buffer', or may point at RCU-protected data. Therefore, callers > * that wish to hold these over quiescent periods must make a copy of these > - * fields before quiescing. > + * fields before quiescing. 'attrs.dp_extra_info' is a dynamically allocated > + * string that should be freed if provided by the datapath. > * > * Callers should always provide 'key' to improve dpif logging in the event of > * errors or unexpected behaviour. > diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c > index 1ae8230fa..f8c46bbaa 100644 > --- a/lib/netdev-offload-dpdk.c > +++ b/lib/netdev-offload-dpdk.c > @@ -1272,6 +1272,7 @@ netdev_offload_dpdk_flow_get(struct netdev *netdev, > } > memcpy(stats, &rte_flow_data->stats, sizeof *stats); > out: > + attrs->dp_extra_info = NULL; > return ret; > } > > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c > index 4988dadc3..723ec376d 100644 > --- a/lib/netdev-offload-tc.c > +++ b/lib/netdev-offload-tc.c > @@ -873,6 +873,7 @@ parse_tc_flower_to_match(struct tc_flower *flower, > attrs->offloaded = (flower->offloaded_state == TC_OFFLOADED_STATE_IN_HW) > || (flower->offloaded_state == TC_OFFLOADED_STATE_UNDEFINED); > attrs->dp_layer = "tc"; > + attrs->dp_extra_info = NULL; > > return 0; > } > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index 409286ab1..3aef9a6c3 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -2646,6 +2646,9 @@ revalidate(struct revalidator *revalidator) > bool already_dumped; > int error; > > + /* We don't need an extra information. */ > + free(f->attrs.dp_extra_info); > + > if (ukey_acquire(udpif, f, &ukey, &error)) { > if (error == EBUSY) { > /* Another thread is processing this flow, so don't bother > tested and fixes the issue. thanks! Acked-by: Roi Dayan <roid@mellanox.com>
On 20.01.2020 17:38, Roi Dayan wrote: > > > On 2020-01-18 12:27 AM, Ilya Maximets wrote: >> 'dpif_probe_feature'/'revalidate' doesn't free the 'dp_extra_info' >> string. Also, all the implementations of dpif_flow_get() should >> initialize the value to avoid printing/freeing of random memory. >> >> 30 bytes in 1 blocks are definitely lost in loss record 323 of 889 >> at 0x483AD19: realloc (vg_replace_malloc.c:836) >> by 0xDDAD89: xrealloc (util.c:149) >> by 0xCE1609: ds_reserve (dynamic-string.c:63) >> by 0xCE1A90: ds_put_format_valist (dynamic-string.c:161) >> by 0xCE19B9: ds_put_format (dynamic-string.c:142) >> by 0xCCCEA9: dp_netdev_flow_to_dpif_flow (dpif-netdev.c:3170) >> by 0xCCD2DD: dpif_netdev_flow_get (dpif-netdev.c:3278) >> by 0xCCEA0A: dpif_netdev_operate (dpif-netdev.c:3868) >> by 0xCDF81B: dpif_operate (dpif.c:1361) >> by 0xCDEE93: dpif_flow_get (dpif.c:1002) >> by 0xCDECF9: dpif_probe_feature (dpif.c:962) >> by 0xC635D2: check_recirc (ofproto-dpif.c:896) >> by 0xC65C02: check_support (ofproto-dpif.c:1567) >> by 0xC63274: open_dpif_backer (ofproto-dpif.c:818) >> by 0xC65E3E: construct (ofproto-dpif.c:1605) >> by 0xC4D436: ofproto_create (ofproto.c:549) >> by 0xC3931A: bridge_reconfigure (bridge.c:877) >> by 0xC3FEAC: bridge_run (bridge.c:3324) >> by 0xC4551D: main (ovs-vswitchd.c:127) >> >> CC: Emma Finn <emma.finn@intel.com> >> Fixes: 0e8f5c6a38d0 ("dpif-netdev: Modified ovs-appctl dpctl/dump-flows command") >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >> --- >> lib/dpif-netlink.c | 1 + >> lib/dpif.c | 1 + >> lib/dpif.h | 7 ++++--- >> lib/netdev-offload-dpdk.c | 1 + >> lib/netdev-offload-tc.c | 1 + >> ofproto/ofproto-dpif-upcall.c | 3 +++ >> 6 files changed, 11 insertions(+), 3 deletions(-) >> >> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c >> index d865c0bdb..5b5c96d72 100644 >> --- a/lib/dpif-netlink.c >> +++ b/lib/dpif-netlink.c >> @@ -1590,6 +1590,7 @@ dpif_netlink_flow_to_dpif_flow(struct dpif_flow *dpif_flow, >> dpif_netlink_flow_get_stats(datapath_flow, &dpif_flow->stats); >> dpif_flow->attrs.offloaded = false; >> dpif_flow->attrs.dp_layer = "ovs"; >> + dpif_flow->attrs.dp_extra_info = NULL; >> } >> >> /* The design is such that all threads are working together on the first dump >> diff --git a/lib/dpif.c b/lib/dpif.c >> index 9d9c716c1..6cbcdfb2e 100644 >> --- a/lib/dpif.c >> +++ b/lib/dpif.c >> @@ -966,6 +966,7 @@ dpif_probe_feature(struct dpif *dpif, const char *name, >> && ovs_u128_equals(*ufid, flow.ufid)))) { >> enable_feature = true; >> } >> + free(flow.attrs.dp_extra_info); >> >> error = dpif_flow_del(dpif, key->data, key->size, ufid, >> NON_PMD_CORE_ID, NULL); >> diff --git a/lib/dpif.h b/lib/dpif.h >> index 59d82dce3..286a0e2d5 100644 >> --- a/lib/dpif.h >> +++ b/lib/dpif.h >> @@ -741,11 +741,12 @@ struct dpif_execute { >> * 'buffer' must point to an initialized buffer, with a recommended size of >> * DPIF_FLOW_BUFSIZE bytes. >> * >> - * On success, 'flow' will be populated with the mask, actions and stats for >> - * the datapath flow corresponding to 'key'. The mask and actions may point >> + * On success, 'flow' will be populated with the mask, actions, stats and attrs >> + * for the datapath flow corresponding to 'key'. The mask and actions may point >> * within '*buffer', or may point at RCU-protected data. Therefore, callers >> * that wish to hold these over quiescent periods must make a copy of these >> - * fields before quiescing. >> + * fields before quiescing. 'attrs.dp_extra_info' is a dynamically allocated >> + * string that should be freed if provided by the datapath. >> * >> * Callers should always provide 'key' to improve dpif logging in the event of >> * errors or unexpected behaviour. >> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c >> index 1ae8230fa..f8c46bbaa 100644 >> --- a/lib/netdev-offload-dpdk.c >> +++ b/lib/netdev-offload-dpdk.c >> @@ -1272,6 +1272,7 @@ netdev_offload_dpdk_flow_get(struct netdev *netdev, >> } >> memcpy(stats, &rte_flow_data->stats, sizeof *stats); >> out: >> + attrs->dp_extra_info = NULL; >> return ret; >> } >> >> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c >> index 4988dadc3..723ec376d 100644 >> --- a/lib/netdev-offload-tc.c >> +++ b/lib/netdev-offload-tc.c >> @@ -873,6 +873,7 @@ parse_tc_flower_to_match(struct tc_flower *flower, >> attrs->offloaded = (flower->offloaded_state == TC_OFFLOADED_STATE_IN_HW) >> || (flower->offloaded_state == TC_OFFLOADED_STATE_UNDEFINED); >> attrs->dp_layer = "tc"; >> + attrs->dp_extra_info = NULL; >> >> return 0; >> } >> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c >> index 409286ab1..3aef9a6c3 100644 >> --- a/ofproto/ofproto-dpif-upcall.c >> +++ b/ofproto/ofproto-dpif-upcall.c >> @@ -2646,6 +2646,9 @@ revalidate(struct revalidator *revalidator) >> bool already_dumped; >> int error; >> >> + /* We don't need an extra information. */ >> + free(f->attrs.dp_extra_info); >> + >> if (ukey_acquire(udpif, f, &ukey, &error)) { >> if (error == EBUSY) { >> /* Another thread is processing this flow, so don't bother >> > > tested and fixes the issue. thanks! > > Acked-by: Roi Dayan <roid@mellanox.com> > Thanks! Applied to master. Best regards, Ilya Maximets.
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index d865c0bdb..5b5c96d72 100644 --- a/lib/dpif-netlink.c +++ b/lib/dpif-netlink.c @@ -1590,6 +1590,7 @@ dpif_netlink_flow_to_dpif_flow(struct dpif_flow *dpif_flow, dpif_netlink_flow_get_stats(datapath_flow, &dpif_flow->stats); dpif_flow->attrs.offloaded = false; dpif_flow->attrs.dp_layer = "ovs"; + dpif_flow->attrs.dp_extra_info = NULL; } /* The design is such that all threads are working together on the first dump diff --git a/lib/dpif.c b/lib/dpif.c index 9d9c716c1..6cbcdfb2e 100644 --- a/lib/dpif.c +++ b/lib/dpif.c @@ -966,6 +966,7 @@ dpif_probe_feature(struct dpif *dpif, const char *name, && ovs_u128_equals(*ufid, flow.ufid)))) { enable_feature = true; } + free(flow.attrs.dp_extra_info); error = dpif_flow_del(dpif, key->data, key->size, ufid, NON_PMD_CORE_ID, NULL); diff --git a/lib/dpif.h b/lib/dpif.h index 59d82dce3..286a0e2d5 100644 --- a/lib/dpif.h +++ b/lib/dpif.h @@ -741,11 +741,12 @@ struct dpif_execute { * 'buffer' must point to an initialized buffer, with a recommended size of * DPIF_FLOW_BUFSIZE bytes. * - * On success, 'flow' will be populated with the mask, actions and stats for - * the datapath flow corresponding to 'key'. The mask and actions may point + * On success, 'flow' will be populated with the mask, actions, stats and attrs + * for the datapath flow corresponding to 'key'. The mask and actions may point * within '*buffer', or may point at RCU-protected data. Therefore, callers * that wish to hold these over quiescent periods must make a copy of these - * fields before quiescing. + * fields before quiescing. 'attrs.dp_extra_info' is a dynamically allocated + * string that should be freed if provided by the datapath. * * Callers should always provide 'key' to improve dpif logging in the event of * errors or unexpected behaviour. diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c index 1ae8230fa..f8c46bbaa 100644 --- a/lib/netdev-offload-dpdk.c +++ b/lib/netdev-offload-dpdk.c @@ -1272,6 +1272,7 @@ netdev_offload_dpdk_flow_get(struct netdev *netdev, } memcpy(stats, &rte_flow_data->stats, sizeof *stats); out: + attrs->dp_extra_info = NULL; return ret; } diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c index 4988dadc3..723ec376d 100644 --- a/lib/netdev-offload-tc.c +++ b/lib/netdev-offload-tc.c @@ -873,6 +873,7 @@ parse_tc_flower_to_match(struct tc_flower *flower, attrs->offloaded = (flower->offloaded_state == TC_OFFLOADED_STATE_IN_HW) || (flower->offloaded_state == TC_OFFLOADED_STATE_UNDEFINED); attrs->dp_layer = "tc"; + attrs->dp_extra_info = NULL; return 0; } diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 409286ab1..3aef9a6c3 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -2646,6 +2646,9 @@ revalidate(struct revalidator *revalidator) bool already_dumped; int error; + /* We don't need an extra information. */ + free(f->attrs.dp_extra_info); + if (ukey_acquire(udpif, f, &ukey, &error)) { if (error == EBUSY) { /* Another thread is processing this flow, so don't bother
'dpif_probe_feature'/'revalidate' doesn't free the 'dp_extra_info' string. Also, all the implementations of dpif_flow_get() should initialize the value to avoid printing/freeing of random memory. 30 bytes in 1 blocks are definitely lost in loss record 323 of 889 at 0x483AD19: realloc (vg_replace_malloc.c:836) by 0xDDAD89: xrealloc (util.c:149) by 0xCE1609: ds_reserve (dynamic-string.c:63) by 0xCE1A90: ds_put_format_valist (dynamic-string.c:161) by 0xCE19B9: ds_put_format (dynamic-string.c:142) by 0xCCCEA9: dp_netdev_flow_to_dpif_flow (dpif-netdev.c:3170) by 0xCCD2DD: dpif_netdev_flow_get (dpif-netdev.c:3278) by 0xCCEA0A: dpif_netdev_operate (dpif-netdev.c:3868) by 0xCDF81B: dpif_operate (dpif.c:1361) by 0xCDEE93: dpif_flow_get (dpif.c:1002) by 0xCDECF9: dpif_probe_feature (dpif.c:962) by 0xC635D2: check_recirc (ofproto-dpif.c:896) by 0xC65C02: check_support (ofproto-dpif.c:1567) by 0xC63274: open_dpif_backer (ofproto-dpif.c:818) by 0xC65E3E: construct (ofproto-dpif.c:1605) by 0xC4D436: ofproto_create (ofproto.c:549) by 0xC3931A: bridge_reconfigure (bridge.c:877) by 0xC3FEAC: bridge_run (bridge.c:3324) by 0xC4551D: main (ovs-vswitchd.c:127) CC: Emma Finn <emma.finn@intel.com> Fixes: 0e8f5c6a38d0 ("dpif-netdev: Modified ovs-appctl dpctl/dump-flows command") Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- lib/dpif-netlink.c | 1 + lib/dpif.c | 1 + lib/dpif.h | 7 ++++--- lib/netdev-offload-dpdk.c | 1 + lib/netdev-offload-tc.c | 1 + ofproto/ofproto-dpif-upcall.c | 3 +++ 6 files changed, 11 insertions(+), 3 deletions(-)