ofproto-dpif: Fix use-after-free for OFPP_CONTROLLER flows.
[sliver-openvswitch.git] / ofproto / ofproto-dpif.c
index 88032bf..7eb8b41 100644 (file)
@@ -318,7 +318,7 @@ static bool execute_controller_action(struct ofproto_dpif *,
                                       const struct flow *,
                                       const struct nlattr *odp_actions,
                                       size_t actions_len,
-                                      struct ofpbuf *packet);
+                                      struct ofpbuf *packet, bool clone);
 
 static void facet_flush_stats(struct ofproto_dpif *, struct facet *);
 
@@ -373,8 +373,7 @@ static struct subfacet *subfacet_create(struct ofproto_dpif *, struct facet *,
                                         const struct nlattr *key,
                                         size_t key_len, ovs_be16 initial_tci);
 static struct subfacet *subfacet_find(struct ofproto_dpif *,
-                                      const struct nlattr *key, size_t key_len,
-                                      const struct flow *);
+                                      const struct nlattr *key, size_t key_len);
 static void subfacet_destroy(struct ofproto_dpif *, struct subfacet *);
 static void subfacet_destroy__(struct ofproto_dpif *, struct subfacet *);
 static void subfacet_reset_dp_stats(struct subfacet *,
@@ -2476,7 +2475,7 @@ handle_flow_miss(struct ofproto_dpif *ofproto, struct flow_miss *miss,
         }
         if (!execute_controller_action(ofproto, &facet->flow,
                                        subfacet->actions,
-                                       subfacet->actions_len, packet)) {
+                                       subfacet->actions_len, packet, true)) {
             struct flow_miss_op *op = &ops[(*n_ops)++];
             struct dpif_execute *execute = &op->dpif_op.execute;
 
@@ -2573,6 +2572,7 @@ handle_miss_upcalls(struct ofproto_dpif *ofproto, struct dpif_upcall *upcalls,
                                                 upcall->key, upcall->key_len,
                                                 &flow, &initial_tci);
         if (fitness == ODP_FIT_ERROR) {
+            ofpbuf_delete(upcall->packet);
             continue;
         }
         flow_extract(upcall->packet, flow.priority, flow.tun_id,
@@ -2649,6 +2649,7 @@ handle_userspace_upcall(struct ofproto_dpif *ofproto,
                                             upcall->key_len, &flow,
                                             &initial_tci);
     if (fitness == ODP_FIT_ERROR) {
+        ofpbuf_delete(upcall->packet);
         return;
     }
 
@@ -2664,6 +2665,7 @@ handle_userspace_upcall(struct ofproto_dpif *ofproto,
                               &flow, false);
     } else {
         VLOG_WARN_RL(&rl, "invalid user cookie : 0x%"PRIx64, upcall->userdata);
+        ofpbuf_delete(upcall->packet);
     }
 }
 
@@ -2782,16 +2784,9 @@ update_stats(struct ofproto_dpif *p)
 
     dpif_flow_dump_start(&dump, p->dpif);
     while (dpif_flow_dump_next(&dump, &key, &key_len, NULL, NULL, &stats)) {
-        enum odp_key_fitness fitness;
         struct subfacet *subfacet;
-        struct flow flow;
-
-        fitness = odp_flow_key_to_flow(key, key_len, &flow);
-        if (fitness == ODP_FIT_ERROR) {
-            continue;
-        }
 
-        subfacet = subfacet_find(p, key, key_len, &flow);
+        subfacet = subfacet_find(p, key, key_len);
         if (subfacet && subfacet->installed) {
             struct facet *facet = subfacet->facet;
 
@@ -2815,9 +2810,18 @@ update_stats(struct ofproto_dpif *p)
             facet_account(p, facet);
             facet_push_stats(facet);
         } else {
+            if (!VLOG_DROP_WARN(&rl)) {
+                struct ds s;
+
+                ds_init(&s);
+                odp_flow_key_format(key, key_len, &s);
+                VLOG_WARN("unexpected flow from datapath %s", ds_cstr(&s));
+                ds_destroy(&s);
+            }
+
+            COVERAGE_INC(facet_unexpected);
             /* There's a flow in the datapath that we know nothing about, or a
              * flow that shouldn't be installed but was anyway.  Delete it. */
-            COVERAGE_INC(facet_unexpected);
             dpif_flow_del(p->dpif, key, key_len, NULL);
         }
     }
@@ -2995,11 +2999,17 @@ facet_free(struct facet *facet)
     free(facet);
 }
 
+/* If the 'actions_len' bytes of actions in 'odp_actions' are just a single
+ * OVS_ACTION_ATTR_USERSPACE action, executes it internally and returns true.
+ * Otherwise, returns false without doing anything.
+ *
+ * If 'clone' is true, the caller always retains ownership of 'packet'.
+ * Otherwise, ownership is transferred to this function if it returns true. */
 static bool
 execute_controller_action(struct ofproto_dpif *ofproto,
                           const struct flow *flow,
                           const struct nlattr *odp_actions, size_t actions_len,
-                          struct ofpbuf *packet)
+                          struct ofpbuf *packet, bool clone)
 {
     if (actions_len
         && odp_actions->nla_type == OVS_ACTION_ATTR_USERSPACE
@@ -3015,7 +3025,7 @@ execute_controller_action(struct ofproto_dpif *ofproto,
 
         nla = nl_attr_find_nested(odp_actions, OVS_USERSPACE_ATTR_USERDATA);
         send_packet_in_action(ofproto, packet, nl_attr_get_u64(nla), flow,
-                              false);
+                              clone);
         return true;
     } else {
         return false;
@@ -3036,7 +3046,7 @@ execute_odp_actions(struct ofproto_dpif *ofproto, const struct flow *flow,
     int error;
 
     if (execute_controller_action(ofproto, flow, odp_actions, actions_len,
-                                  packet)) {
+                                  packet, false)) {
         return true;
     }
 
@@ -3508,12 +3518,18 @@ subfacet_create(struct ofproto_dpif *ofproto, struct facet *facet,
  * 'flow'.  Returns the subfacet if one exists, otherwise NULL. */
 static struct subfacet *
 subfacet_find(struct ofproto_dpif *ofproto,
-              const struct nlattr *key, size_t key_len,
-              const struct flow *flow)
+              const struct nlattr *key, size_t key_len)
 {
     uint32_t key_hash = odp_flow_key_hash(key, key_len);
+    enum odp_key_fitness fitness;
+    struct flow flow;
+
+    fitness = odp_flow_key_to_flow(key, key_len, &flow);
+    if (fitness == ODP_FIT_ERROR) {
+        return NULL;
+    }
 
-    return subfacet_find__(ofproto, key, key_len, key_hash, flow);
+    return subfacet_find__(ofproto, key, key_len, key_hash, &flow);
 }
 
 /* Uninstalls 'subfacet' from the datapath, if it is installed, removes it from
@@ -4026,165 +4042,6 @@ fix_sflow_action(struct action_xlate_ctx *ctx)
     cookie->vlan_tci = base->vlan_tci;
 }
 
-static void
-commit_set_action(struct ofpbuf *odp_actions, enum ovs_key_attr key_type,
-                  const void *key, size_t key_size)
-{
-    size_t offset = nl_msg_start_nested(odp_actions, OVS_ACTION_ATTR_SET);
-    nl_msg_put_unspec(odp_actions, key_type, key, key_size);
-    nl_msg_end_nested(odp_actions, offset);
-}
-
-static void
-commit_set_tun_id_action(const struct flow *flow, struct flow *base,
-                         struct ofpbuf *odp_actions)
-{
-    if (base->tun_id == flow->tun_id) {
-        return;
-    }
-    base->tun_id = flow->tun_id;
-
-    commit_set_action(odp_actions, OVS_KEY_ATTR_TUN_ID,
-                      &base->tun_id, sizeof(base->tun_id));
-}
-
-static void
-commit_set_ether_addr_action(const struct flow *flow, struct flow *base,
-                             struct ofpbuf *odp_actions)
-{
-    struct ovs_key_ethernet eth_key;
-
-    if (eth_addr_equals(base->dl_src, flow->dl_src) &&
-        eth_addr_equals(base->dl_dst, flow->dl_dst)) {
-        return;
-    }
-
-    memcpy(base->dl_src, flow->dl_src, ETH_ADDR_LEN);
-    memcpy(base->dl_dst, flow->dl_dst, ETH_ADDR_LEN);
-
-    memcpy(eth_key.eth_src, base->dl_src, ETH_ADDR_LEN);
-    memcpy(eth_key.eth_dst, base->dl_dst, ETH_ADDR_LEN);
-
-    commit_set_action(odp_actions, OVS_KEY_ATTR_ETHERNET,
-                      &eth_key, sizeof(eth_key));
-}
-
-static void
-commit_vlan_action(const struct flow *flow, struct flow *base,
-                   struct ofpbuf *odp_actions)
-{
-    if (base->vlan_tci == flow->vlan_tci) {
-        return;
-    }
-
-    if (base->vlan_tci & htons(VLAN_CFI)) {
-        nl_msg_put_flag(odp_actions, OVS_ACTION_ATTR_POP_VLAN);
-    }
-
-    if (flow->vlan_tci & htons(VLAN_CFI)) {
-        struct ovs_action_push_vlan vlan;
-
-        vlan.vlan_tpid = htons(ETH_TYPE_VLAN);
-        vlan.vlan_tci = flow->vlan_tci;
-        nl_msg_put_unspec(odp_actions, OVS_ACTION_ATTR_PUSH_VLAN,
-                          &vlan, sizeof vlan);
-    }
-    base->vlan_tci = flow->vlan_tci;
-}
-
-static void
-commit_set_nw_action(const struct flow *flow, struct flow *base,
-                     struct ofpbuf *odp_actions)
-{
-    struct ovs_key_ipv4 ipv4_key;
-
-    if (base->dl_type != htons(ETH_TYPE_IP) ||
-        !base->nw_src || !base->nw_dst) {
-        return;
-    }
-
-    if (base->nw_src == flow->nw_src &&
-        base->nw_dst == flow->nw_dst &&
-        base->nw_tos == flow->nw_tos &&
-        base->nw_ttl == flow->nw_ttl &&
-        base->nw_frag == flow->nw_frag) {
-        return;
-    }
-
-    ipv4_key.ipv4_src = base->nw_src = flow->nw_src;
-    ipv4_key.ipv4_dst = base->nw_dst = flow->nw_dst;
-    ipv4_key.ipv4_tos = base->nw_tos = flow->nw_tos;
-    ipv4_key.ipv4_ttl = base->nw_ttl = flow->nw_ttl;
-    ipv4_key.ipv4_proto = base->nw_proto;
-    ipv4_key.ipv4_frag = (base->nw_frag == 0 ? OVS_FRAG_TYPE_NONE
-                          : base->nw_frag == FLOW_NW_FRAG_ANY
-                          ? OVS_FRAG_TYPE_FIRST : OVS_FRAG_TYPE_LATER);
-
-    commit_set_action(odp_actions, OVS_KEY_ATTR_IPV4,
-                      &ipv4_key, sizeof(ipv4_key));
-}
-
-static void
-commit_set_port_action(const struct flow *flow, struct flow *base,
-                       struct ofpbuf *odp_actions)
-{
-    if (!base->tp_src || !base->tp_dst) {
-        return;
-    }
-
-    if (base->tp_src == flow->tp_src &&
-        base->tp_dst == flow->tp_dst) {
-        return;
-    }
-
-    if (flow->nw_proto == IPPROTO_TCP) {
-        struct ovs_key_tcp port_key;
-
-        port_key.tcp_src = base->tp_src = flow->tp_src;
-        port_key.tcp_dst = base->tp_dst = flow->tp_dst;
-
-        commit_set_action(odp_actions, OVS_KEY_ATTR_TCP,
-                          &port_key, sizeof(port_key));
-
-    } else if (flow->nw_proto == IPPROTO_UDP) {
-        struct ovs_key_udp port_key;
-
-        port_key.udp_src = base->tp_src = flow->tp_src;
-        port_key.udp_dst = base->tp_dst = flow->tp_dst;
-
-        commit_set_action(odp_actions, OVS_KEY_ATTR_UDP,
-                          &port_key, sizeof(port_key));
-    }
-}
-
-static void
-commit_set_priority_action(const struct flow *flow, struct flow *base,
-                           struct ofpbuf *odp_actions)
-{
-    if (base->priority == flow->priority) {
-        return;
-    }
-    base->priority = flow->priority;
-
-    commit_set_action(odp_actions, OVS_KEY_ATTR_PRIORITY,
-                      &base->priority, sizeof(base->priority));
-}
-
-static void
-commit_odp_actions(struct action_xlate_ctx *ctx)
-{
-    const struct flow *flow = &ctx->flow;
-    struct flow *base = &ctx->base_flow;
-    struct ofpbuf *odp_actions = ctx->odp_actions;
-
-    commit_set_tun_id_action(flow, base, odp_actions);
-    commit_set_ether_addr_action(flow, base, odp_actions);
-    commit_vlan_action(flow, base, odp_actions);
-    commit_set_nw_action(flow, base, odp_actions);
-    commit_set_port_action(flow, base, odp_actions);
-    commit_set_priority_action(flow, base, odp_actions);
-}
-
 static void
 compose_output_action__(struct action_xlate_ctx *ctx, uint16_t ofp_port,
                         bool check_stp)
@@ -4219,7 +4076,7 @@ compose_output_action__(struct action_xlate_ctx *ctx, uint16_t ofp_port,
     if (out_port != odp_port) {
         ctx->flow.vlan_tci = htons(0);
     }
-    commit_odp_actions(ctx);
+    commit_odp_actions(&ctx->flow, &ctx->base_flow, ctx->odp_actions);
     nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_OUTPUT, out_port);
 
     ctx->sflow_odp_port = odp_port;
@@ -4330,7 +4187,7 @@ compose_controller_action(struct action_xlate_ctx *ctx, int len)
 {
     struct user_action_cookie cookie;
 
-    commit_odp_actions(ctx);
+    commit_odp_actions(&ctx->flow, &ctx->base_flow, ctx->odp_actions);
     cookie.type = USER_ACTION_COOKIE_CONTROLLER;
     cookie.data = len;
     cookie.n_output = 0;
@@ -5081,7 +4938,9 @@ add_mirror_actions(struct action_xlate_ctx *ctx, const struct flow *orig_flow)
         }
 
         ofport = get_odp_port(ofproto, nl_attr_get_u32(a));
-        mirrors |= ofport ? ofport->bundle->dst_mirrors : 0;
+        if (ofport && ofport->bundle) {
+            mirrors |= ofport->bundle->dst_mirrors;
+        }
     }
 
     if (!mirrors) {