On 26 September 2016 at 18:46, Jarno Rajahalme <ja...@ovn.org> wrote:
> 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>
I see about three separate changes here, it would be easier to
understand what you're trying to solve if they were proposed
separately. I also see a few cosmetic changes which are usually
handled separate from functional changes.
* Ensuring match on TCP for alg=ftp looks good
* Enforcing the action inconsistency check
Falling back to OF1.0 could allow one flow to execute CT() on any
{ip,ipv6} packets, so while it may fail it's also not a terrible
assumption that it may remedy the situation. That said, it sounds like
a reasonable change to me - I doubt that people are relying on this,
and it makes the errors more explicit by erroring back at the OF layer
rather than printing a message in a kernel log somewhere.
There may be an argument for the two above being bugfixes: people
running v2.5 LTS are more likely to run into unexpected translation
errors in the datapath without these checks being properly presented
at the OpenFlow layer.
* Removing IP check from action inconsistency check
I wouldn't mind some input from eg. OVN pipeline writers whether it is
useful to match on any proto (ie, ipv4+ipv6) to do CT(). This is a
basic tradeoff between pipeline flexibility and explicit erroring at
the OpenFlow layer. I could see someone wanting to build conjunctive
flows that do CT(), but it's disallowed today and no-one's complained,
so is it really that useful?
Regardless I'm not sure that silently skipping the CT() action is
quite right - shouldn't it complain that the flows are wrong?
> ---
> 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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
