dpif: Fix dp_extra_info leak by reworking the allocation scheme.

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.

Fixes: 0e8f5c6a38d0 ("dpif-netdev: Modified ovs-appctl dpctl/dump-flows command")
Tested-by: Emma Finn <emma.finn@intel.com>
Acked-by: Emma Finn <emma.finn@intel.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

4 files changed, 21 insertions, 22 deletions

diff --git a/lib/dpctl.c b/lib/dpctl.c
index 4ebb00456..db2b1f896 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -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 {
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 3be41014e..d393aab5e 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -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);
@@ -3355,6 +3345,18 @@ dp_netdev_flow_add(struct dp_netdev_pmd_thread *pmd,
     cls = dp_netdev_pmd_find_dpcls(pmd, in_port);
     dpcls_insert(cls, &flow->cr, &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);
+
     cmap_insert(&pmd->flow_table, CONST_CAST(struct cmap_node *, &flow->node),
                 dp_netdev_flow_hash(&flow->ufid));
 
diff --git a/lib/dpif.c b/lib/dpif.c
index 6cbcdfb2e..9d9c716c1 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -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);
diff --git a/lib/dpif.h b/lib/dpif.h
index 286a0e2d5..4df8f7c8b 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -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.