It is possible for a datapath to dump the same flow twice, for instance if the flow is the last in a batch of flows to be dumped, then a new flow is inserted into the same bucket before the flow dumper fetches another batch.
In this case, datapath flow stats may be duplicated: The revalidator records the stats from the first flow, using the ukey to get the stats delta. The ukey is deleted, then the revalidator reads the second (duplicate) flow and cannot lookup the ukey for the delta. As such, it will push the stats as-is. This patch reduces the likelihood of such stats duplications by deferring ukey deletion until after stats are pushed for deleted flows. Signed-off-by: Joe Stringer <[email protected]> --- ofproto/ofproto-dpif-upcall.c | 41 ++++++++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index e0a5aed..90329f1 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -1360,17 +1360,19 @@ exit: return ok; } +struct dump_op { + struct udpif_key *ukey; + struct udpif_flow_dump *udump; + struct dpif_flow_stats stats; /* Stats for 'op'. */ + struct dpif_op op; /* Flow del operation. */ +}; + static void revalidate_udumps(struct revalidator *revalidator, struct list *udumps) { struct udpif *udpif = revalidator->udpif; - struct { - struct dpif_flow_stats ukey_stats; /* Stats stored in the ukey. */ - struct dpif_flow_stats stats; /* Stats for 'op'. */ - struct dpif_op op; /* Flow del operation. */ - } ops[REVALIDATE_MAX_BATCH]; - + struct dump_op ops[REVALIDATE_MAX_BATCH]; struct dpif_op *opsp[REVALIDATE_MAX_BATCH]; struct udpif_flow_dump *udump, *next_udump; size_t n_ops, i, n_flows; @@ -1403,21 +1405,15 @@ revalidate_udumps(struct revalidator *revalidator, struct list *udumps) } if (must_del || (used && used < now - max_idle)) { - struct dpif_flow_stats *ukey_stats = &ops[n_ops].ukey_stats; - struct dpif_op *op = &ops[n_ops].op; + struct dump_op *dop = &ops[n_ops++]; + struct dpif_op *op = &dop->op; + dop->ukey = ukey; + dop->udump = udump; op->type = DPIF_OP_FLOW_DEL; op->u.flow_del.key = udump->key; op->u.flow_del.key_len = udump->key_len; op->u.flow_del.stats = &ops[n_ops].stats; - n_ops++; - - if (ukey) { - *ukey_stats = ukey->stats; - ukey_delete(revalidator, ukey); - } else { - memset(ukey_stats, 0, sizeof *ukey_stats); - } continue; } @@ -1456,7 +1452,7 @@ revalidate_udumps(struct revalidator *revalidator, struct list *udumps) for (i = 0; i < n_ops; i++) { struct dpif_flow_stats push, *stats, *ukey_stats; - ukey_stats = &ops[i].ukey_stats; + ukey_stats = &ops[i].ukey->stats; stats = ops[i].op.u.flow_del.stats; push.used = MAX(stats->used, ukey_stats->used); push.tcp_flags = stats->tcp_flags | ukey_stats->tcp_flags; @@ -1489,6 +1485,17 @@ revalidate_udumps(struct revalidator *revalidator, struct list *udumps) } } + for (i = 0; i < n_ops; i++) { + struct udpif_key *ukey = ops[i].ukey; + + /* Look up the ukey to prevent double-free if the datapath dumps the + * same flow twice. */ + ukey = ukey_lookup(revalidator, ops[i].udump); + if (ukey) { + ukey_delete(revalidator, ukey); + } + } + LIST_FOR_EACH_SAFE (udump, next_udump, list_node, udumps) { list_remove(&udump->list_node); free(udump); -- 1.7.9.5 _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
