Skip to content

Commit

Permalink
mpls: Fix MPLS restoration after patch port and group bucket.
Browse files Browse the repository at this point in the history 
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: 8bfd0fd ("Enhance userspace support for MPLS, for up to 3 labels.")
Fixes: 1b035ef ("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]>
  • Loading branch information
Jarno Rajahalme committed Dec 3, 2016
1 parent 3fa215b commit 742c0ac
Show file tree
Hide file tree
Showing 5 changed files with 89 additions and 10 deletions.
14 changes: 8 additions & 6 deletions lib/flow.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion lib/flow.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
16 changes: 14 additions & 2 deletions lib/odp-util.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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++;
}
Expand Down Expand Up @@ -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);
Expand Down
3 changes: 2 additions & 1 deletion ofproto/ofproto-dpif-xlate.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
64 changes: 64 additions & 0 deletions tests/mpls-xlate.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit 742c0ac

Please sign in to comment.