[ovs-dev] [PATCH] dpif: Fix leak and usage of uninitialized dp_extra_info.
Ilya Maximets
i.maximets at ovn.org
Fri Jan 17 22:27:02 UTC 2020
'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 at intel.com>
Fixes: 0e8f5c6a38d0 ("dpif-netdev: Modified ovs-appctl dpctl/dump-flows command")
Signed-off-by: Ilya Maximets <i.maximets at 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
--
2.17.1
More information about the dev
mailing list