[ovs-dev] [PATCH] ofproto-dpif: Keep subfacets longer to avoid assert-fail in facet_account().

[Ben Pfaff]

If a subfacet expired when its facet still had statistics that had not
yet been pushed into the rule, and the facet either used the "normal"
action or the bridge contained a bond port, then facet_account() would
be called after the last subfacet was removed from its facet's list of
subfacets, triggering an assertion failure in list_front().

This fixes the problem by always running facet_flush_stats() (which calls
facet_account()) before deleting the last subfacet from a facet.

This problem took a while to surface because subfacets usually expire only
long after their statistics have been pushed into the rule.

Signed-off-by: Ben Pfaff <blp at nicira.com>
Reported-by: Mike Kruze <mkruze at nicira.com>
Bug #9074.
---
 AUTHORS                |    1 +
 ofproto/ofproto-dpif.c |   23 +++++++++++++++++++----
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/AUTHORS b/AUTHORS
index 821f780..631dfec 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -100,6 +100,7 @@ Michael A. Collins      mike.a.collins at ark-net.org
 Michael Hu              mhu at nicira.com
 Michael Mao             mmao at nicira.com
 Mike Bursell            mike.bursell at citrix.com
+Mike Kruze              mkruze at nicira.com
 Murphy McCauley         murphy.mccauley at gmail.com
 Mikael Doverhag         mdoverhag at nicira.com
 Niklas Andersson        nandersson at nicira.com
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index baa191e..fe99d70 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -3231,12 +3231,25 @@ facet_remove(struct ofproto_dpif *ofproto, struct facet *facet)
 {
     struct subfacet *subfacet, *next_subfacet;
 
+    assert(!list_is_empty(&facet->subfacets));
+
+    /* First uninstall all of the subfacets to get final statistics. */
+    LIST_FOR_EACH (subfacet, list_node, &facet->subfacets) {
+        subfacet_uninstall(ofproto, subfacet);
+    }
+
+    /* Flush the final stats to the rule.
+     *
+     * This might require us to have at least one subfacet around so that we
+     * can use its actions for accounting in facet_account(), which is why we
+     * have uninstalled but not yet destroyed the subfacets. */
+    facet_flush_stats(ofproto, facet);
+
+    /* Now we're really all done so destroy everything. */
     LIST_FOR_EACH_SAFE (subfacet, next_subfacet, list_node,
                         &facet->subfacets) {
         subfacet_destroy__(ofproto, subfacet);
     }
-
-    facet_flush_stats(ofproto, facet);
     hmap_remove(&ofproto->facets, &facet->hmap_node);
     list_remove(&facet->list_node);
     facet_free(facet);
@@ -3711,9 +3724,11 @@ subfacet_destroy(struct ofproto_dpif *ofproto, struct subfacet *subfacet)
 {
     struct facet *facet = subfacet->facet;
 
-    subfacet_destroy__(ofproto, subfacet);
-    if (list_is_empty(&facet->subfacets)) {
+    if (list_is_singleton(&facet->subfacets)) {
+        /* facet_remove() needs at least one subfacet (it will remove it). */
         facet_remove(ofproto, facet);
+    } else {
+        subfacet_destroy__(ofproto, subfacet);
     }
 }
 
-- 
1.7.2.5