Re: [ovs-dev] [PATCH] ofp-actions: Error on conntrack action inconsistency.

[Jarno Rajahalme]

> On Sep 27, 2016, at 11:06 AM, Joe Stringer  wrote:
> 
> On 26 September 2016 at 18:46, Jarno Rajahalme  > 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 
> 
> 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)
>> -#defineIPPORT_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(>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, 

Re: [ovs-dev] [PATCH] ofp-actions: Error on conntrack action inconsistency.

[Joe Stringer]

On 26 September 2016 at 18:46, Jarno Rajahalme  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 

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)
> -#defineIPPORT_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(>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 !=