diff mbox

[ovs-dev,v2,3/4] New action "ct_clear".

Message ID 20170106045811.12024-1-blp@ovn.org
State Superseded
Headers show

Commit Message

Ben Pfaff Jan. 6, 2017, 4:58 a.m. UTC
This is being introduced specifically to allow a user of the "clone" action
to clear the connection tracking state, but it's implemented as a separate
action as a matter of clean design and in case another use case arises
later.

Reported-by: Mickey Spiegel <mickeys.dev@gmail.com>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-January/326981.html
Fixes: 7ae62a676d3a ("ofp-actions: Add clone action.")
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Mickey Spiegel <mickeys.dev@gmail.com>
---
 NEWS                              |  2 +-
 include/openvswitch/ofp-actions.h |  3 +-
 lib/ofp-actions.c                 | 43 +++++++++++++++++++++++++++-
 ofproto/ofproto-dpif-xlate.c      | 17 ++++++++---
 tests/ofp-actions.at              |  3 ++
 tests/ofproto-dpif.at             | 60 +++++++++++++++++++++++++++++++++++++++
 utilities/ovs-ofctl.8.in          |  6 ++++
 7 files changed, 127 insertions(+), 7 deletions(-)

Comments

Mickey Spiegel Jan. 6, 2017, 8:54 a.m. UTC | #1
On Thu, Jan 5, 2017 at 8:58 PM, Ben Pfaff <blp@ovn.org> wrote:

> This is being introduced specifically to allow a user of the "clone" action
> to clear the connection tracking state, but it's implemented as a separate
> action as a matter of clean design and in case another use case arises
> later.
>
> Reported-by: Mickey Spiegel <mickeys.dev@gmail.com>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-
> January/326981.html
> Fixes: 7ae62a676d3a ("ofp-actions: Add clone action.")
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> Acked-by: Mickey Spiegel <mickeys.dev@gmail.com>
>

The patch applies now, but I am getting a test failure. See inline for that
plus one other nit.


> ---
>  NEWS                              |  2 +-
>  include/openvswitch/ofp-actions.h |  3 +-
>  lib/ofp-actions.c                 | 43 +++++++++++++++++++++++++++-
>  ofproto/ofproto-dpif-xlate.c      | 17 ++++++++---
>  tests/ofp-actions.at              |  3 ++
>  tests/ofproto-dpif.at             | 60 ++++++++++++++++++++++++++++++
> +++++++++
>  utilities/ovs-ofctl.8.in          |  6 ++++
>  7 files changed, 127 insertions(+), 7 deletions(-)
>
>
<snip>


> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index ceeaac6..347dc95 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -2874,12 +2874,15 @@ xlate_commit_actions(struct xlate_ctx *ctx)
>  }
>
>  static void
> -clear_conntrack(struct flow *flow)
> +clear_conntrack(struct xlate_ctx *ctx)
>  {
> +    ctx->conntracked = false;
> +
> +    struct flow *flow = &ctx->xin->flow;
>      flow->ct_state = 0;
>      flow->ct_zone = 0;
>      flow->ct_mark = 0;
> -    memset(&flow->ct_label, 0, sizeof flow->ct_label);
> +    flow->ct_label = OVS_U128_ZERO;
>  }
>
>  static bool
> @@ -2979,7 +2982,7 @@ compose_output_action__(struct xlate_ctx *ctx,
> ofp_port_t ofp_port,
>          memset(flow->regs, 0, sizeof flow->regs);
>          flow->actset_output = OFPP_UNSET;
>          ctx->conntracked = false;
>

Since clear_conntrack now sets ctx->conntracked = false, the line above is
unnecessary.


> -        clear_conntrack(flow);
> +        clear_conntrack(ctx);
>
>          /* When the patch port points to a different bridge, then the
> mirrors
>           * for that bridge clearly apply independently to the packet, so
> we
> @@ -4510,6 +4513,7 @@ freeze_unroll_actions(const struct ofpact *a, const
> struct ofpact *end,
>          case OFPACT_CLONE:
>          case OFPACT_DEBUG_RECIRC:
>          case OFPACT_CT:
> +        case OFPACT_CT_CLEAR:
>          case OFPACT_NAT:
>              /* These may not generate PACKET INs. */
>              break;
> @@ -4765,6 +4769,7 @@ recirc_for_mpls(const struct ofpact *a, struct
> xlate_ctx *ctx)
>      case OFPACT_CLONE:
>      case OFPACT_UNROLL_XLATE:
>      case OFPACT_CT:
> +    case OFPACT_CT_CLEAR:
>      case OFPACT_NAT:
>      case OFPACT_DEBUG_RECIRC:
>      case OFPACT_METER:
> @@ -5130,6 +5135,10 @@ do_xlate_actions(const struct ofpact *ofpacts,
> size_t ofpacts_len,
>              compose_conntrack_action(ctx, ofpact_get_CT(a));
>              break;
>
> +        case OFPACT_CT_CLEAR:
> +            clear_conntrack(ctx);
> +            break;
> +
>          case OFPACT_NAT:
>              /* This will be processed by compose_conntrack_action(). */
>              ctx->ct_nat_action = ofpact_get_NAT(a);
> @@ -5516,7 +5525,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
> *xout)
>          xlate_report(&ctx, "- Resuming from table %"PRIu8, ctx.table_id);
>
>          if (!state->conntracked) {
> -            clear_conntrack(flow);
> +            clear_conntrack(&ctx);
>          }
>
>          /* Restore pipeline metadata. May change flow's in_port and other
> diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at
> index 3881f9f..33d4bea 100644
> --- a/tests/ofp-actions.at
> +++ b/tests/ofp-actions.at
> @@ -247,6 +247,9 @@ fe800000 00000000 020c 29ff fe88 0001 dnl
>  fe800000 00000000 020c 29ff fe88 a18b dnl
>  00ff1000 00000000
>
> +# actions=ct_clear
> +ffff 0010 00002320 002a 000000000000
> +
>

I am getting a test failure due to this change:

## ---------------------- ##
## Detailed failed tests. ##
## ---------------------- ##

#                             -*- compilation -*-
137. ofp-actions.at:3: testing OpenFlow 1.0 action translation ...
./ofp-actions.at:281: ovs-ofctl '-vPATTERN:console:%c|%p|%m' parse-actions
OpenFlow10 < input.txt
--- expout      2017-01-05 23:43:29.892861427 -0800
+++ /home/mickey/ovs5/ovs/tests/testsuite.dir/at-groups/137/stdout
 2017-01-05 23:43:29.904861427 -0800
@@ -132,7 +132,7 @@

 actions=ct(commit,nat(src=[fe80::20c:29ff:fe88:1]-[fe80::20c:29ff:fe88:a18b]:255-4096,random))

-actions=ct_clear
+actions=clone(drop)

I changed 002a to 002b and it passed.


>  # actions=output(port=1,max_len=100)
>  ffff 0010 00002320 0027 0001 00000064
>

<snip>

Mickey
Ben Pfaff Jan. 6, 2017, 4:32 p.m. UTC | #2
On Fri, Jan 06, 2017 at 12:54:00AM -0800, Mickey Spiegel wrote:
> On Thu, Jan 5, 2017 at 8:58 PM, Ben Pfaff <blp@ovn.org> wrote:
> 
> > This is being introduced specifically to allow a user of the "clone" action
> > to clear the connection tracking state, but it's implemented as a separate
> > action as a matter of clean design and in case another use case arises
> > later.
> >
> > Reported-by: Mickey Spiegel <mickeys.dev@gmail.com>
> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-
> > January/326981.html
> > Fixes: 7ae62a676d3a ("ofp-actions: Add clone action.")
> > Signed-off-by: Ben Pfaff <blp@ovn.org>
> > Acked-by: Mickey Spiegel <mickeys.dev@gmail.com>
> >
> 
> The patch applies now, but I am getting a test failure. See inline for that
> plus one other nit.

Oops.  Those were foolish mistakes.

I'll post a v3.
diff mbox

Patch

diff --git a/NEWS b/NEWS
index 48ca679..0a9551c 100644
--- a/NEWS
+++ b/NEWS
@@ -35,7 +35,7 @@  Post-v2.6.0
        details.
      * The "sample" action now supports "ingress" and "egress" options.
      * The "ct" action now supports the TFTP ALG where support is available.
-     * New action "clone".
+     * New actions "clone" and "ct_clear".
    - ovs-ofctl:
      * 'bundle' command now supports packet-out messages.
      * New syntax for 'ovs-ofctl packet-out' command, which uses the
diff --git a/include/openvswitch/ofp-actions.h b/include/openvswitch/ofp-actions.h
index df9025c..8ca787a 100644
--- a/include/openvswitch/ofp-actions.h
+++ b/include/openvswitch/ofp-actions.h
@@ -1,5 +1,5 @@ 
 /*
- * Copyright (c) 2012, 2013, 2014, 2015, 2016 Nicira, Inc.
+ * Copyright (c) 2012, 2013, 2014, 2015, 2016, 2017 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -107,6 +107,7 @@ 
     OFPACT(SAMPLE,          ofpact_sample,      ofpact, "sample")       \
     OFPACT(UNROLL_XLATE,    ofpact_unroll_xlate, ofpact, "unroll_xlate") \
     OFPACT(CT,              ofpact_conntrack,   ofpact, "ct")           \
+    OFPACT(CT_CLEAR,        ofpact_null,        ofpact, "ct_clear")     \
     OFPACT(NAT,             ofpact_nat,         ofpact, "nat")          \
     OFPACT(OUTPUT_TRUNC,    ofpact_output_trunc,ofpact, "output_trunc") \
     OFPACT(CLONE,           ofpact_nest,        actions, "clone")       \
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index f29673f..4736521 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -1,5 +1,5 @@ 
 /*
- * Copyright (c) 2008-2016 Nicira, Inc.
+ * Copyright (c) 2008-2017 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -328,6 +328,9 @@  enum ofp_raw_action_type {
     /* NX1.0+(42): struct ext_action_header, ... */
     NXAST_RAW_CLONE,
 
+    /* NX1.0+(43): void. */
+    NXAST_RAW_CT_CLEAR,
+
 /* ## ------------------ ## */
 /* ## Debugging actions. ## */
 /* ## ------------------ ## */
@@ -449,6 +452,7 @@  ofpact_next_flattened(const struct ofpact *ofpact)
     case OFPACT_EXIT:
     case OFPACT_SAMPLE:
     case OFPACT_UNROLL_XLATE:
+    case OFPACT_CT_CLEAR:
     case OFPACT_DEBUG_RECIRC:
     case OFPACT_METER:
     case OFPACT_CLEAR_ACTIONS:
@@ -5495,6 +5499,36 @@  format_CT(const struct ofpact_conntrack *a, struct ds *s)
     ds_put_format(s, "%s)%s", colors.paren, colors.end);
 }
 
+/* ct_clear action. */
+
+static enum ofperr
+decode_NXAST_RAW_CT_CLEAR(struct ofpbuf *out)
+{
+    ofpact_put_CT_CLEAR(out);
+    return 0;
+}
+
+static void
+encode_CT_CLEAR(const struct ofpact_null *null OVS_UNUSED,
+                enum ofp_version ofp_version OVS_UNUSED,
+                struct ofpbuf *out)
+{
+    put_NXAST_CT_CLEAR(out);
+}
+
+static char * OVS_WARN_UNUSED_RESULT
+parse_CT_CLEAR(char *arg OVS_UNUSED, struct ofpbuf *ofpacts,
+               enum ofputil_protocol *usable_protocols OVS_UNUSED)
+{
+    ofpact_put_CT_CLEAR(ofpacts);
+    return NULL;
+}
+
+static void
+format_CT_CLEAR(const struct ofpact_null *a OVS_UNUSED, struct ds *s)
+{
+    ds_put_format(s, "%sct_clear%s", colors.value, colors.end);
+}
 /* NAT action. */
 
 /* Which optional fields are present? */
@@ -6280,6 +6314,7 @@  ofpact_is_set_or_move_action(const struct ofpact *a)
     case OFPACT_BUNDLE:
     case OFPACT_CLEAR_ACTIONS:
     case OFPACT_CT:
+    case OFPACT_CT_CLEAR:
     case OFPACT_CLONE:
     case OFPACT_NAT:
     case OFPACT_CONTROLLER:
@@ -6360,6 +6395,7 @@  ofpact_is_allowed_in_actions_set(const struct ofpact *a)
     case OFPACT_CLONE:
     case OFPACT_CONTROLLER:
     case OFPACT_CT:
+    case OFPACT_CT_CLEAR:
     case OFPACT_NAT:
     case OFPACT_ENQUEUE:
     case OFPACT_EXIT:
@@ -6594,6 +6630,7 @@  ovs_instruction_type_from_ofpact_type(enum ofpact_type type)
     case OFPACT_SAMPLE:
     case OFPACT_DEBUG_RECIRC:
     case OFPACT_CT:
+    case OFPACT_CT_CLEAR:
     case OFPACT_NAT:
     default:
         return OVSINST_OFPIT11_APPLY_ACTIONS;
@@ -7180,6 +7217,9 @@  ofpact_check__(enum ofputil_protocol *usable_protocols, struct ofpact *a,
                              usable_protocols);
     }
 
+    case OFPACT_CT_CLEAR:
+        return 0;
+
     case OFPACT_NAT: {
         struct ofpact_nat *on = ofpact_get_NAT(a);
 
@@ -7721,6 +7761,7 @@  ofpact_outputs_to_port(const struct ofpact *ofpact, ofp_port_t port)
     case OFPACT_GROUP:
     case OFPACT_DEBUG_RECIRC:
     case OFPACT_CT:
+    case OFPACT_CT_CLEAR:
     case OFPACT_NAT:
     default:
         return false;
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index ceeaac6..347dc95 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2874,12 +2874,15 @@  xlate_commit_actions(struct xlate_ctx *ctx)
 }
 
 static void
-clear_conntrack(struct flow *flow)
+clear_conntrack(struct xlate_ctx *ctx)
 {
+    ctx->conntracked = false;
+
+    struct flow *flow = &ctx->xin->flow;
     flow->ct_state = 0;
     flow->ct_zone = 0;
     flow->ct_mark = 0;
-    memset(&flow->ct_label, 0, sizeof flow->ct_label);
+    flow->ct_label = OVS_U128_ZERO;
 }
 
 static bool
@@ -2979,7 +2982,7 @@  compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
         memset(flow->regs, 0, sizeof flow->regs);
         flow->actset_output = OFPP_UNSET;
         ctx->conntracked = false;
-        clear_conntrack(flow);
+        clear_conntrack(ctx);
 
         /* When the patch port points to a different bridge, then the mirrors
          * for that bridge clearly apply independently to the packet, so we
@@ -4510,6 +4513,7 @@  freeze_unroll_actions(const struct ofpact *a, const struct ofpact *end,
         case OFPACT_CLONE:
         case OFPACT_DEBUG_RECIRC:
         case OFPACT_CT:
+        case OFPACT_CT_CLEAR:
         case OFPACT_NAT:
             /* These may not generate PACKET INs. */
             break;
@@ -4765,6 +4769,7 @@  recirc_for_mpls(const struct ofpact *a, struct xlate_ctx *ctx)
     case OFPACT_CLONE:
     case OFPACT_UNROLL_XLATE:
     case OFPACT_CT:
+    case OFPACT_CT_CLEAR:
     case OFPACT_NAT:
     case OFPACT_DEBUG_RECIRC:
     case OFPACT_METER:
@@ -5130,6 +5135,10 @@  do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
             compose_conntrack_action(ctx, ofpact_get_CT(a));
             break;
 
+        case OFPACT_CT_CLEAR:
+            clear_conntrack(ctx);
+            break;
+
         case OFPACT_NAT:
             /* This will be processed by compose_conntrack_action(). */
             ctx->ct_nat_action = ofpact_get_NAT(a);
@@ -5516,7 +5525,7 @@  xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
         xlate_report(&ctx, "- Resuming from table %"PRIu8, ctx.table_id);
 
         if (!state->conntracked) {
-            clear_conntrack(flow);
+            clear_conntrack(&ctx);
         }
 
         /* Restore pipeline metadata. May change flow's in_port and other
diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at
index 3881f9f..33d4bea 100644
--- a/tests/ofp-actions.at
+++ b/tests/ofp-actions.at
@@ -247,6 +247,9 @@  fe800000 00000000 020c 29ff fe88 0001 dnl
 fe800000 00000000 020c 29ff fe88 a18b dnl
 00ff1000 00000000
 
+# actions=ct_clear
+ffff 0010 00002320 002a 000000000000
+
 # actions=output(port=1,max_len=100)
 ffff 0010 00002320 0027 0001 00000064
 
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index e35a806..4d0f935 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -9150,6 +9150,66 @@  n_packets=0
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
+dnl This is a truncated version of "ofproto-dpif - conntrack - controller",
+dnl with extra send-to-controller actions following ct_clear to show that
+dnl the connection tracking data has been cleared.
+AT_SETUP([ofproto-dpif - conntrack - ct_clear])
+OVS_VSWITCHD_START
+
+add_of_ports br0 1 2
+
+AT_CHECK([ovs-appctl vlog/set dpif_netdev:dbg vconn:info ofproto_dpif:info])
+
+dnl Allow new connections on p1->p2, but not on p2->p1.
+AT_DATA([flows.txt], [dnl
+dnl Table 0
+dnl
+table=0,priority=100,arp,action=normal
+table=0,priority=10,in_port=1,udp,action=ct(commit,zone=0),controller,ct_clear,controller
+table=0,priority=10,in_port=2,udp,action=ct(table=1,zone=0)
+table=0,priority=1,action=drop
+dnl
+dnl Table 1
+dnl
+table=1,priority=10,in_port=2,ct_state=+trk+est-new,udp,action=controller,ct_clear,controller
+table=1,priority=1,action=drop
+])
+
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+
+AT_CAPTURE_FILE([ofctl_monitor.log])
+AT_CHECK([ovs-ofctl monitor br0 65534 invalid_ttl -P nxt_packet_in --detach --no-chdir --pidfile 2> ofctl_monitor.log])
+
+AT_CHECK([ovs-appctl netdev-dummy/receive p2 'in_port(2),eth(src=50:54:00:00:00:0a,dst=50:54:00:00:00:09),eth_type(0x0800),ipv4(src=10.1.1.2,dst=10.1.1.1,proto=17,tos=0,ttl=64,frag=no),udp(src=2,dst=1)'])
+
+dnl OK, now start a new connection from port 1.
+AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.1.1.1,dst=10.1.1.2,proto=17,tos=0,ttl=64,frag=no),udp(src=1,dst=2)'])
+
+dnl Now try a reply from port 2.
+AT_CHECK([ovs-appctl netdev-dummy/receive p2 'in_port(2),eth(src=50:54:00:00:00:0a,dst=50:54:00:00:00:09),eth_type(0x0800),ipv4(src=10.1.1.2,dst=10.1.1.1,proto=17,tos=0,ttl=64,frag=no),udp(src=2,dst=1)'])
+
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 8])
+OVS_WAIT_UNTIL([ovs-appctl -t ovs-ofctl exit])
+
+dnl Check this output. We only see the latter two packets, not the first.
+dnl Note that the first packet doesn't have the ct_state bits set. This
+dnl happens because the ct_state field is available only after recirc.
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+NXT_PACKET_IN (xid=0x0): cookie=0x0 total_len=42 in_port=1 (via action) data_len=42 (unbuffered)
+udp,vlan_tci=0x0000,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,nw_src=10.1.1.1,nw_dst=10.1.1.2,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=1,tp_dst=2 udp_csum:e9d6
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0x0 total_len=42 in_port=1 (via action) data_len=42 (unbuffered)
+udp,vlan_tci=0x0000,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,nw_src=10.1.1.1,nw_dst=10.1.1.2,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=1,tp_dst=2 udp_csum:e9d6
+dnl
+NXT_PACKET_IN (xid=0x0): table_id=1 cookie=0x0 total_len=42 ct_state=est|rpl|trk,in_port=2 (via action) data_len=42 (unbuffered)
+udp,vlan_tci=0x0000,dl_src=50:54:00:00:00:0a,dl_dst=50:54:00:00:00:09,nw_src=10.1.1.2,nw_dst=10.1.1.1,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=2,tp_dst=1 udp_csum:e9d6
+dnl
+NXT_PACKET_IN (xid=0x0): table_id=1 cookie=0x0 total_len=42 in_port=2 (via action) data_len=42 (unbuffered)
+udp,vlan_tci=0x0000,dl_src=50:54:00:00:00:0a,dl_dst=50:54:00:00:00:09,nw_src=10.1.1.2,nw_dst=10.1.1.1,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=2,tp_dst=1 udp_csum:e9d6
+])
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
 AT_SETUP([ofproto - set mtu])
 OVS_VSWITCHD_START
 
diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
index 0fdaa48..17a049e 100644
--- a/utilities/ovs-ofctl.8.in
+++ b/utilities/ovs-ofctl.8.in
@@ -1973,6 +1973,12 @@  Currently, connection tracking is only available on Linux kernels with the
 nf_conntrack module loaded. The \fBct\fR action was introduced in Open vSwitch
 2.5.
 .
+.IP \fBct_clear\fR
+Clears connection tracking state from the flow, zeroing
+\fBct_state\fR, \fBct_zone\fR, \fBct_mark\fR, and \fBct_label\fR.
+.IP
+This action was introduced in Open vSwitch 2.6.90.
+.
 .IP \fBdec_ttl\fR
 .IQ \fBdec_ttl(\fIid1\fR[\fB,\fIid2\fR]...\fB)\fR
 Decrement TTL of IPv4 packet or hop limit of IPv6 packet.  If the