ofproto-dpif: Keep subfacets longer to avoid assert-fail in facet_account().

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@nicira.com>
Reported-by: Mike Kruze <mkruze@nicira.com>
Bug #9074.

diff --git a/AUTHORS b/AUTHORS
index 02f4840..b50d6c7 100644 (file)
--- a/AUTHORS
+++ b/AUTHORS
@@ -93,6 +93,7 @@ Luiz Henrique Ozaki     luiz.ozaki@gmail.com
 Michael A. Collins      mike.a.collins@ark-net.org
 Michael Hu              mhu@nicira.com
 Michael Mao             mmao@nicira.com
+Mike Kruze              mkruze@nicira.com
 Murphy McCauley         murphy.mccauley@gmail.com
 Mikael Doverhag         mdoverhag@nicira.com
 Niklas Andersson        nandersson@nicira.com

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index cca23ca..09f4724 100644 (file)
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -3166,12 +3166,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);
@@ -3646,9 +3659,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);
     }
 }