[ovs-dev] [PATCH] ofp-actions: Error on conntrack action inconsistency.
Jarno Rajahalme
jarno at ovn.org
Tue Sep 27 01:46:41 UTC 2016
Setting up a datapath flow that has a conntrack action with 'alg=ftp',
but does not match on 'nw_proto=tcp' fails with 'WARN' in
ovs-vswitchd.log. It is better to flag such inconsistencies during
OpenFlow rule set-up time. Also, conntrack action inconsistencies
should be flagged as such, rather than assuming that falling back to
OpenFlow 1.0 (which allows inconsistent actions) would remedy the
situation.
Remove the IP check from the action inconsistency check so that it is
possible to write rules that operate on both IPv4 and IPv6 packets,
when the action itself is not IP version dependent. Correspondingly,
when translating a conntrack action, do not issue any datapath actions
if the packet being translated is not an IP packet, as conntrack can
operate only on IP packets and a datapath flow set-up otherwise fails
anyway.
Signed-off-by: Jarno Rajahalme <jarno at ovn.org>
---
include/openvswitch/ofp-actions.h | 2 +-
lib/ofp-actions.c | 22 +++++++++++-----------
ofproto/ofproto-dpif-xlate.c | 10 +++++++---
tests/ofproto-dpif.at | 6 +++---
tests/ovs-ofctl.at | 34 +++++++++++++++++-----------------
5 files changed, 39 insertions(+), 35 deletions(-)
diff --git a/include/openvswitch/ofp-actions.h b/include/openvswitch/ofp-actions.h
index 67e84fa..74e9dcc 100644
--- a/include/openvswitch/ofp-actions.h
+++ b/include/openvswitch/ofp-actions.h
@@ -556,7 +556,7 @@ enum nx_conntrack_flags {
#define NX_CT_RECIRC_NONE OFPTT_ALL
#if !defined(IPPORT_FTP)
-#define IPPORT_FTP 21
+#define IPPORT_FTP 21
#endif
/* OFPACT_CT.
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 22c7b16..4a8e79d 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -7029,31 +7029,31 @@ ofpact_check__(enum ofputil_protocol *usable_protocols, struct ofpact *a,
case OFPACT_CT: {
struct ofpact_conntrack *oc = ofpact_get_CT(a);
- enum ofperr err;
- if (!dl_type_is_ip_any(flow->dl_type)
- || (flow->ct_state & CS_INVALID && oc->flags & NX_CT_F_COMMIT)) {
- inconsistent_match(usable_protocols);
+ if ((flow->ct_state & CS_INVALID && oc->flags & NX_CT_F_COMMIT)
+ || (oc->alg == IPPORT_FTP && flow->nw_proto != IPPROTO_TCP)) {
+ /* We can't downgrade to OF1.0 and expect inconsistent CT actions
+ * be silently disgarded. Instead, datapath flow install fails, so
+ * it is better to flag inconsistent CT actions as hard errors. */
+ return OFPERR_OFPBAC_MATCH_INCONSISTENT;
}
if (oc->zone_src.field) {
return mf_check_src(&oc->zone_src, flow);
}
- err = ofpacts_check(oc->actions, ofpact_ct_get_action_len(oc),
- flow, max_ports, table_id, n_tables,
- usable_protocols);
- return err;
+ return ofpacts_check(oc->actions, ofpact_ct_get_action_len(oc),
+ flow, max_ports, table_id, n_tables,
+ usable_protocols);
}
case OFPACT_NAT: {
struct ofpact_nat *on = ofpact_get_NAT(a);
- if (!dl_type_is_ip_any(flow->dl_type) ||
- (on->range_af == AF_INET && flow->dl_type != htons(ETH_TYPE_IP)) ||
+ if ((on->range_af == AF_INET && flow->dl_type != htons(ETH_TYPE_IP)) ||
(on->range_af == AF_INET6
&& flow->dl_type != htons(ETH_TYPE_IPV6))) {
- inconsistent_match(usable_protocols);
+ return OFPERR_OFPBAC_MATCH_INCONSISTENT;
}
return 0;
}
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 4c28712..479bf13 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -5030,12 +5030,16 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
break;
case OFPACT_CT:
- compose_conntrack_action(ctx, ofpact_get_CT(a));
+ if (is_ip_any(flow)) {
+ compose_conntrack_action(ctx, ofpact_get_CT(a));
+ }
break;
case OFPACT_NAT:
- /* This will be processed by compose_conntrack_action(). */
- ctx->ct_nat_action = ofpact_get_NAT(a);
+ if (is_ip_any(flow)) {
+ /* This will be processed by compose_conntrack_action(). */
+ ctx->ct_nat_action = ofpact_get_NAT(a);
+ }
break;
case OFPACT_DEBUG_RECIRC:
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index e2b983f..cc3265f 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -8407,14 +8407,14 @@ AT_CHECK([ovs-ofctl monitor br0 65534 invalid_ttl -P nxt_packet_in --detach --no
AT_CHECK([ovs-appctl time/stop])
-AT_CHECK([ovs-appctl netdev-dummy/receive p2 'eth(src=80:88:88:88:88:88,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=192.168.0.1,tip=192.168.0.2,op=1,sha=50:54:00:00:00:05,tha=00:00:00:00:00:00)'])
+AT_CHECK([ovs-appctl netdev-dummy/receive p2 '0060970769ea0000860580da86dd6000000000203afffe80000000000000020086fffe0580dafe80000000000000026097fffe0769ea870068bd00000000fe80000000000000026097fffe0769ea01010000860580da'])
OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 1])
OVS_WAIT_UNTIL([ovs-appctl -t ovs-ofctl exit])
AT_CHECK([cat ofctl_monitor.log], [0], [dnl
-NXT_PACKET_IN (xid=0x0): cookie=0x0 total_len=42 ct_state=inv|trk,in_port=2 (via action) data_len=42 (unbuffered)
-arp,vlan_tci=0x0000,dl_src=80:88:88:88:88:88,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=192.168.0.1,arp_tpa=192.168.0.2,arp_op=1,arp_sha=50:54:00:00:00:05,arp_tha=00:00:00:00:00:00
+NXT_PACKET_IN (xid=0x0): cookie=0x0 total_len=86 ct_state=inv|trk,in_port=2 (via action) data_len=86 (unbuffered)
+icmp6,vlan_tci=0x0000,dl_src=00:00:86:05:80:da,dl_dst=00:60:97:07:69:ea,ipv6_src=fe80::200:86ff:fe05:80da,ipv6_dst=fe80::260:97ff:fe07:69ea,ipv6_label=0x00000,nw_tos=0,nw_ecn=0,nw_ttl=255,icmp_type=135,icmp_code=0,nd_target=fe80::260:97ff:fe07:69ea,nd_sll=00:00:86:05:80:da,nd_tll=00:00:00:00:00:00 icmp6_csum:68bd
])
OVS_VSWITCHD_STOP
diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
index da7b262..7f59555 100644
--- a/tests/ovs-ofctl.at
+++ b/tests/ovs-ofctl.at
@@ -193,20 +193,20 @@ actions=sample(probability=12345,collector_set_id=23456,obs_domain_id=34567,obs_
actions=ct(nat)
actions=ct(commit,nat(dst))
actions=ct(commit,nat(src))
-actions=ct(commit,nat(src=10.0.0.240,random))
-actions=ct(commit,nat(src=10.0.0.240:32768-65535,random))
-actions=ct(commit,nat(dst=10.0.0.128-10.0.0.254,hash))
-actions=ct(commit,nat(src=10.0.0.240-10.0.0.254:32768-65535,persistent))
-actions=ct(commit,nat(src=fe80::20c:29ff:fe88:a18b,random))
-actions=ct(commit,nat(src=fe80::20c:29ff:fe88:1-fe80::20c:29ff:fe88:a18b,random))
-actions=ct(commit,nat(src=[fe80::20c:29ff:fe88:1]-[fe80::20c:29ff:fe88:a18b]:255-4096,random))
-actions=ct(commit,nat(src=10.1.1.240-10.1.1.255),alg=ftp)
+ip,actions=ct(commit,nat(src=10.0.0.240,random))
+ip,actions=ct(commit,nat(src=10.0.0.240:32768-65535,random))
+ip,actions=ct(commit,nat(dst=10.0.0.128-10.0.0.254,hash))
+ip,actions=ct(commit,nat(src=10.0.0.240-10.0.0.254:32768-65535,persistent))
+ipv6,actions=ct(commit,nat(src=fe80::20c:29ff:fe88:a18b,random))
+ipv6,actions=ct(commit,nat(src=fe80::20c:29ff:fe88:1-fe80::20c:29ff:fe88:a18b,random))
+ipv6,actions=ct(commit,nat(src=[fe80::20c:29ff:fe88:1]-[fe80::20c:29ff:fe88:a18b]:255-4096,random))
+tcp,actions=ct(commit,nat(src=10.1.1.240-10.1.1.255),alg=ftp)
]])
AT_CHECK([ovs-ofctl parse-flows flows.txt
], [0], [stdout])
AT_CHECK([[sed 's/ (xid=0x[0-9a-fA-F]*)//' stdout]], [0],
-[[usable protocols: OpenFlow10,NXM
+[[usable protocols: any
chosen protocol: OpenFlow10-table_id
OFPT_FLOW_MOD: ADD tcp,tp_src=123 actions=FLOOD
OFPT_FLOW_MOD: ADD in_port=LOCAL,dl_vlan=9,dl_src=00:0a:e4:25:6b:b0 actions=drop
@@ -224,14 +224,14 @@ OFPT_FLOW_MOD: ADD actions=sample(probability=12345,collector_set_id=23456,obs_d
OFPT_FLOW_MOD: ADD actions=ct(nat)
OFPT_FLOW_MOD: ADD actions=ct(commit,nat(dst))
OFPT_FLOW_MOD: ADD actions=ct(commit,nat(src))
-OFPT_FLOW_MOD: ADD actions=ct(commit,nat(src=10.0.0.240,random))
-OFPT_FLOW_MOD: ADD actions=ct(commit,nat(src=10.0.0.240:32768-65535,random))
-OFPT_FLOW_MOD: ADD actions=ct(commit,nat(dst=10.0.0.128-10.0.0.254,hash))
-OFPT_FLOW_MOD: ADD actions=ct(commit,nat(src=10.0.0.240-10.0.0.254:32768-65535,persistent))
-OFPT_FLOW_MOD: ADD actions=ct(commit,nat(src=fe80::20c:29ff:fe88:a18b,random))
-OFPT_FLOW_MOD: ADD actions=ct(commit,nat(src=fe80::20c:29ff:fe88:1-fe80::20c:29ff:fe88:a18b,random))
-OFPT_FLOW_MOD: ADD actions=ct(commit,nat(src=[fe80::20c:29ff:fe88:1]-[fe80::20c:29ff:fe88:a18b]:255-4096,random))
-OFPT_FLOW_MOD: ADD actions=ct(commit,nat(src=10.1.1.240-10.1.1.255),alg=ftp)
+OFPT_FLOW_MOD: ADD ip actions=ct(commit,nat(src=10.0.0.240,random))
+OFPT_FLOW_MOD: ADD ip actions=ct(commit,nat(src=10.0.0.240:32768-65535,random))
+OFPT_FLOW_MOD: ADD ip actions=ct(commit,nat(dst=10.0.0.128-10.0.0.254,hash))
+OFPT_FLOW_MOD: ADD ip actions=ct(commit,nat(src=10.0.0.240-10.0.0.254:32768-65535,persistent))
+OFPT_FLOW_MOD: ADD ipv6 actions=ct(commit,nat(src=fe80::20c:29ff:fe88:a18b,random))
+OFPT_FLOW_MOD: ADD ipv6 actions=ct(commit,nat(src=fe80::20c:29ff:fe88:1-fe80::20c:29ff:fe88:a18b,random))
+OFPT_FLOW_MOD: ADD ipv6 actions=ct(commit,nat(src=[fe80::20c:29ff:fe88:1]-[fe80::20c:29ff:fe88:a18b]:255-4096,random))
+OFPT_FLOW_MOD: ADD tcp actions=ct(commit,nat(src=10.1.1.240-10.1.1.255),alg=ftp)
]])
AT_CLEANUP
--
2.1.4
More information about the dev
mailing list