Message-Id: <6A09EE11-CDFA-4E49-AB7F-34746ED4765D@nicira.com>
Date:	Fri, 15 Nov 2013 08:55:42 -0800
From:	Jarno Rajahalme <jrajahalme@...ira.com>
To:	Simon Horman <horms@...ge.net.au>
Cc:	"dev@...nvswitch.org" <dev@...nvswitch.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	Jesse Gross <jesse@...ira.com>, Ben Pfaff <blp@...ira.com>,
	Ravi K <rkerur@...il.com>
Subject: Re: [ovs-dev] [PATCH v2.50 1/6] ofp-actions: Consider L4 actions after mpls_push as inconsistent

Looks good to me,

Acked-by: Jarno Rajahalme <jrajahalme@...ira.com>

> On Nov 15, 2013, at 1:10 AM, Simon Horman <horms@...ge.net.au> 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@...ge.net.au>
> 
> ---
> v2.50
> * Correct typo in comment. nw_protocol -> nw_proto
> 
> 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..03ad1a4 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_proto and flow->vlan_tci,
> + * 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
> 
> _______________________________________________
> dev mailing list
> dev@...nvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists