ofproto-dpif: Fix VLAN and other field handling in OFPP_NORMAL.

This is a nontrivial cross-port of commit 823518f1f "ofproto-dpif: Fix VLAN
and other field handling in OFPP_NORMAL" from the "master" branch.

compose_actions(), which is part of the OFPP_NORMAL implementation, had
multiple flaws that this commit corrects.

First, it did not commit changes made to the flow by actions preceding the
output to OFPP_NORMAL.  This means that, for example, if an OpenFlow action
to modify an L2 or L3 header preceded the output to OFPP_NORMAL, then
OFPP_NORMAL would output its packets without those changes.

Second, it did not update the action_xlate_ctx's notion of the VLAN that
was currently set, in the cases where it modified the current VLAN.  This
means that actions following the output to OFPP_NORMAL could output to
unexpected VLANs.

Bug #6001.
Reported-by: Michael Mao <mmao@nicira.com>

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 005daf4..e894fd6 100644 (file)
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2814,6 +2814,23 @@ static void do_xlate_actions(const union ofp_action *in, size_t n_in,
                              struct action_xlate_ctx *ctx);
 static void xlate_normal(struct action_xlate_ctx *);
 
+static void
+commit_vlan_tci(struct action_xlate_ctx *ctx, ovs_be16 vlan_tci)
+{
+    struct flow *base = &ctx->base_flow;
+    struct ofpbuf *odp_actions = ctx->odp_actions;
+
+    if (base->vlan_tci != vlan_tci) {
+        if (!(vlan_tci & htons(VLAN_CFI))) {
+            nl_msg_put_flag(odp_actions, ODP_ACTION_ATTR_STRIP_VLAN);
+        } else {
+            nl_msg_put_be16(odp_actions, ODP_ACTION_ATTR_SET_DL_TCI,
+                            vlan_tci & ~htons(VLAN_CFI));
+        }
+        base->vlan_tci = vlan_tci;
+    }
+}
+
 static void
 commit_odp_actions(struct action_xlate_ctx *ctx)
 {
@@ -2841,15 +2858,7 @@ commit_odp_actions(struct action_xlate_ctx *ctx)
         base->nw_tos = flow->nw_tos;
     }
 
-    if (base->vlan_tci != flow->vlan_tci) {
-        if (!(flow->vlan_tci & htons(VLAN_CFI))) {
-            nl_msg_put_flag(odp_actions, ODP_ACTION_ATTR_STRIP_VLAN);
-        } else {
-            nl_msg_put_be16(odp_actions, ODP_ACTION_ATTR_SET_DL_TCI,
-                            flow->vlan_tci & ~htons(VLAN_CFI));
-        }
-        base->vlan_tci = flow->vlan_tci;
-    }
+    commit_vlan_tci(ctx, flow->vlan_tci);
 
     if (base->tp_src != flow->tp_src) {
         nl_msg_put_be16(odp_actions, ODP_ACTION_ATTR_SET_TP_SRC, flow->tp_src);
@@ -3577,8 +3586,12 @@ compose_actions(struct action_xlate_ctx *ctx, uint16_t vlan,
     dst_set_init(&set);
     compose_dsts(ctx, vlan, in_bundle, out_bundle, &set);
     compose_mirror_dsts(ctx, vlan, in_bundle, &set);
+    if (!set.n) {
+        return;
+    }
 
     /* Output all the packets we can without having to change the VLAN. */
+    commit_odp_actions(ctx);
     initial_vlan = vlan_tci_to_vid(ctx->flow.vlan_tci);
     if (initial_vlan == 0) {
         initial_vlan = OFP_VLAN_NONE;
@@ -3598,15 +3611,15 @@ compose_actions(struct action_xlate_ctx *ctx, uint16_t vlan,
             continue;
         }
         if (dst->vlan != cur_vlan) {
-            if (dst->vlan == OFP_VLAN_NONE) {
-                nl_msg_put_flag(ctx->odp_actions, ODP_ACTION_ATTR_STRIP_VLAN);
-            } else {
-                ovs_be16 tci;
-                tci = htons(dst->vlan & VLAN_VID_MASK);
-                tci |= ctx->flow.vlan_tci & htons(VLAN_PCP_MASK);
-                nl_msg_put_be16(ctx->odp_actions,
-                                ODP_ACTION_ATTR_SET_DL_TCI, tci);
+            ovs_be16 tci;
+
+            tci = htons(dst->vlan == OFP_VLAN_NONE ? 0 : dst->vlan);
+            tci |= ctx->flow.vlan_tci & htons(VLAN_PCP_MASK);
+            if (tci) {
+                tci |= htons(VLAN_CFI);
             }
+            commit_vlan_tci(ctx, tci);
+
             cur_vlan = dst->vlan;
         }
         nl_msg_put_u32(ctx->odp_actions,