On Sat, Dec 3, 2016 at 12:38 PM, Jarno Rajahalme <[email protected]> wrote:
> This patch fixes problems with MPLS handling related to patch ports > and group buckets. > > If a group bucket or a peer bridge across a patch port pushes MPLS > headers to a non-MPLS packet and outputs, the flow translation after > returning from the group bucket or patch port would undo the packet > transformations so that the processing could continue with the packet > as it was before entering the patch port. There were two problems > with this: > > 1. As part of the first MPLS push on a non-MPLS packet, the flow > translation would first clear the L3/4 headers of the 'flow' to mark > those fields invalid. Later, when committing 'flow' changes to > datapath actions before output, the necessary datapath MPLS actions > are created and the corresponding changes updated to the 'base flow'. > This was done using the same flow_push_mpls() function that clears > the L2/3 headers, so also the 'base flow' L2/3 headers were cleared. > > Then, when translation returns from a patch port or group bucket, the > original 'flow' is restored, now showing no sign of the MPLS labels. > Since the 'base flow' now has the MPLS labels, following translations > know to issue MPLS POP actions before any output actions. However, as > part of checking for changes to IP headers we test that the IP > protocol type was not changed. But now the 'base flow's 'nw_proto' > field is zero and an assert fail crashes OVS. > > This is solved by not clearing the L3/4 fields of the 'base > flow'. This allows the processing after the patch port to continue > with L3/4 fields as if no MPLS was done, after first issuing the > necessary MPLS POP actions. > > 2. IP header updates were done before the MPLS POP actions were > issued. This caused incorrect packet output after, e.g., group action > or patch port. For example, with actions: > > group 1234: all bucket=push_mpls,output:LOCAL > > ip actions=group:1234,dec_ttl,output:LOCAL,output:LOCAL > > the dec_ttl would only be executed before the last output to LOCAL, > since at the time of committing IP changes after the group action the > packet was still an MPLS packet. > > This is solved by checking the dl_type of both 'flow' and 'base flow' > and issuing MPLS actions if they can transform the packet from an MPLS > packet to a non-MPLS packet. For an IP packet the change in ttl can > then be correctly committed before the last two output actions. > > Two test cases are added to prevent future regressions. > > Reported-by: Thomas Morin <[email protected]> > Suggested-by: Takashi YAMAMOTO <[email protected]> > Fixes: 8bfd0fdac ("Enhance userspace support for MPLS, for up to 3 > labels.") > Fixes: 1b035ef20 ("mpls: Allow l3 and l4 actions to prior to a push_mpls > action") > Signed-off-by: Jarno Rajahalme <[email protected]> > Acked-by: YAMAMOTO Takashi <[email protected]> --- > lib/flow.c | 14 +++++----- > lib/flow.h | 2 +- > lib/odp-util.c | 16 +++++++++-- > ofproto/ofproto-dpif-xlate.c | 3 ++- > tests/mpls-xlate.at | 64 ++++++++++++++++++++++++++++++ > ++++++++++++++ > 5 files changed, 89 insertions(+), 10 deletions(-) > > diff --git a/lib/flow.c b/lib/flow.c > index f4ac8b3..b6d0d15 100644 > --- a/lib/flow.c > +++ b/lib/flow.c > @@ -2096,7 +2096,7 @@ flow_count_common_mpls_labels(const struct flow *a, > int an, > */ > void > flow_push_mpls(struct flow *flow, int n, ovs_be16 mpls_eth_type, > - struct flow_wildcards *wc) > + struct flow_wildcards *wc, bool clear_flow_L3) > { > ovs_assert(eth_type_mpls(mpls_eth_type)); > ovs_assert(n < FLOW_MAX_MPLS_LABELS); > @@ -2134,11 +2134,13 @@ flow_push_mpls(struct flow *flow, int n, ovs_be16 > mpls_eth_type, > > flow->mpls_lse[0] = set_mpls_lse_values(ttl, tc, 1, htonl(label)); > > - /* Clear all L3 and L4 fields and dp_hash. */ > - BUILD_ASSERT(FLOW_WC_SEQ == 36); > - memset((char *) flow + FLOW_SEGMENT_2_ENDS_AT, 0, > - sizeof(struct flow) - FLOW_SEGMENT_2_ENDS_AT); > - flow->dp_hash = 0; > + if (clear_flow_L3) { > + /* Clear all L3 and L4 fields and dp_hash. */ > + BUILD_ASSERT(FLOW_WC_SEQ == 36); > + memset((char *) flow + FLOW_SEGMENT_2_ENDS_AT, 0, > + sizeof(struct flow) - FLOW_SEGMENT_2_ENDS_AT); > + flow->dp_hash = 0; > + } > } > flow->dl_type = mpls_eth_type; > } > diff --git a/lib/flow.h b/lib/flow.h > index 35724b1..62315bc 100644 > --- a/lib/flow.h > +++ b/lib/flow.h > @@ -95,7 +95,7 @@ int flow_count_common_mpls_labels(const struct flow *a, > int an, > const struct flow *b, int bn, > struct flow_wildcards *wc); > void flow_push_mpls(struct flow *, int n, ovs_be16 mpls_eth_type, > - struct flow_wildcards *); > + struct flow_wildcards *, bool clear_flow_L3); > bool flow_pop_mpls(struct flow *, int n, ovs_be16 eth_type, > struct flow_wildcards *); > void flow_set_mpls_label(struct flow *, int idx, ovs_be32 label); > diff --git a/lib/odp-util.c b/lib/odp-util.c > index f09aabe..427dd65 100644 > --- a/lib/odp-util.c > +++ b/lib/odp-util.c > @@ -5553,7 +5553,10 @@ commit_mpls_action(const struct flow *flow, struct > flow *base, > sizeof *mpls); > mpls->mpls_ethertype = flow->dl_type; > mpls->mpls_lse = flow->mpls_lse[flow_n - base_n - 1]; > - flow_push_mpls(base, base_n, mpls->mpls_ethertype, NULL); > + /* Update base flow's MPLS stack, but do not clear L3. We need > the L3 > + * headers if the flow is restored later due to returning from a > patch > + * port or group bucket. */ > + flow_push_mpls(base, base_n, mpls->mpls_ethertype, NULL, false); > flow_set_mpls_lse(base, 0, mpls->mpls_lse); > base_n++; > } > @@ -5916,12 +5919,21 @@ commit_odp_actions(const struct flow *flow, struct > flow *base, > bool use_masked) > { > enum slow_path_reason slow1, slow2; > + bool mpls_done = false; > > commit_set_ether_addr_action(flow, base, odp_actions, wc, > use_masked); > + /* Make packet a non-MPLS packet before committing L3/4 actions, > + * which would otherwise do nothing. */ > + if (eth_type_mpls(base->dl_type) && !eth_type_mpls(flow->dl_type)) { > + commit_mpls_action(flow, base, odp_actions); > + mpls_done = true; > + } > slow1 = commit_set_nw_action(flow, base, odp_actions, wc, use_masked); > commit_set_port_action(flow, base, odp_actions, wc, use_masked); > slow2 = commit_set_icmp_action(flow, base, odp_actions, wc); > - commit_mpls_action(flow, base, odp_actions); > + if (!mpls_done) { > + commit_mpls_action(flow, base, odp_actions); > + } > commit_vlan_action(flow->vlan_tci, base, odp_actions, wc); > commit_set_priority_action(flow, base, odp_actions, wc, use_masked); > commit_set_pkt_mark_action(flow, base, odp_actions, wc, use_masked); > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 55ac371..d0f9a33 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -3823,7 +3823,8 @@ compose_mpls_push_action(struct xlate_ctx *ctx, > struct ofpact_push_mpls *mpls) > return; > } > > - flow_push_mpls(flow, n, mpls->ethertype, ctx->wc); > + /* Update flow's MPLS stack, and clear L3/4 fields to mark them > invalid. */ > + flow_push_mpls(flow, n, mpls->ethertype, ctx->wc, true); > } > > static void > diff --git a/tests/mpls-xlate.at b/tests/mpls-xlate.at > index 598a05a..04c6193 100644 > --- a/tests/mpls-xlate.at > +++ b/tests/mpls-xlate.at > @@ -132,3 +132,67 @@ AT_CHECK([tail -1 stdout], [0], > > OVS_VSWITCHD_STOP > AT_CLEANUP > + > +AT_SETUP([MPLS xlate action - patch-port]) > + > +OVS_VSWITCHD_START( > + [add-port br0 p0 -- set Interface p0 type=dummy ofport_request=1 -- \ > + add-port br0 p1 -- set Interface p1 type=patch \ > + options:peer=p2 ofport_request=2 > -- \ > + add-br br1 -- \ > + set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \ > + set bridge br1 datapath-type=dummy other-config:datapath-id=1234 \ > + fail-mode=secure -- \ > + add-port br1 p2 -- set Interface p2 type=patch \ > + options:peer=p1 -- \ > + add-port br1 p3 -- set Interface p3 type=dummy ofport_request=3]) > + > +AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg > ofproto_dpif_upcall:dbg]) > + > +AT_CHECK([ovs-appctl dpif/show], [0], [dnl > +dummy@ovs-dummy: hit:0 missed:0 > + br0: > + br0 65534/100: (dummy-internal) > + p0 1/1: (dummy) > + p1 2/none: (patch: peer=p2) > + br1: > + br1 65534/101: (dummy-internal) > + p2 1/none: (patch: peer=p1) > + p3 3/3: (dummy) > +]) > + > +dnl MPLS PUSH + POP. > +AT_CHECK([ovs-ofctl del-flows br0]) > + > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 > in_port=local,ip,actions=2,1,1]) > + > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br1 > in_port=1,ip,actions=dec_ttl,push_mpls:0x8847,3]) > + > +dnl MPLS push+pop > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(100),eth(src=f8:bc: > 12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x0800),ipv4( > src=10.1.1.22,dst=10.0.0.3,proto=6,tos=0,ttl=64,frag=no),tcp(src=53295,dst=8080)'], > [0], [stdout]) > +AT_CHECK([tail -1 stdout], [0], > + [Datapath actions: set(ipv4(ttl=63)),push_mpls( > label=0,tc=0,ttl=63,bos=1,eth_type=0x8847),3,pop_mpls(eth_ > type=0x800),set(ipv4(tos=0/0xfc,ttl=64)),1,1 > +]) > + > +OVS_VSWITCHD_STOP > +AT_CLEANUP > + > +AT_SETUP([MPLS xlate action - group bucket]) > + > +OVS_VSWITCHD_START > +add_of_ports br0 1 > + > +AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg > ofproto_dpif_upcall:dbg]) > + > +AT_CHECK([ovs-ofctl del-flows br0]) > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-group br0 'group_id=1234,type=all, > bucket=push_mpls:0x8847,output:1']) > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 in_port=local,ip,actions= > group:1234,output:1,output:1]) > + > +dnl MPLS push in a bucket > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(100),eth(src=f8:bc: > 12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x0800),ipv4( > src=10.1.1.22,dst=10.0.0.3,proto=6,tos=0,ttl=64,frag=no),tcp(src=53295,dst=8080)'], > [0], [stdout]) > +AT_CHECK([tail -1 stdout], [0], > + [Datapath actions: push_mpls(label=0,tc=0,ttl=64, > bos=1,eth_type=0x8847),1,pop_mpls(eth_type=0x800),1,1 > +]) > + > +OVS_VSWITCHD_STOP > +AT_CLEANUP > -- > 2.1.4 > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
