1. Flows with an explicit match on nw_frag, where the LATER bit is 1:
Prohibit setting transport header fields (port numbers) with
set_field or move, or using such a field as a source in a move.
2. Flows that wildcard the nw_frag LATER bit: At flow translation
time, detect the fact that the packet/flow has no transport header,
and (silently) do nothing when translating a set_field, set_tp_src,
set_tp_dst, or reg_move action that reads or writes on transport
headers. nw_frag is exact matched, so non-LATER packets deal with
the transport ports as before.
2. alone would suffice for correct behavior, but 1. seems like a right
thing to do, anyway.
Finally, we add tests testing the new behavior.
Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
---
lib/meta-flow.c | 9 +++
lib/nx-match.c | 14 +++-
lib/ofp-actions.c | 28 ++++----
ofproto/ofproto-dpif-xlate.c | 12 +++-
ofproto/ofproto-dpif.c | 2 +-
tests/ofproto-dpif.at | 160 ++++++++++++++++++++++++++++++++++++++++--
6 files changed, 202 insertions(+), 23 deletions(-)
diff --git a/lib/meta-flow.c b/lib/meta-flow.c
index 7871545..345fa63 100644
--- a/lib/meta-flow.c
+++ b/lib/meta-flow.c
@@ -1531,6 +1531,15 @@ mf_check__(const struct mf_subfield *sf, const struct
flow *flow,
VLOG_WARN_RL(&rl, "%s field %s lacks correct prerequisites",
type, sf->field->name);
return OFPERR_OFPBAC_MATCH_INCONSISTENT;
+ } else if (flow && (flow->nw_frag & FLOW_NW_FRAG_LATER) &&
+ (sf->field->id == MFF_TCP_SRC || sf->field->id == MFF_TCP_DST ||
+ sf->field->id == MFF_UDP_SRC || sf->field->id == MFF_UDP_DST ||
+ sf->field->id == MFF_SCTP_SRC ||
+ sf->field->id == MFF_SCTP_DST)) {
+ VLOG_WARN_RL(&rl,
+ "transport layer field %s as a %s on a later fragment",
+ sf->field->name, type);
+ return OFPERR_OFPBAC_MATCH_INCONSISTENT;
} else {
return 0;
}
diff --git a/lib/nx-match.c b/lib/nx-match.c
index 82b472c..3a9a8af 100644
--- a/lib/nx-match.c
+++ b/lib/nx-match.c
@@ -1378,7 +1378,7 @@ nxm_reg_move_check(const struct ofpact_reg_move *move,
const struct flow *flow)
return error;
}
- return mf_check_dst(&move->dst, NULL);
+ return mf_check_dst(&move->dst, flow);
}
/* nxm_execute_reg_move(). */
@@ -1390,6 +1390,18 @@ nxm_execute_reg_move(const struct ofpact_reg_move *move,
union mf_value src_value;
union mf_value dst_value;
+ /* A flow may wildcard nw_frag. Do nothing if setting a transport
+ * header field on a packet that does not have them. */
+ if ((flow->nw_frag & FLOW_NW_FRAG_LATER) &&
+ (move->dst.field->id == MFF_TCP_SRC ||
+ move->dst.field->id == MFF_TCP_DST ||
+ move->dst.field->id == MFF_UDP_SRC ||
+ move->dst.field->id == MFF_UDP_DST ||
+ move->dst.field->id == MFF_SCTP_SRC ||
+ move->dst.field->id == MFF_SCTP_DST)) {
+ return;
+ }
+
mf_mask_field_and_prereqs(move->dst.field, &wc->masks);
mf_mask_field_and_prereqs(move->src.field, &wc->masks);
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 41c7622..29f69a0 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -5333,19 +5333,8 @@ ofpact_check__(enum ofputil_protocol *usable_protocols,
struct ofpact *a,
return 0;
case OFPACT_SET_L4_SRC_PORT:
- if (!is_ip_any(flow) ||
- (flow->nw_proto != IPPROTO_TCP && flow->nw_proto != IPPROTO_UDP
- && flow->nw_proto != IPPROTO_SCTP)) {
- inconsistent_match(usable_protocols);
- }
- /* Note on which transport protocol the port numbers are set.
- * This allows this set action to be converted to an OF1.2 set field
- * action. */
- ofpact_get_SET_L4_SRC_PORT(a)->flow_ip_proto = flow->nw_proto;
- return 0;
-
case OFPACT_SET_L4_DST_PORT:
- if (!is_ip_any(flow) ||
+ if (!is_ip_any(flow) || (flow->nw_frag & FLOW_NW_FRAG_LATER) ||
(flow->nw_proto != IPPROTO_TCP && flow->nw_proto != IPPROTO_UDP
&& flow->nw_proto != IPPROTO_SCTP)) {
inconsistent_match(usable_protocols);
@@ -5353,7 +5342,11 @@ ofpact_check__(enum ofputil_protocol *usable_protocols,
struct ofpact *a,
/* Note on which transport protocol the port numbers are set.
* This allows this set action to be converted to an OF1.2 set field
* action. */
- ofpact_get_SET_L4_DST_PORT(a)->flow_ip_proto = flow->nw_proto;
+ if (a->type == OFPACT_SET_L4_SRC_PORT) {
+ ofpact_get_SET_L4_SRC_PORT(a)->flow_ip_proto = flow->nw_proto;
+ } else {
+ ofpact_get_SET_L4_DST_PORT(a)->flow_ip_proto = flow->nw_proto;
+ }
return 0;
case OFPACT_REG_MOVE:
@@ -5368,6 +5361,15 @@ ofpact_check__(enum ofputil_protocol *usable_protocols,
struct ofpact *a,
mf->name);
return OFPERR_OFPBAC_MATCH_INCONSISTENT;
}
+ /* Prohibit setting of transport ports on later fragments. */
+ if (flow->nw_frag & FLOW_NW_FRAG_LATER &&
+ (mf->id == MFF_TCP_SRC || mf->id == MFF_TCP_DST ||
+ mf->id == MFF_UDP_SRC || mf->id == MFF_UDP_DST ||
+ mf->id == MFF_SCTP_SRC || mf->id == MFF_SCTP_DST)) {
+ VLOG_WARN_RL(&rl, "set_field %s on a later fragment",
+ mf->name);
+ return OFPERR_OFPBAC_MATCH_INCONSISTENT;
+ }
/* Remember if we saw a vlan tag in the flow to aid translating to
* OpenFlow 1.1 if need be. */
ofpact_get_SET_FIELD(a)->flow_has_vlan =
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 12bfb9f..2bde8f6 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3747,7 +3747,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
ofpacts_len,
break;
case OFPACT_SET_L4_SRC_PORT:
- if (is_ip_any(flow)) {
+ if (is_ip_any(flow) && !(flow->nw_frag & FLOW_NW_FRAG_LATER)) {
memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src);
flow->tp_src = htons(ofpact_get_SET_L4_SRC_PORT(a)->port);
@@ -3755,7 +3755,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
ofpacts_len,
break;
case OFPACT_SET_L4_DST_PORT:
- if (is_ip_any(flow)) {
+ if (is_ip_any(flow) && !(flow->nw_frag & FLOW_NW_FRAG_LATER)) {
memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
memset(&wc->masks.tp_dst, 0xff, sizeof wc->masks.tp_dst);
flow->tp_dst = htons(ofpact_get_SET_L4_DST_PORT(a)->port);
@@ -3802,6 +3802,14 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
ofpacts_len,
&& !eth_type_mpls(flow->dl_type)) {
break;
}
+ /* A flow may wildcard nw_frag. Do nothing if setting a trasport
+ * header field on a packet that does not have them. */
+ if (flow->nw_frag & FLOW_NW_FRAG_LATER &&
+ (mf->id == MFF_TCP_SRC || mf->id == MFF_TCP_DST ||
+ mf->id == MFF_UDP_SRC || mf->id == MFF_UDP_DST ||
+ mf->id == MFF_SCTP_SRC || mf->id == MFF_SCTP_DST)) {
+ break;
+ }
mf_mask_field_and_prereqs(mf, &wc->masks);
mf_set_flow_value_masked(mf, &set_field->value, &set_field->mask,
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index d965d38..0544508 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4643,7 +4643,7 @@ ofproto_unixctl_trace_actions(struct unixctl_conn *conn,
int argc,
/* Do the same checks as handle_packet_out() in ofproto.c.
*
- * We pass a 'table_id' of 0 to ofproto_check_ofpacts(), which isn't
+ * We pass a 'table_id' of 0 to ofpacts_check(), which isn't
* strictly correct because these actions aren't in any table, but it's OK
* because it 'table_id' is used only to check goto_table instructions, but
* packet-outs take a list of actions and therefore it can't include
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 05bb0e3..345a901 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -3284,16 +3284,16 @@ OFPST_FLOW reply (OF1.2):
OVS_VSWITCHD_STOP
AT_CLEANUP
-AT_SETUP([ofproto-dpif - fragment handling])
+AT_SETUP([ofproto-dpif - fragment handling - trace])
OVS_VSWITCHD_START
ADD_OF_PORTS([br0], [1], [2], [3], [4], [5], [6], [90])
AT_DATA([flows.txt], [dnl
priority=75 tcp ip_frag=no tp_dst=80
actions=move:OXM_OF_TCP_DST[[]]->OXM_OF_TCP_SRC[[]],output:1
priority=75 tcp ip_frag=first tp_dst=80
actions=move:OXM_OF_TCP_DST[[]]->OXM_OF_TCP_SRC[[]],output:2
-priority=75 tcp ip_frag=later tp_dst=80
actions=move:OXM_OF_TCP_DST[[]]->OXM_OF_TCP_SRC[[]],output:3
+priority=75 tcp ip_frag=later tp_dst=80 actions=output:3
priority=50 tcp ip_frag=no
actions=move:OXM_OF_TCP_DST[[]]->OXM_OF_TCP_SRC[[]],output:4
priority=50 tcp ip_frag=first
actions=move:OXM_OF_TCP_DST[[]]->OXM_OF_TCP_SRC[[]],output:5
-priority=50 tcp ip_frag=later
actions=move:OXM_OF_TCP_DST[[]]->OXM_OF_TCP_SRC[[]],output:6
+priority=50 tcp ip_frag=later actions=output:6
])
AT_CHECK([ovs-ofctl replace-flows br0 flows.txt])
@@ -3323,9 +3323,7 @@ do
if test $mode = drop && test $type != no; then
echo 'Packets dropped because they are IP fragments and the fragment
handling mode is "drop".' >> expout
echo "Datapath actions: $exp_output" >> expout
- elif test $mode = normal && test $type = later; then
- echo "Datapath actions: $exp_output" >> expout
- elif test $mode = nx-match && test $type = later; then
+ elif test $type = later; then
echo "Datapath actions: $exp_output" >> expout
else
echo "Datapath actions: set(tcp(src=80,dst=80)),$exp_output" >> expout
@@ -3336,6 +3334,156 @@ done
OVS_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([ofproto-dpif - fragment handling - upcall])
+OVS_VSWITCHD_START
+ADD_OF_PORTS([br0], [1], [2], [3], [4], [5], [6], [90])
+AT_DATA([flows.txt], [dnl
+priority=75 tcp ip_frag=no tp_dst=80 actions=set_field:81->tcp_dst,output:1
+priority=75 tcp ip_frag=first tp_dst=80 actions=set_field:81->tcp_dst,output:2
+priority=75 tcp ip_frag=later tp_dst=80 actions=output:3
+priority=50 tcp ip_frag=no actions=set_field:81->tcp_dst,output:4
+priority=50 tcp ip_frag=first actions=set_field:81->tcp_dst,output:5
+priority=50 tcp ip_frag=later actions=output:6
+])
+AT_CHECK([ovs-ofctl replace-flows br0 flows.txt])
+
+base_flow="in_port(90),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=128"
+no_flow="$base_flow,frag=no),tcp(src=12345,dst=80)"
+first_flow="$base_flow,frag=first),tcp(src=12345,dst=80)"
+later_flow="$base_flow,frag=later)"
+
+AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg])
+
+mode=normal
+
+AT_CHECK([ovs-ofctl set-frags br0 $mode])
+for type in no first later; do
+ eval flow=\$${type}_flow
+ printf "\n%s\n" "----$mode $type-----"
+
+ AT_CHECK([ovs-appctl netdev-dummy/receive p90 "$flow"], [0], [stdout])
+done
+
+AT_CHECK([ovs-appctl dpctl/dump-flows], [0], [dnl
+recirc_id(0),in_port(90),eth_type(0x0800),ipv4(proto=6,frag=no),tcp(dst=80),
packets:0, bytes:0, used:never, actions:set(tcp(dst=81)),1
+recirc_id(0),in_port(90),eth_type(0x0800),ipv4(proto=6,frag=first),tcp(dst=80),
packets:0, bytes:0, used:never, actions:set(tcp(dst=81)),5
+recirc_id(0),in_port(90),eth_type(0x0800),ipv4(proto=6,frag=later), packets:0,
bytes:0, used:never, actions:6
+])
+
+mode=drop
+
+AT_CHECK([ovs-appctl dpctl/del-flows], [0])
+AT_CHECK([ovs-ofctl set-frags br0 $mode])
+for type in no first later; do
+ eval flow=\$${type}_flow
+ printf "\n%s\n" "----$mode $type-----"
+
+ AT_CHECK([ovs-appctl netdev-dummy/receive p90 "$flow"], [0], [stdout])
+done
+
+AT_CHECK([ovs-appctl dpctl/dump-flows], [0], [dnl
+recirc_id(0),in_port(90),eth_type(0x0800),ipv4(proto=6,frag=no),tcp(dst=80),
packets:0, bytes:0, used:never, actions:set(tcp(dst=81)),1
+recirc_id(0),in_port(90),eth_type(0x0800),ipv4(frag=first), packets:0,
bytes:0, used:never, actions:drop
+recirc_id(0),in_port(90),eth_type(0x0800),ipv4(frag=later), packets:0,
bytes:0, used:never, actions:drop
+])
+
+mode=nx-match
+
+AT_CHECK([ovs-appctl dpctl/del-flows], [0])
+AT_CHECK([ovs-ofctl set-frags br0 $mode])
+for type in no first later; do
+ eval flow=\$${type}_flow
+ printf "\n%s\n" "----$mode $type-----"
+
+ AT_CHECK([ovs-appctl netdev-dummy/receive p90 "$flow"], [0], [stdout])
+done
+
+AT_CHECK([ovs-appctl dpctl/dump-flows], [0], [dnl
+recirc_id(0),in_port(90),eth_type(0x0800),ipv4(proto=6,frag=no),tcp(dst=80),
packets:0, bytes:0, used:never, actions:set(tcp(dst=81)),1
+recirc_id(0),in_port(90),eth_type(0x0800),ipv4(proto=6,frag=first),tcp(dst=80),
packets:0, bytes:0, used:never, actions:set(tcp(dst=81)),2
+recirc_id(0),in_port(90),eth_type(0x0800),ipv4(proto=6,frag=later), packets:0,
bytes:0, used:never, actions:6
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - fragment handling - actions])
+OVS_VSWITCHD_START
+ADD_OF_PORTS([br0], [1], [2], [3], [4], [5], [6], [90])
+
+AT_CHECK([ovs-ofctl add-flow br0 "tcp,ip_frag=later
actions=move:OXM_OF_TCP_DST[[0..7]]->OXM_OF_TCP_SRC[[0..7]],output:1"], [1],
[], [stderr])
+AT_CHECK([tail -2 stderr | sed 's/^.*|WARN|//'], [0], [dnl
+transport layer field tcp_dst as a source on a later fragment
+ovs-ofctl: actions are invalid with specified match (OFPBAC_MATCH_INCONSISTENT)
+])
+
+AT_CHECK([ovs-ofctl -O OpenFlow15 add-flow br0 "tcp,ip_frag=later
actions=move:OXM_OF_PKT_REG0[[0..7]]->OXM_OF_TCP_SRC[[0..7]],output:1"], [1],
[], [stderr])
+AT_CHECK([tail -2 stderr | sed 's/^.*|WARN|//'], [0], [dnl
+transport layer field tcp_src as a destination on a later fragment
+ovs-ofctl: actions are invalid with specified match (OFPBAC_MATCH_INCONSISTENT)
+])
+
+AT_CHECK([ovs-ofctl add-flow br0 "udp,ip_frag=later
actions=set_field:8888->udp_src,output:1"], [1], [], [stderr])
+AT_CHECK([tail -2 stderr | sed 's/^.*|WARN|//'], [0], [dnl
+set_field udp_src on a later fragment
+ovs-ofctl: actions are invalid with specified match (OFPBAC_MATCH_INCONSISTENT)
+])
+
+AT_CHECK([ovs-ofctl add-flow br0 "udp,ip_frag=later
actions=load:8888->NXM_OF_UDP_DST[[]],output:1"], [1], [], [stderr])
+AT_CHECK([tail -2 stderr | sed 's/^.*|WARN|//'], [0], [dnl
+set_field udp_dst on a later fragment
+ovs-ofctl: actions are invalid with specified match (OFPBAC_MATCH_INCONSISTENT)
+])
+
+AT_CHECK([ovs-ofctl add-flow br0 "sctp,ip_frag=later
actions=set_field:8888->sctp_src,output:1"], [1], [], [stderr])
+AT_CHECK([tail -2 stderr | sed 's/^.*|WARN|//'], [0], [dnl
+set_field sctp_src on a later fragment
+ovs-ofctl: actions are invalid with specified match (OFPBAC_MATCH_INCONSISTENT)
+])
+
+AT_CHECK([ovs-ofctl add-flow br0 "sctp,ip_frag=later
actions=set_field:8888->sctp_dst,output:1"], [1], [], [stderr])
+AT_CHECK([tail -2 stderr | sed 's/^.*|WARN|//'], [0], [dnl
+set_field sctp_dst on a later fragment
+ovs-ofctl: actions are invalid with specified match (OFPBAC_MATCH_INCONSISTENT)
+])
+
+AT_CHECK([ovs-ofctl add-flow br0 "tcp,ip_frag=later
actions=learn(table=1,hard_timeout=60,eth_type=0x800,nw_proto=6,NXM_OF_IP_SRC[[]]=NXM_OF_IP_DST[[]],NXM_OF_TCP_SRC[[]]=NXM_OF_TCP_DST[[]],output:NXM_NX_REG0[[0..15]]),output:1"],
[1], [], [stderr])
+AT_CHECK([tail -2 stderr | sed 's/^.*|WARN|//'], [0], [dnl
+transport layer field tcp_dst as a source on a later fragment
+ovs-ofctl: actions are invalid with specified match (OFPBAC_MATCH_INCONSISTENT)
+])
+
+AT_DATA([flows.txt], [dnl
+priority=75 tcp
actions=move:NXM_NX_REG0[[0..7]]->OXM_OF_TCP_SRC[[0..7]],output:1
+])
+AT_CHECK([ovs-ofctl -O OpenFlow12 replace-flows br0 flows.txt])
+
+base_flow="in_port(90),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=128"
+no_flow="$base_flow,frag=no),tcp(src=12345,dst=80)"
+first_flow="$base_flow,frag=first),tcp(src=12345,dst=80)"
+later_flow="$base_flow,frag=later)"
+
+AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg])
+
+mode=normal
+
+AT_CHECK([ovs-ofctl set-frags br0 $mode])
+for type in no first later; do
+ eval flow=\$${type}_flow
+ printf "\n%s\n" "----$mode $type-----"
+
+ AT_CHECK([ovs-appctl netdev-dummy/receive p90 "$flow"], [0], [stdout])
+done
+
+AT_CHECK([ovs-appctl dpctl/dump-flows], [0], [dnl
+recirc_id(0),in_port(90),eth_type(0x0800),ipv4(proto=6,frag=no),tcp(src=12345),
packets:0, bytes:0, used:never, actions:set(tcp(src=12288)),1
+recirc_id(0),in_port(90),eth_type(0x0800),ipv4(proto=6,frag=first),tcp(src=12345),
packets:0, bytes:0, used:never, actions:set(tcp(src=12288)),1
+recirc_id(0),in_port(90),eth_type(0x0800),ipv4(proto=6,frag=later), packets:0,
bytes:0, used:never, actions:1
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([ofproto-dpif - exit])
OVS_VSWITCHD_START
ADD_OF_PORTS([br0], [1], [2], [3], [10], [11], [12], [13], [14])
--
1.7.10.4
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
