@@ -832,7 +832,6 @@ format_dpif_flow(struct ds *ds, const struct dpif_flow *f, struct hmap *ports,
if (dpctl_p->verbosity && f->attrs.dp_extra_info) {
ds_put_format(ds, ", dp-extra-info:%s", f->attrs.dp_extra_info);
}
- free(f->attrs.dp_extra_info);
}
struct dump_types {
@@ -551,6 +551,7 @@ struct dp_netdev_flow {
struct packet_batch_per_flow *batch;
/* Packet classification. */
+ char *dp_extra_info; /* String to return in a flow dump/get. */
struct dpcls_rule cr; /* In owning dp_netdev's 'cls'. */
/* 'cr' must be the last member. */
};
@@ -2096,6 +2097,7 @@ static void
dp_netdev_flow_free(struct dp_netdev_flow *flow)
{
dp_netdev_actions_free(dp_netdev_flow_get_actions(flow));
+ free(flow->dp_extra_info);
free(flow);
}
@@ -3158,21 +3160,7 @@ dp_netdev_flow_to_dpif_flow(const struct dp_netdev *dp,
flow->pmd_id = netdev_flow->pmd_id;
get_dpif_flow_status(dp, netdev_flow, &flow->stats, &flow->attrs);
-
- struct ds extra_info = DS_EMPTY_INITIALIZER;
- size_t unit;
-
- ds_put_cstr(&extra_info, "miniflow_bits(");
- FLOWMAP_FOR_EACH_UNIT (unit) {
- if (unit) {
- ds_put_char(&extra_info, ',');
- }
- ds_put_format(&extra_info, "%d",
- count_1bits(netdev_flow->cr.mask->mf.map.bits[unit]));
- }
- ds_put_char(&extra_info, ')');
- flow->attrs.dp_extra_info = ds_steal_cstr(&extra_info);
- ds_destroy(&extra_info);
+ flow->attrs.dp_extra_info = netdev_flow->dp_extra_info;
}
static int
@@ -3312,9 +3300,11 @@ dp_netdev_flow_add(struct dp_netdev_pmd_thread *pmd,
const struct nlattr *actions, size_t actions_len)
OVS_REQUIRES(pmd->flow_mutex)
{
+ struct ds extra_info = DS_EMPTY_INITIALIZER;
struct dp_netdev_flow *flow;
struct netdev_flow_key mask;
struct dpcls *cls;
+ size_t unit;
/* Make sure in_port is exact matched before we read it. */
ovs_assert(match->wc.masks.in_port.odp_port == ODPP_NONE);
@@ -3351,6 +3341,18 @@ dp_netdev_flow_add(struct dp_netdev_pmd_thread *pmd,
dp_netdev_get_mega_ufid(match, CONST_CAST(ovs_u128 *, &flow->mega_ufid));
netdev_flow_key_init_masked(&flow->cr.flow, &match->flow, &mask);
+ ds_put_cstr(&extra_info, "miniflow_bits(");
+ FLOWMAP_FOR_EACH_UNIT (unit) {
+ if (unit) {
+ ds_put_char(&extra_info, ',');
+ }
+ ds_put_format(&extra_info, "%d",
+ count_1bits(flow->cr.mask->mf.map.bits[unit]));
+ }
+ ds_put_char(&extra_info, ')');
+ flow->dp_extra_info = ds_steal_cstr(&extra_info);
+ ds_destroy(&extra_info);
+
/* Select dpcls for in_port. Relies on in_port to be exact match. */
cls = dp_netdev_pmd_find_dpcls(pmd, in_port);
dpcls_insert(cls, &flow->cr, &mask);
@@ -966,7 +966,6 @@ 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);
@@ -511,9 +511,9 @@ struct dpif_flow_detailed_stats {
};
struct dpif_flow_attrs {
- bool offloaded; /* True if flow is offloaded to HW. */
- const char *dp_layer; /* DP layer the flow is handled in. */
- char *dp_extra_info; /* Extra information provided by DP. */
+ bool offloaded; /* True if flow is offloaded to HW. */
+ const char *dp_layer; /* DP layer the flow is handled in. */
+ const char *dp_extra_info; /* Extra information provided by DP. */
};
struct dpif_flow_dump_types {
@@ -745,8 +745,7 @@ struct dpif_execute {
* 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. 'attrs.dp_extra_info' is a dynamically allocated
- * string that should be freed if provided by the datapath.
+ * fields before quiescing.
*
* Callers should always provide 'key' to improve dpif logging in the event of
* errors or unexpected behaviour.
@@ -2646,9 +2646,6 @@ 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
@@ -6282,9 +6282,6 @@ ofproto_unixctl_dpif_dump_flows(struct unixctl_conn *conn,
while (dpif_flow_dump_next(flow_dump_thread, &f, 1)) {
struct flow flow;
- /* No need for extra info. */
- free(f.attrs.dp_extra_info);
-
if ((odp_flow_key_to_flow(f.key, f.key_len, &flow, NULL)
== ODP_FIT_ERROR)
|| (xlate_lookup_ofproto(ofproto->backer, &flow, NULL, NULL)
dpctl module leaks the 'dp_extra_info' in case the dumped flow doesn't fit the dump filter while executing dpctl/dump-flows and also while executing dpctl/get-flow. This is already a 3rd attempt to fix all the leaks and incorrect usage of this string that definitely indicates poor initial design of the feature. Flow dump/get documentation clearly states that the caller does not own the data provided in dpif_flow. Datapath still owns all the data and promises to not free/modify it until the next quiescent period, however we're requesting the caller to free 'dp_extra_info' and this obviously breaks the rules. This patch fixes the issue by by storing 'dp_extra_info' within 'struct dp_netdev_flow' making datapath to own it. 'dp_netdev_flow' is RCU-protected, so it will be valid until the next quiescent period. 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/dpctl.c | 1 - lib/dpif-netdev.c | 32 +++++++++++++++++--------------- lib/dpif.c | 1 - lib/dpif.h | 9 ++++----- ofproto/ofproto-dpif-upcall.c | 3 --- ofproto/ofproto-dpif.c | 3 --- 6 files changed, 21 insertions(+), 28 deletions(-)