Hi,
this patch fixes a memory corruptor when a bridge containing rules with
meters is removed from the system. It was observed and fixed on
branch-2.5.
This was regtested by "make check", though without SSL and IPsec tests,
and the patch introduced no regressions. It also actually fixes the
corruptor in our system.
Please consider applying.
On master, I see that ofproto_rule_remove__ is called in
delete_flows_start__ instead of delete_flows_finish__. But otherwise
the sequencing is the same and I think the problem should exhibit on
master as well. We don't however have resources to forward-port our
provider and actually observe the issue in practice.
Thank you,
Petr Machata
--8<--------------------------------------------------------
An ofproto that's being destroyed could include rules with meters. Each
meter in turn holds a list of rules where that meter is attached. Rules
that are destroyed are removed from the list of rules at the meter
attached to the rule under destruction.
But the function ofproto_destroy sequences meter removal before a call
to ofproto_flush__, which takes care of rule removal. Thus at the time
that rules are removed from the list at a meter, that list is already
gone. This leads to invalid memory accesses.
The particular call chains are:
- ofproto_destroy calls meter_delete calls free
- ofproto_destroy calls ofproto_flush__ calls delete_flows__ calls
delete_flows_finish__ calls ofproto_rule_remove__ calls
list_remove(&rule->meter_list_node). rule->meter_list_node's next
references a free'd meter->rules.
To fix this problem, change the order in which rules and meters are
destroyed.
Signed-off-by: Amit Nishry <am...@mellanox.com>
Signed-off-by: Mykola Zhuravel <myk...@mellanox.com>
---
ofproto/ofproto.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index c6b802b..6cdfa8c 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -1590,6 +1590,8 @@ ofproto_destroy(struct ofproto *p, bool del)
return;
}
+ ofproto_flush__(p);
+
if (p->meters) {
meter_delete(p, 1, p->meter_features.max_meters);
p->meter_features.max_meters = 0;
@@ -1597,7 +1599,6 @@ ofproto_destroy(struct ofproto *p, bool del)
p->meters = NULL;
}
- ofproto_flush__(p);
HMAP_FOR_EACH_SAFE (ofport, next_ofport, hmap_node, &p->ports) {
ofport_destroy(ofport, del);
}
--
2.1.0
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
