secchan: Validate subrules before attempting to dereference their super-rules.

rule_make_actions() is supposed to compose the datapath actions for an
exact-match rule, and to do so it needs to look up the super-rule (if the
rule is a subrule).  The "super" pointer might be set to UNKNOWN_SUPER,
though, and before this commit that would cause a segfault.

This commit modifies the callers of rule_make_actions() to ensure that
the rule passed in can never have a "super" of UNKNOWN_SUPER.  In most
cases, this was already impossible (e.g. we're passing in a new rule that
we just added to the table), but in two cases where the rule was obtained
from a bare classifier lookup we needed to validate the rule before
attempting to use it.

Fixes a crash reported by Keith.

diff --git a/secchan/ofproto.c b/secchan/ofproto.c
index 532f837727f061177871ae06f48b0e4e73a328a8..a4f68b01fd76d3dd21cba8478303935ec518c3cf 100644 (file)
--- a/secchan/ofproto.c
+++ b/secchan/ofproto.c
@@ -1255,6 +1255,7 @@ rule_has_out_port(const struct rule *rule, uint16_t out_port)
     return false;
 }
 
+/* The caller must ensure that rule->super != UNKNOWN_SUPER. */
 static void
 rule_make_actions(struct ofproto *p, struct rule *rule, bool revalidating,
                   struct odp_actions *actions)
@@ -1459,41 +1460,45 @@ struct action_xlate_ctx {
 static void do_xlate_actions(const union ofp_action *in, size_t n_in,
                              struct action_xlate_ctx *ctx);
 
-static void
-xlate_table_action(struct action_xlate_ctx *ctx, uint16_t in_port)
+static struct rule *
+lookup_valid_rule(struct ofproto *ofproto, const flow_t *flow)
 {
-    struct ofproto *p = ctx->ofproto;
     struct rule *rule;
-    flow_t flow;
-
-    if (ctx->recurse) {
-        return;
+    rule = rule_from_cls_rule(classifier_lookup(&ofproto->cls, flow));
+
+    /* The rule we found might not be valid, since we could be in need of
+     * revalidation.  If it is not valid, don't return it. */
+    if (rule
+        && rule->super
+        && (ofproto->need_revalidate || rule->super == UNKNOWN_SUPER)
+        && !revalidate_rule(ofproto, rule)) {
+        return NULL;
     }
 
-    flow = *ctx->flow;
-    flow.in_port = in_port;
+    return rule;
+}
 
-    rule = rule_from_cls_rule(classifier_lookup(&p->cls, &flow));
-    if (!rule) {
-        return;
-    } else if (rule->super && p->need_revalidate) {
-        /* This might be a subrule that is now invalid.  Revalidate it. */
-        if (!revalidate_rule(p, rule)) {
-            /* The subrule got deleted so we can optimize slightly by only
-             * looking through the wildcarded rules. */
-            rule = rule_from_cls_rule(classifier_lookup_wild(&p->cls, &flow));
-            if (!rule) {
-                return;
+static void
+xlate_table_action(struct action_xlate_ctx *ctx, uint16_t in_port)
+{
+    if (!ctx->recurse) {
+        struct rule *rule;
+        flow_t flow;
+
+        flow = *ctx->flow;
+        flow.in_port = in_port;
+
+        rule = lookup_valid_rule(ctx->ofproto, &flow);
+        if (rule) {
+            if (rule->super) {
+                rule = rule->super;
             }
+
+            ctx->recurse++;
+            do_xlate_actions(rule->actions, rule->n_actions, ctx);
+            ctx->recurse--;
         }
     }
-    if (rule->super) {
-        rule = rule->super;
-    }
-
-    ctx->recurse++;
-    do_xlate_actions(rule->actions, rule->n_actions, ctx);
-    ctx->recurse--;
 }
 
 static void
@@ -2569,7 +2574,7 @@ handle_odp_msg(struct ofproto *p, struct ofpbuf *packet)
     payload.size = msg->length - sizeof *msg;
     flow_extract(&payload, msg->port, &flow);
 
-    rule = rule_from_cls_rule(classifier_lookup(&p->cls, &flow));
+    rule = lookup_valid_rule(p, &flow);
     if (!rule) {
         struct ofport *port;