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 <ja...@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 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev