From 823518f1f26c2320c35e762c9d1bd29e68f9c5b8 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Fri, 23 Sep 2011 17:11:49 -0700 Subject: [PATCH] ofproto-dpif: Fix VLAN and other field handling in OFPP_NORMAL. 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. Third, when it switched to VLAN 0, it unconditionally also stripped the whole 802.1Q header, so that if the packet originally was priority tagged, the output packet was not priority tagged. This is reasonable behavior, but it is a change in behavior from what previous releases of OVS did, so this commit reverts it. Based on a patch from and a conversation with Pravin Shelar . --- ofproto/ofproto-dpif.c | 56 +++++++++++++++++++++++++----------------- 1 file changed, 33 insertions(+), 23 deletions(-) diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 47a9d29ef..ec5e3be2e 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -2887,6 +2887,26 @@ 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, OVS_ACTION_ATTR_POP_VLAN); + } else { + if (base->vlan_tci != htons(0)) { + nl_msg_put_flag(odp_actions, OVS_ACTION_ATTR_POP_VLAN); + } + nl_msg_put_be16(odp_actions, OVS_ACTION_ATTR_PUSH_VLAN, + vlan_tci & ~htons(VLAN_CFI)); + } + base->vlan_tci = vlan_tci; + } +} + static void commit_odp_actions(struct action_xlate_ctx *ctx) { @@ -2914,18 +2934,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, OVS_ACTION_ATTR_POP_VLAN); - } else { - if (base->vlan_tci != htons(0)) { - nl_msg_put_flag(odp_actions, OVS_ACTION_ATTR_POP_VLAN); - } - nl_msg_put_be16(odp_actions, OVS_ACTION_ATTR_PUSH_VLAN, - 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, OVS_ACTION_ATTR_SET_TP_SRC, flow->tp_src); @@ -3741,8 +3750,13 @@ 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) { + dst_set_free(&set); + 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; @@ -3762,19 +3776,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, OVS_ACTION_ATTR_POP_VLAN); - } else { - ovs_be16 tci; + ovs_be16 tci; - if (cur_vlan != OFP_VLAN_NONE) { - nl_msg_put_flag(ctx->odp_actions, OVS_ACTION_ATTR_POP_VLAN); - } - tci = htons(dst->vlan & VLAN_VID_MASK); - tci |= ctx->flow.vlan_tci & htons(VLAN_PCP_MASK); - nl_msg_put_be16(ctx->odp_actions, - OVS_ACTION_ATTR_PUSH_VLAN, 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, -- 2.43.0