Re: [PATCH v2.49 1/6] ofp-actions: Consider L4 actions after mpls_push as inconsistent

[Simon Horman <horms@verge.net.au>]

On Fri, Nov 15, 2013 at 04:50:53PM +0900, Simon Horman wrote:
> After an mpls_push action the resulting packet is MPLS and
> the MPLS payload is opaque. Thus nothing can be assumed
> about the packets network protocol and it is inconsistent
> to apply L4 actions.
> 
> With regards to actions that affect the packet at other layers
> of the protocol stack:
> 
> * L3: The consistency of L3 actions should already be handled correctly
>   by virtue of the dl_type of the flow being temporarily altered
>   during consistency checking by both push_mpls and pop_mpls actions.
> 
> * MPLS: The consistency checking of MPLS actions appear to already be
>   handled correctly.
> 
> * VLAN: At this time Open vSwitch on mpls_push an MPLS LSE is always
>   added after any VLAN tags that follow the ethernet header.
>   That is the tag ordering defined prior to OpenFlow1.3. As such
>   VLAN actions should sill be equally valid before and after mpls_push
>   and mpls_pop actions.
> 
> * L2 actions are equally valid before and after mpls_push and mpls_pop actions.
> 
> Signed-off-by: Simon Horman <horms@verge.net.au>
> 
> ---
> v2.49
> * First post
> ---
>  lib/ofp-actions.c    |  9 ++++++++-
>  tests/ofp-actions.at | 12 ++++++++++++
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index abc9505..e4e05dd 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -1869,7 +1869,8 @@ ofpact_check_output_port(ofp_port_t port, ofp_port_t max_ports)
>      }
>  }
>  
> -/* May modify flow->dl_type and flow->vlan_tci, caller must restore them.
> +/* May modify flow->dl_type, flow->nw_protocol and flow->vlan_tci,

s/nw_protocol/nw_proto/

I will re-post.

> + * caller must restore them.
>   *
>   * Modifies some actions, filling in fields that could not be properly set
>   * without context. */
> @@ -2056,6 +2057,10 @@ ofpact_check__(struct ofpact *a, struct flow *flow,
>  
>      case OFPACT_PUSH_MPLS:
>          flow->dl_type = ofpact_get_PUSH_MPLS(a)->ethertype;
> +        /* The packet is now MPLS and the MPLS payload is opaque.
> +         * Thus nothing can be assumed about the network protocol.
> +         * Temporarily mark that we have no nw_proto. */
> +        flow->nw_proto = 0;
>          return 0;
>  
>      case OFPACT_POP_MPLS:
> @@ -2127,6 +2132,7 @@ 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;
> +    uint8_t nw_proto = flow->nw_proto;
>      enum ofperr error = 0;
>  
>      OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
> @@ -2139,6 +2145,7 @@ ofpacts_check(struct ofpact ofpacts[], size_t ofpacts_len,
>      /* Restore fields that may have been modified. */
>      flow->dl_type = dl_type;
>      flow->vlan_tci = vlan_tci;
> +    flow->nw_proto = nw_proto;
>      return error;
>  }
>  
> diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at
> index cdca8ca..08ebccf 100644
> --- a/tests/ofp-actions.at
> +++ b/tests/ofp-actions.at
> @@ -477,3 +477,15 @@ AT_CHECK(
>    [ovs-ofctl '-vPATTERN:console:%c|%p|%m' parse-ofp11-instructions < input.txt],
>    [0], [expout], [experr])
>  AT_CLEANUP
> +
> +AT_SETUP([ofp-actions - inconsistent MPLS actions])
> +OVS_VSWITCHD_START
> +dnl OK: Use fin_timeout action on TCP flow
> +AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn add-flow br0 'tcp actions=fin_timeout(idle_timeout=1)'])
> +dnl Bad: Use fin_timeout action on TCP flow that has been converted to MPLS
> +AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn add-flow br0 'tcp actions=push_mpls:0x8847,fin_timeout(idle_timeout=1)'],
> +         [1], [], [dnl
> +ovs-ofctl: actions are invalid with specified match (OFPBAC_MATCH_INCONSISTENT)
> +])
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> -- 
> 1.8.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>