Petr, While deleting all the rules before deleting al the meters is more efficient, I think we should make the code more future proof by making sure we never delete a meter without deleting the rules referring to it.
Would you try the following patch to see if it fixes the problem you face: https://patchwork.ozlabs.org/patch/677164/ Regards, Jarno > On Sep 22, 2016, at 8:03 AM, Petr Machata <pe...@mellanox.com> wrote: > > 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 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
