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

[Ben Pfaff <blp@nicira.com>]

Thanks Simon, thanks Jarno, applied.

On Fri, Nov 15, 2013 at 08:55:42AM -0800, Jarno Rajahalme wrote:
> Looks good to me,
> 
> Acked-by: Jarno Rajahalme <jrajahalme@nicira.com>
> 
> > On Nov 15, 2013, at 1:10 AM, Simon Horman <horms@verge.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@verge.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@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev