[ovs-dev] [PATCH] ofp-actions: Account for OF version when enforcing action consistency

[Simon Horman]

On Wed, Nov 13, 2013 at 10:45:13AM -0800, Ben Pfaff wrote:
> On Tue, Nov 05, 2013 at 05:34:34PM +0900, Simon Horman wrote:
> > Some different versions of OpenFlow require different consistency
> > checking. This patch allows for three different variants to be checked.
> > 
> > 1. OpenFlow1.0
> > 
> >    This variant implicitly pushes a VLAN tag if one doesn't exist
> >    and an action to modify a VLAN tag is encountered.
> > 
> >    MPLS push is supported as a Nicira extension whose behaviour is
> >    the same as OpenFlow1.1 - 1.2.
> > 
> >    In practice Open vSwtich allows inconsistent OpenFlow 1.0 actions so
> >    this portion of the change is just for completeness. I do not believe it
> >    has any run-time affect. And I would not be opposed to folding this case
> >    into the handling of OpenFlow1.1 - 1.2 other than that I believe it will
> >    make the logic in parse_ofp_str__() and ofpact_check__() slightly less
> >    intuitive in parts.
> > 
> > 2. OpenFlow1.1 - 1.2
> > 
> >    This does not have the implicit VLAN tag push behaviour of OpenFlow1.0.
> > 
> >    An MPLS push action pushes an MPLS LSE after any VLAN tags that are
> >    present.
> > 
> > 3. OpenFlow1.3
> > 
> >    This does not have the implicit VLAN tag push behaviour of OpenFlow1.0.
> > 
> >    An MPLS push action pushes an MPLS LSE before any VLAN tags that are
> >    present.
> > 
> > ofpact_get_SET_VLAN_VID(a)->push_vlan_if_needed and similar devices
> > can't be used in order to effect this logic when ofpact_check__()
> > is called via parse_ofp_str() as they are not filled in for
> > specific OF versions at that time. Again, for OpenFlow1.0 and
> > thus push_vlan_if_needed I believe this is cosmetic. But that is not
> > the case for the MPLS portion of this change.
> > 
> > The net effect of this change on run-time is:
> > 
> > * To increase logging under some circumstances as ofpacts_check()
> >   may now be called up to four times instead of up to twice
> >   for each invocation of parse_ofp_str__()
> > 
> > * To disallow MPLS push actions on VLAN flows using OpenFlow1.3.
> >   These did not comply to the OpenFlow1.3 as they would push an MPLS LSE
> >   after any VLAN tags that were present. A subsequent patch will support
> >   them in a manner that complies with the OpenFlow1.3.
> > 
> > Signed-off-by: Simon Horman <horms at verge.net.au>
> > 
> > ---
> > 
> > This patch is targeted as a pre-requisite for v2.48 of
> > the "MPLS actions and matches" series. I am posting it separately
> > as I would like some feedback on the approach I have taken and
> > I believe it can be reviewed independently of the MPLS series.
> 
> This needs a respin:
> 
>     ../ofproto/ofproto-dpif.c:5495:67: error: too few arguments to function call, expected 8, have 7
>                                u16_to_ofp(ofproto->up.max_ports), 0, 0);
>                                                                       ^
>     ../lib/ofp-actions.h:609:1: note: 'ofpacts_check' declared here
>     enum ofperr ofpacts_check(enum ofp_version ofp_version,
>     ^
>     1 error generated.

Thanks, I will fix that.

> But I don't really understand your second point here:
> 
>   * To disallow MPLS push actions on VLAN flows using OpenFlow1.3.
>     These did not comply to the OpenFlow1.3 as they would push an MPLS LSE
>     after any VLAN tags that were present. A subsequent patch will support
>     them in a manner that complies with the OpenFlow1.3.
> 
> Do you mean that OF1.3 forbids push_mpls when a VLAN header is
> present?  If this is new to me.  Maybe you mean something else.
> Either way, can you explain further?

Sorry, that is rather bogus.
OF1.3 certainly doesn't forbid push_mpls in the presence of a VLAN header.

What follows is rather long but it tries to explain the crux
of the problem I am trying to solve: the motivation for this patch.

* To support verification of combinations of VLAN and MPLS actions
  using OpenFlow1.3 tag ordering.

  Currently Open vSwitch always uses the pre-OpenFlow1.3 tag ordering.
  That is an mpls_push action adds an MPLS LSE after any VLAN tags that
  immediately follow the ethernet header (dl type, source and destination
  address).

  The OpenFlow1.3+ tag order is different. In this case an mpls_push action
  will place an MPLS LSE immediately after the ethernet header and thus
  before any VLAN tags which previously occupied that position.  Support
  for this new OF1.3 tag order proposed as part "[PATCH v2.48 0/4] MPLS
  actions and matches".

  In the case of the pre-OpenFlow1.3 tag ordering an mpls_push action does
  not change whether a packet is a VLAN packet or not. This is because the
  MPLS LSE that is pushed always goes after any VLAN tags.  However, with
  the OpenFlow1.3+ tag ordering after an mpls_push any previously present
  VLAN tags will follow the new MPLS LSE and are thus are no longer
  directly after the ethernet header. I believe that from the point of view
  of verifying actions this means the packet is no longer a VLAN packet.

  For example: Using the pre-OpenFlow1.3 tag ordering on a packet with a
  VLAN tag the following is valid. It pushes an MPLS LSE after the existing
  VLAN tag and then updates that existing VLAN tag.

  push_mpls:0x8847,set_vlan_pcp:3

  But following my reasoning above it is not valid using the OpenFlow1.3
  tag ordering, unless a VLAN tag is implicitly pushed as a result of the
  set_vlan_pcp action (which may or may not be the case). Because after the
  push_mpls action there is no longer a VLAN tag present after the ethernet
  header.

  Conversely, even though pushing a VLAN onto a VLAN packet is currently
  prohibited by Open vSwitch the following actions may be applied to a VLAN
  packet with the OpenFlow1.3 tag ordering because after the mpls_push
  action there is no longer a VLAN tag present after the ethernet header.

  set_vlan_pcp:3,push_mpls:0x8847,push_vlan:0x8100

* This above creates an inconsistency. Using OpenFlow1.3+ tag ordering
  to verify actions while still always using pre-OpenFlow1.3 tag ordering.

  This could be resolved by moving the following portion of this
  patch, which would disable the behaviour described above,
  into the patch which adds support for OpenFlow1.3+ tag ordering.

-    case OFPACT_PUSH_MPLS:
-        flow->dl_type = ofpact_get_PUSH_MPLS(a)->ethertype;
+    case OFPACT_PUSH_MPLS: {
+        struct ofpact_push_mpls *oam = ofpact_get_PUSH_MPLS(a);
+
+        flow->dl_type = oam->ethertype;
+        if (ofp_version >= OFP13_VERSION) {
+            /* Temporary mark that we have no vlan tag. */
+            flow->vlan_tci = htons(0);
+        }
         return 0;
+    }

While writing the above I realised there may be a problem with my patch
in that I think that it allows the following on a VLAN packet
when using OpenFlow1.3:

  set_vlan_pcp:3,push_mpls:0x8847,pop_mpls:0x8100,push_vlan:0x8100

  which is the same as

  set_vlan_pcp:3,push_vlan:0x8100

  Which should be prohibited.