[ovs-dev] [PATCH 1/2] ofp-actions: Make ofpacts_check() report consistency for all protocols.
Simon Horman
horms at verge.net.au
Wed Nov 20 08:19:25 UTC 2013
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 <blp at 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 <horms at 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 <horms at 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 at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
More information about the dev
mailing list