On Mon, Nov 18, 2013 at 03:48:29PM +0900, Simon Horman wrote: > On Fri, Nov 15, 2013 at 02:33:14PM -0800, Ben Pfaff wrote: > > Until now ofpacts_check() has been told either to enforce consistency or > > not, but that means that the caller has to know exactly what protocol is > > going to be in use (because some protocols require consistency to be > > enforced and others don't). This commit changes ofpacts_check() to just > > rule out protocols that require enforcement when it detects > > inconsistencies. > > > > Signed-off-by: Ben Pfaff <b...@nicira.com> > > Hi Ben, > > this approach seems reasonable to me. > > The problem I see with verification of VLAN actions that follow push_mpls > when OpenFlow1.3 is not that different protocols require consistency to > enforced while others don't. Rather the problem I as is that what is > consistent is different for pre-OpenFlow1.3 and OpenFlow1.3+ tag ordering. > > I believe that logic to handle that difference can be built on top of your > patch and what follows is a prototype of such logic. I'm not sure that this > is the direction you want us to take so I would appreciate feedback on this > approach.
Hi Ben, I know this is rather complex and becoming rather drawn out. But if you could take a few (maybe quite a few) moments to look over this I would be most grateful. > > From: Simon Horman <ho...@verge.net.au> > > [PATCH] [RFC] Consistency checking of OF1.3 VLAN actions after mpls_push > > *** Do not apply. *** > > The aim of this patch is to support verification of VLAN > actions after an mpls_push action for OpenFlow1.3. This supplements > existing support for verifying these actions for pre-OpenFlow1.3. > > In OpenFlow1.1 and 1.2 MPLS tags are pushed after any VLAN tags that > immediately follow the ethernet header. This is pre-OpenFlow1.3 tag > ordering. Open vSwitch also uses this ordering when supporting MPLS > actions via Nicira extensions to OpenFlow1.0. > > When using pre-OpenFlow1.3 tag ordering an MPLS push action does not > affect the VLANs of a packet. If VLAN tags are present immediately after > the ethernet header then they remain present there. > > In of OpenFlow1.3+ MPLS LSEs are pushed before any VLAN tags that > immediately follow the ethernet header. This is OpenFlow1.3+ tag > ordering. > > When using OpenFlow1.3+ tag ordering an MPLS push action affects the > VLANs of a packet as any VLAN tags previously present after the ethernet > header are moved to be immediately after the newly pushed MPLS LSE. Thus > for the purpose of action consistency checking a packet may be changed > from a VLAN packet to a non-VLAN packet. > > In this way the effective value of the VLAN TCI of a packet may differ > after an MPLS push depending on the OpenFlow version in use. > > Signed-off-by: Simon Horman <ho...@verge.net.au> > > --- > > This patch is an RFC intended to further discussion. It builds and depends > upon "ofp-actions: Make ofpacts_check() report consistency for all > protocols". > > It also requires a revised version of "odp: Allow VLAN actions after MPLS > actions". I have held off on publishing that patch or a revised version of > the MPLS patchset it is a part of as I believe this proposal can be > discussed without those details. > --- > lib/ofp-actions.c | 130 > ++++++++++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 116 insertions(+), 14 deletions(-) > > diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c > index e07ea1d..47244f5 100644 > --- a/lib/ofp-actions.c > +++ b/lib/ofp-actions.c > @@ -1879,15 +1879,102 @@ inconsistent_match(enum ofputil_protocol > *usable_protocols) > *usable_protocols &= OFPUTIL_P_OF10_ANY; > } > > +/* Checks the consistency of a VLAN action by checking the state of the > + * VLAN_CFI bit of the VLAN TCI against 'desired_cfi' and updating > + * '*usable_protocols' accordingly. > + * > + * 'pre_of13_vlan_tci' is the VLAN TCI to check against for pre-OpenFlow1.3 > + * protocols. 'of13_vlan_tci' is the VLAN TCI to check against for > + * OpenFlow1.3+ protocols. > + * > + * If the desired_cfi bit is in the desired state for any usable protocol > + * then true is returned. Else false is returned. > + * > + * The check for consistent VLAN actions may depend on the OpenFlow version > + * used and whether the VLAN action is preceded by an MPLS push action or > + * not. > + * > + * In OpenFlow1.1 and 1.2 MPLS tags are pushed after any VLAN tags that > + * immediately follow the ethernet header. This is pre-OpenFlow1.3 tag > + * ordering. Open vSwitch also uses this ordering when supporting MPLS > + * actions via Nicira extensions to OpenFlow1.0. > + * > + * When using pre-OpenFlow1.3 tag ordering an MPLS push action does not > + * affect the VLANs of a packet. If VLAN tags are present immediately after > + * the ethernet header then they remain present there. > + * > + * In of OpenFlow1.3+ MPLS LSEs are pushed before any VLAN tags that > + * immediately follow the ethernet header. This is OpenFlow1.3+ tag > + * ordering. > + * > + * When using OpenFlow1.3+ tag ordering an MPLS push action affects the > + * VLANs of a packet as any VLAN tags previously present after the ethernet > + * header are moved to be immediately after the newly pushed MPLS LSE. Thus > + * for the purpose of action consistency checking a packet may be changed > + * from a VLAN packet to a non-VLAN packet. > + * > + * In this way the effective value of the VLAN TCI of a packet may differ > + * after an MPLS push depending on the OpenFlow version in use. This > + * corresponds to the 'pre_of13_vlan_tci' and 'of13_vlan_tci' parameters of > + * this function. */ > +static bool > +check_vlan_action(enum ofputil_protocol *usable_protocols, > + ovs_be32 pre_of13_vlan_tci, ovs_be32 of13_vlan_tci, > + bool desired_cfi) > +{ > + bool ok = false; > + ovs_be16 tci; > + > + if (desired_cfi) { > + tci = htons(VLAN_CFI); > + } else { > + tci = htons(0); > + } > + > + if (*usable_protocols & OFPUTIL_P_OF13_UP) { > + if ((of13_vlan_tci & tci) != tci) { > + /* CFI miss-match for OpenFlow1.3+: clear those protocols */ > + *usable_protocols &= ~OFPUTIL_P_OF13_UP; > + } else { > + /* CFI bit is valid for a usable protocol: > + * eventually return true */ > + ok = true; > + } > + } > + > + if (*usable_protocols & ~OFPUTIL_P_OF13_UP) { > + if ((pre_of13_vlan_tci & tci) != tci) { > + enum ofputil_protocol of13_up; > + > + /* CFI miss-match for pre-OpenFlow1.3: clear those protocols > + * except OFPUTIL_P_OF10_ANY as it allows inconsistency. */ > + of13_up = *usable_protocols & OFPUTIL_P_OF13_UP; > + inconsistent_match(usable_protocols); > + *usable_protocols &= of13_up; > + } else { > + /* CFI bit is valid for a usable protocol: > + * eventually return true */ > + ok = true; > + } > + } > + > + return ok; > +} > + > /* May modify flow->dl_type, flow->nw_proto and flow->vlan_tci, > * caller must restore them. > * > + * May also modify of13_vlan_tci which the caller should discard. On the > + * first call to ofpact_check__() of13_vlan_tci should be set to > + * flow->vlan_tci > + * > * Modifies some actions, filling in fields that could not be properly set > * without context. */ > static enum ofperr > ofpact_check__(enum ofputil_protocol *usable_protocols, struct ofpact *a, > struct flow *flow, ofp_port_t max_ports, > - uint8_t table_id, uint8_t n_tables) > + uint8_t table_id, uint8_t n_tables, > + ovs_be32 *of13_vlan_tci) > { > const struct ofpact_enqueue *enqueue; > const struct mf_field *mf; > @@ -1920,12 +2007,13 @@ ofpact_check__(enum ofputil_protocol > *usable_protocols, struct ofpact *a, > * OpenFlow 1.1+ if need be. */ > ofpact_get_SET_VLAN_VID(a)->flow_has_vlan = > (flow->vlan_tci & htons(VLAN_CFI)) == htons(VLAN_CFI); > - if (!(flow->vlan_tci & htons(VLAN_CFI)) && > - !ofpact_get_SET_VLAN_VID(a)->push_vlan_if_needed) { > - inconsistent_match(usable_protocols); > + if (!ofpact_get_SET_VLAN_VID(a)->push_vlan_if_needed) { > + check_vlan_action(usable_protocols, flow->vlan_tci, > + *of13_vlan_tci, true); > } > /* Temporary mark that we have a vlan tag. */ > flow->vlan_tci |= htons(VLAN_CFI); > + *of13_vlan_tci |= htons(VLAN_CFI); > return 0; > > case OFPACT_SET_VLAN_PCP: > @@ -1933,29 +2021,31 @@ ofpact_check__(enum ofputil_protocol > *usable_protocols, struct ofpact *a, > * OpenFlow 1.1+ if need be. */ > ofpact_get_SET_VLAN_PCP(a)->flow_has_vlan = > (flow->vlan_tci & htons(VLAN_CFI)) == htons(VLAN_CFI); > - if (!(flow->vlan_tci & htons(VLAN_CFI)) && > - !ofpact_get_SET_VLAN_PCP(a)->push_vlan_if_needed) { > - inconsistent_match(usable_protocols); > + if (!ofpact_get_SET_VLAN_PCP(a)->push_vlan_if_needed) { > + check_vlan_action(usable_protocols, flow->vlan_tci, > + *of13_vlan_tci, true); > } > /* Temporary mark that we have a vlan tag. */ > flow->vlan_tci |= htons(VLAN_CFI); > + *of13_vlan_tci |= htons(VLAN_CFI); > return 0; > > case OFPACT_STRIP_VLAN: > - if (!(flow->vlan_tci & htons(VLAN_CFI))) { > - inconsistent_match(usable_protocols); > - } > + check_vlan_action(usable_protocols, flow->vlan_tci, *of13_vlan_tci, > + true); > /* Temporary mark that we have no vlan tag. */ > - flow->vlan_tci = htons(0); > + *of13_vlan_tci = flow->vlan_tci = htons(0); > return 0; > > case OFPACT_PUSH_VLAN: > - if (flow->vlan_tci & htons(VLAN_CFI)) { > + if (!check_vlan_action(usable_protocols, flow->vlan_tci, > + *of13_vlan_tci, false)) { > /* Multiple VLAN headers not supported. */ > return OFPERR_OFPBAC_BAD_TAG; > } > /* Temporary mark that we have a vlan tag. */ > flow->vlan_tci |= htons(VLAN_CFI); > + *of13_vlan_tci |= htons(VLAN_CFI); > return 0; > > case OFPACT_SET_ETH_SRC: > @@ -2012,7 +2102,9 @@ ofpact_check__(enum ofputil_protocol *usable_protocols, > struct ofpact *a, > mf = ofpact_get_SET_FIELD(a)->field; > /* Require OXM_OF_VLAN_VID to have an existing VLAN header. */ > if (!mf_are_prereqs_ok(mf, flow) || > - (mf->id == MFF_VLAN_VID && !(flow->vlan_tci & htons(VLAN_CFI)))) > { > + (mf->id == MFF_VLAN_VID && > + !check_vlan_action(usable_protocols, flow->vlan_tci, > + *of13_vlan_tci, true))) { > VLOG_WARN_RL(&rl, "set_field %s lacks correct prerequisities", > mf->name); > return OFPERR_OFPBAC_MATCH_INCONSISTENT; > @@ -2025,6 +2117,7 @@ ofpact_check__(enum ofputil_protocol *usable_protocols, > struct ofpact *a, > /* The set field may add or remove the vlan tag, > * Mark the status temporarily. */ > flow->vlan_tci = ofpact_get_SET_FIELD(a)->value.be16; > + *of13_vlan_tci = ofpact_get_SET_FIELD(a)->value.be16; > } > return 0; > > @@ -2071,6 +2164,13 @@ ofpact_check__(enum ofputil_protocol > *usable_protocols, struct ofpact *a, > * Thus nothing can be assumed about the network protocol. > * Temporarily mark that we have no nw_proto. */ > flow->nw_proto = 0; > + > + /* In the case of OpenFlow1.3+ the MPLS LSE is pushed before > + * any VLAN tags which were previously present immediately > + * after the VLAN header. > + * Temporarily mark that there is no VLAN tag in the case > + * of OpenFlow1.3+ */ > + *of13_vlan_tci = htons(0); > return 0; > > case OFPACT_POP_MPLS: > @@ -2144,12 +2244,14 @@ ofpacts_check(struct ofpact ofpacts[], size_t > ofpacts_len, > struct ofpact *a; > ovs_be16 dl_type = flow->dl_type; > ovs_be16 vlan_tci = flow->vlan_tci; > + ovs_be32 of13_vlan_tci = flow->vlan_tci; > uint8_t nw_proto = flow->nw_proto; > enum ofperr error = 0; > > OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) { > error = ofpact_check__(usable_protocols, a, flow, > - max_ports, table_id, n_tables); > + max_ports, table_id, n_tables, > + &of13_vlan_tci); > if (error) { > break; > } > -- > 1.8.4 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev