Thanks for reporting the bug. I'll look into backporting the fix.
On Wed, Sep 17, 2014 at 2:17 PM, Anup Khadka <[email protected]> wrote: > Yes, I got a crash with 100 rules which led me to inspect the code. > > The collect_rule function inside collect_rules_loose returns > OFPROTO_POSTPONE if rule->pending is non-zero (this is possible if the > ofproto vendor class in not done inserting the rules). > > In such cases, collect_rules_loose function will call a free on rules. > > Without your commit in July, delete_flows_loose will call free again on the > rules structure. > > This is what was happening in my code base too. I was using OVS 2.1. > > Thanks, > Anup > > > > > > > On Wed, Sep 17, 2014 at 5:01 PM, Ben Pfaff <[email protected]> wrote: >> >> According to the commit message, the bug could not cause a real problem >> in practice. Do you see a way that it could? >> >> On Wed, Sep 17, 2014 at 04:50:04PM -0400, Anup Khadka wrote: >> > Looks like this code was added in July: >> > https://github.com/openvswitch/ovs/commit/bfd3dbf6a0c978ceb20faf292bca51 >> > 3a63e2b68c >> > I was using an older code-base. >> > Thanks, >> > Anup >> > >> > On Wed, Sep 17, 2014 at 4:37 PM, Ben Pfaff <[email protected]> wrote: >> > >> > > On Wed, Sep 17, 2014 at 02:58:50PM -0400, Anup Khadka wrote: >> > > > On Tue, Sep 16, 2014 at 3:30 PM, Anup Khadka <[email protected]> >> > > wrote: >> > > > >> > > > > It looks like OVS tries to double-free in delete_flows_loose if >> > > > > the >> > > > > rules->rules (inside struct rule_collection *rules is not equal to >> > > > > rules->stub). >> > > > > >> > > > > A little more detail: >> > > > > In the function delete_flows_loose, the call to the function >> > > > > collect_rules_loose takes care of freeing rules (again struct >> > > > > rule_collection *rules) if there is any error while collecting the >> > > rule. >> > > > > >> > > > > The function returns back to delete_flows_loose where it calls >> > > > > rule_collection_destroy again. >> > > > > >> > > > > Because rules->rules is still not rules->stab, it attempts to free >> > > > > the >> > > > > rules structure again, resulting in a double-free. >> > > > > >> > > > > Perhaps rules->rules can be set to rules->stab inside >> > > > > rule_collection_destroy function after its freed. Or perhaps, >> > > > > rule_collection_destroy should only be called from >> > > > > delete_flows_loose >> > > if >> > > > > there is no error, or perhaps collect_rules_loose should not take >> > > > > care >> > > of >> > > > > freeing the data structure. >> > > >> > > rule_collection_destroy() already reinitializes 'rules' after it >> > > destroys it: >> > > >> > > void >> > > rule_collection_destroy(struct rule_collection *rules) >> > > { >> > > if (rules->rules != rules->stub) { >> > > free(rules->rules); >> > > } >> > > >> > > /* Make repeated destruction harmless. */ >> > > rule_collection_init(rules); >> > > } >> > > > > -- "I don't normally do acked-by's. I think it's my way of avoiding getting blamed when it all blows up." Andrew Morton _______________________________________________ discuss mailing list [email protected] http://openvswitch.org/mailman/listinfo/discuss
