On Fri, Jan 10, 2014 at 01:11:38PM +0900, Simon Horman wrote:
> The patch "Implement OpenFlow support for MPLS, for up to 3 labels"
> allows ovs-vswtichd to fully represent an MPLS label stack containing
> up to three labels in a flow.
>
> MPLS push and pop actions are not permitted by that patch in the case where
> there are more than three labels present or more than three labels would be
> present as the result of an MPLS push action as it is not possible to
> accurately describe changes to the MPLS label stack using flows.
>
> This patch alters the behaviour in such situations to drop the packet
> rather than skipping to the next action. This avoids the danger of
> subsequent actions making invalid assumptions about the packet as a result
> of the skipped actions.
>
> As suggested by Jesse Gross.
>
> Cc: Jesse Gross <je...@nicira.com>
> Signed-off-by: Simon Horman <ho...@verge.net.au>
>
> ---
>
> v2
> * Set ctx->exit
> * Clear odp_actions in compose_mpls_pop_action()
> to be consistent with compose_mpls_push_action()
>
> This is an incremental patch for "Implement OpenFlow support for MPLS, for
> up to 3 labels" intended to further discussion of an issue raised by Jesse
> Gross. I am happy for it to be squashed into "Implement OpenFlow support
> for MPLS, for up to 3 labels" if appropriate.
Thanks, done. I don't think we need both ctx->exit and the return
value, so I actually squashed in the following:
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index f6671f2..01fba27 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2122,6 +2122,15 @@ compose_mpls_push_action(struct xlate_ctx *ctx, struct
ofpact_push_mpls *mpls)
&ctx->xout->odp_actions,
&ctx->xout->wc);
} else if (n >= ARRAY_SIZE(flow->mpls_lse)) {
+ if (ctx->xin->packet != NULL) {
+ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+ VLOG_WARN_RL(&rl, "bridge %s: dropping packet which after an "
+ "MPLS push action will have more MPLS LSEs than "
+ "the %"PRIuSIZE" that can be handled.",
+ ctx->xbridge->name, ARRAY_SIZE(flow->mpls_lse) - 1);
+ }
+ ctx->exit = true;
+ ofpbuf_clear(&ctx->xout->odp_actions);
return;
}
@@ -2135,7 +2144,19 @@ compose_mpls_pop_action(struct xlate_ctx *ctx, ovs_be16
eth_type)
struct flow_wildcards *wc = &ctx->xout->wc;
struct flow *flow = &ctx->xin->flow;
- flow_pop_mpls(flow, eth_type, wc);
+ if (!flow_pop_mpls(flow, eth_type, wc) &&
+ flow_count_mpls_labels(flow, wc) >= ARRAY_SIZE(flow->mpls_lse)) {
+ if (ctx->xin->packet != NULL) {
+ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+ VLOG_WARN_RL(&rl, "bridge %s: dropping packet on which an "
+ "MPLS pop action can't be performed as it has "
+ "have more MPLS LSEs than the %"PRIuSIZE" "
+ "that can be handled.",
+ ctx->xbridge->name, ARRAY_SIZE(flow->mpls_lse) - 1);
+ }
+ ctx->exit = true;
+ ofpbuf_clear(&ctx->xout->odp_actions);
+ }
}
static bool
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index b77e703..ec0995a 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -2380,6 +2380,53 @@
skb_priority(0),skb_mark(0/0),in_port(p3),eth(src=50:54:00:00:00:09/00:00:00:00:
OVS_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([ofproto-dpif - ovs-appctl dpif/dump-flows - MPLS actions that result
in a drop])
+OVS_VSWITCHD_START([dnl
+ add-port br0 p1 -- set Interface p1 type=dummy
+])
+ON_EXIT([kill `cat ovs-ofctl.pid`])
+
+AT_CAPTURE_FILE([ofctl_monitor.log])
+AT_DATA([flows.txt], [dnl
+dl_src=60:66:66:66:66:00 actions=push_mpls:0x8847,controller
+dl_src=60:66:66:66:66:01 actions=pop_mpls:0x8849,controller
+])
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+
+dnl Packet is dropped because an MPLS POP action is applied to a packet with
+dnl 4 MPLS LSEs but ovs-vswtichd can only handle up to 3 MPLS LSEs and thus
+dnl can't determine the resulting MPLS label after an MPLS POP action.
+dnl
+dnl The input is a frame with two MPLS headers which tcpdump -vve shows as:
+dnl 60:66:66:66:66:00 > 50:54:00:00:00:07, ethertype MPLS unicast (0x8847),
length 74: MPLS (label 20, exp 0, ttl 32)
+dnl (label 20, exp 0, ttl 32)
+dnl (label 20, exp 0, ttl 32)
+dnl (label 20, exp 0, [S], ttl 32)
+dnl (tos 0x0, ttl 64, id 0, offset 0, flags [none], proto TCP (6),
length 44, bad cksum 3b78 (->f978)!)
+dnl 192.168.0.1.80 > 192.168.0.2.0: Flags [none], cksum 0x7744 (correct),
seq 42:46, win 10000, length 4
+AT_CHECK([ovs-appctl netdev-dummy/receive p1 '50 54 00 00 00 07 60 66 66 66 66
00 88 47 00 01 40 20 00 01 40 20 00 01 40 20 00 01 41 20 45 00 00 2c 00 00 00
00 40 06 3b 78 c0 a8 00 01 c0 a8 00 02 00 50 00 00 00 00 00 2a 00 00 00 2a 50
00 27 10 77 44 00 00 48 4f 47 45'])
+
+dnl Packet is dropped because an MPLS PUSH action is applied to a packet with
+dnl 4 MPLS LSEs but ovs-vswtichd can only handle up to 3 MPLS LSEs and thus
+dnl can't determine the resulting MPLS label after an MPLS PUSH action.
+dnl
+dnl The input is a frame with two MPLS headers which tcpdump -vve shows as:
+dnl 60:66:66:66:66:01 > 50:54:00:00:00:07, ethertype MPLS unicast (0x8847),
length 74: MPLS (label 20, exp 0, ttl 32)
+dnl (label 20, exp 0, ttl 32)
+dnl (label 20, exp 0, ttl 32)
+dnl (label 20, exp 0, [S], ttl 32)
+dnl (tos 0x0, ttl 64, id 0, offset 0, flags [none], proto TCP (6),
length 44, bad cksum 3b78 (->f978)!)
+dnl 192.168.0.1.80 > 192.168.0.2.0: Flags [none], cksum 0x7744 (correct),
seq 42:46, win 10000, length 4
+AT_CHECK([ovs-appctl netdev-dummy/receive p1 '50 54 00 00 00 07 60 66 66 66 66
01 88 47 00 01 40 20 00 01 40 20 00 01 40 20 00 01 41 20 45 00 00 2c 00 00 00
00 40 06 3b 78 c0 a8 00 01 c0 a8 00 02 00 50 00 00 00 00 00 2a 00 00 00 2a 50
00 27 10 77 44 00 00 48 4f 47 45'])
+
+AT_CHECK([ovs-appctl dpif/dump-flows br0 | sort | STRIP_USED], [0], [dnl
+skb_priority(0),in_port(1),eth(src=60:66:66:66:66:00/ff:ff:ff:ff:ff:ff,dst=50:54:00:00:00:07/00:00:00:00:00:00),eth_type(0x8847),mpls(lse0=0x140200,lse1=0x140200,lse2=0x140200),
packets:0, bytes:0, used:never, actions:drop
+skb_priority(0),in_port(1),eth(src=60:66:66:66:66:01/ff:ff:ff:ff:ff:ff,dst=50:54:00:00:00:07/00:00:00:00:00:00),eth_type(0x8847),mpls(lse0=0x140200,lse1=0x140200,lse2=0x140200),
packets:0, bytes:0, used:never, actions:drop
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([ofproto-dpif - patch ports])
OVS_VSWITCHD_START([add-br br1 \
-- set bridge br1 datapath-type=dummy fail-mode=secure \
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
