mir/ovs
2
0
Fork 0
mirror of https://github.com/openvswitch/ovs synced 2025-08-30 22:05:19 +00:00
Code Issues Releases Wiki Activity

dpif: Fix dp_extra_info leak by reworking the allocation scheme.

Browse Source 
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: 0e8f5c6a38 ("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>
This commit is contained in:
Ilya Maximets
2020-01-23 19:10:05 +01:00
parent 79eadafeb1
commit 342b8904ab
6 changed files with 21 additions and 28 deletions
Download Patch File Download Diff File Expand all files Collapse all files

32
lib/dpif-netdev.c
View File

@@ -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));