> On Sep 27, 2016, at 11:06 AM, Joe Stringer <j...@ovn.org> wrote:
>
> On 26 September 2016 at 18:46, Jarno Rajahalme <ja...@ovn.org
> <mailto: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.
>
OK.
> * 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?
>
I’ll remove this change in v2 and separate out the changes to separate patches.
Thanks for the review,
Jarno
>> ---
>> 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 <mailto:dev@openvswitch.org>
>> http://openvswitch.org/mailman/listinfo/dev
>> <http://openvswitch.org/mailman/listinfo/dev>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
