[ovs-dev] [PATCH] mpls: Fix MPLS restoration after patch port and group bucket.
Takashi YAMAMOTO
yamamoto at ovn.org
Sat Dec 3 13:54:25 UTC 2016
On Sat, Dec 3, 2016 at 12:38 PM, Jarno Rajahalme <jarno at 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 at orange.com>
> Suggested-by: Takashi YAMAMOTO <yamamoto at 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 at ovn.org>
>
Acked-by: YAMAMOTO Takashi <yamamoto at 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 at 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
>
>
More information about the dev
mailing list