| Message ID | 1480736291-7711-1-git-send-email-jarno@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
On Sat, Dec 3, 2016 at 12:38 PM, Jarno Rajahalme <jarno@ovn.org> 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 <thomas.morin@orange.com> > Suggested-by: Takashi YAMAMOTO <yamamoto@ovn.org> > 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 <jarno@ovn.org> > Acked-by: YAMAMOTO Takashi <yamamoto@ovn.org> --- > 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 > >
Thanks for the review, pushed to master, branch-2.6, and branch-2.5. Jarno > On Dec 3, 2016, at 5:54 AM, Takashi YAMAMOTO <yamamoto@ovn.org> wrote: > > > > On Sat, Dec 3, 2016 at 12:38 PM, Jarno Rajahalme <jarno@ovn.org <mailto:jarno@ovn.org>> 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 <thomas.morin@orange.com <mailto:thomas.morin@orange.com>> > Suggested-by: Takashi YAMAMOTO <yamamoto@ovn.org <mailto:yamamoto@ovn.org>> > 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 <jarno@ovn.org <mailto:jarno@ovn.org>> > > Acked-by: YAMAMOTO Takashi <yamamoto@ovn.org <mailto:yamamoto@ovn.org>> > > --- > lib/flow.c | 14 +++++----- > lib/flow.h | 2 +- > lib/odp-util.c | 16 +++++++++-- > ofproto/ofproto-dpif-xlate.c | 3 ++- > tests/mpls-xlate.at <http://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 <http://mpls-xlate.at/> b/tests/mpls-xlate.at <http://mpls-xlate.at/> > index 598a05a..04c6193 100644 > --- a/tests/mpls-xlate.at <http://mpls-xlate.at/> > +++ b/tests/mpls-xlate.at <http://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
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
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 <thomas.morin@orange.com> Suggested-by: Takashi YAMAMOTO <yamamoto@ovn.org> 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 <jarno@ovn.org> --- 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(-)