ofproto/trace: Add --ct-next option to ofproto/trace

Previous patch enables ofproto/trace to automatically trace a flow
that involves multiple recirculation on conntrack. However, it always
sets the ct_state to trk|est when it processes recirculated conntrack flows.
With this patch, users can customize the expected next ct_state in the
aforementioned use case.

Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>

NEWS

@@ -45,6 +45,7 @@ Post-v2.7.0
      * "ovn-sbctl lflow-list" can now print OpenFlow flows that correspond
        to logical flows.
      * Now uses OVSDB RBAC support to reduce impact of compromised hypervisors.
+   - Tracing with ofproto/trace now traces through recirculation.
    - OVSDB:
      * New support for role-based access control (see ovsdb-server(1)).
    - New commands 'stp/show' and 'rstp/show' (see ovs-vswitchd(8)).

ofproto/ofproto-dpif-trace.c

@@ -18,6 +18,7 @@
 
 #include "ofproto-dpif-trace.h"
 
+#include "conntrack.h"
 #include "dpif.h"
 #include "ofproto-dpif-xlate.h"
 #include "openvswitch/ofp-parse.h"
@@ -26,6 +27,7 @@
 static void ofproto_trace(struct ofproto_dpif *, const struct flow *,
                           const struct dp_packet *packet,
                           const struct ofpact[], size_t ofpacts_len,
+                          struct ovs_list *next_ct_states,
                           struct ds *);
 static void oftrace_node_destroy(struct oftrace_node *);
 
@@ -117,6 +119,25 @@ oftrace_recirc_node_destroy(struct oftrace_recirc_node *node)
     }
 }
 
+static void
+oftrace_push_ct_state(struct ovs_list *next_ct_states, uint32_t ct_state)
+{
+    struct oftrace_next_ct_state *next_ct_state =
+        xmalloc(sizeof *next_ct_state);
+    next_ct_state->state = ct_state;
+    ovs_list_push_back(next_ct_states, &next_ct_state->node);
+}
+
+static uint32_t
+oftrace_pop_ct_state(struct ovs_list *next_ct_states)
+{
+    struct oftrace_next_ct_state *s;
+    LIST_FOR_EACH_POP (s, node, next_ct_states) {
+        return s->state;
+    }
+    OVS_NOT_REACHED();
+}
+
 static void
 oftrace_node_print_details(struct ds *output,
                            const struct ovs_list *nodes, int level)
@@ -158,18 +179,49 @@ oftrace_node_print_details(struct ds *output,
     }
 }
 
+static char * OVS_WARN_UNUSED_RESULT
+parse_oftrace_options(int argc, const char *argv[],
+                      struct ovs_list *next_ct_states)
+{
+    int k;
+    struct ds ds = DS_EMPTY_INITIALIZER;
+
+    for (k = 0; k < argc; k++) {
+        if (!strncmp(argv[k], "--ct-next", 9)) {
+            if (k + 1 > argc) {
+                return xasprintf("Missing argument for option %s", argv[k]);
+            }
+
+            uint32_t ct_state;
+            if (!parse_ct_state(argv[++k], 0, &ct_state, &ds)) {
+                return ds_steal_cstr(&ds);
+            }
+            if (!validate_ct_state(ct_state, &ds)) {
+                return ds_steal_cstr(&ds);
+            }
+            oftrace_push_ct_state(next_ct_states, ct_state);
+        } else {
+            return xasprintf("Invalid option %s", argv[k]);
+        }
+    }
+
+    ds_destroy(&ds);
+    return NULL;
+}
+
 /* Parses the 'argc' elements of 'argv', ignoring argv[0].  The following
  * forms are supported:
  *
- *     - [dpname] odp_flow [-generate | packet]
+ *     - [dpname] odp_flow [OPTIONS] [-generate | packet]
- *     - bridge br_flow [-generate | packet]
+ *     - bridge br_flow [OPTIONS] [-generate | packet]
  *
  * On success, initializes '*ofprotop' and 'flow' and returns NULL.  On failure
  * returns a nonnull malloced error message. */
 static char * OVS_WARN_UNUSED_RESULT
 parse_flow_and_packet(int argc, const char *argv[],
                       struct ofproto_dpif **ofprotop, struct flow *flow,
-                      struct dp_packet **packetp)
+                      struct dp_packet **packetp,
+                      struct ovs_list *next_ct_states)
 {
     const struct dpif_backer *backer = NULL;
     const char *error = NULL;
@@ -178,6 +230,7 @@ parse_flow_and_packet(int argc, const char *argv[],
     struct dp_packet *packet;
     struct ofpbuf odp_key;
     struct ofpbuf odp_mask;
+    int first_option;
 
     ofpbuf_init(&odp_key, 0);
     ofpbuf_init(&odp_mask, 0);
@@ -197,6 +250,25 @@ parse_flow_and_packet(int argc, const char *argv[],
         error = NULL;
     }
 
+    /* Parse options. */
+    if (argc >= 4) {
+        if (!strncmp(argv[2], "--", 2)) {
+            first_option = 2;
+        } else if (!strncmp(argv[3], "--", 2)) {
+            first_option = 3;
+        } else {
+            error = "Syntax error: invalid option";
+            goto exit;
+        }
+
+        m_err = parse_oftrace_options(argc - first_option, argv + first_option,
+                                      next_ct_states);
+        if (m_err) {
+            goto exit;
+        }
+        argc = first_option;
+    }
+
     /* odp_flow can have its in_port specified as a name instead of port no.
      * We do not yet know whether a given flow is a odp_flow or a br_flow.
      * But, to know whether a flow is odp_flow through odp_flow_from_string(),
@@ -328,6 +400,14 @@ exit:
     return m_err;
 }
 
+static void
+free_ct_states(struct ovs_list *ct_states)
+{
+    while (!ovs_list_is_empty(ct_states)) {
+        oftrace_pop_ct_state(ct_states);
+    }
+}
+
 static void
 ofproto_unixctl_trace(struct unixctl_conn *conn, int argc, const char *argv[],
                       void *aux OVS_UNUSED)
@@ -336,13 +416,16 @@ ofproto_unixctl_trace(struct unixctl_conn *conn, int argc, const char *argv[],
     struct dp_packet *packet;
     char *error;
     struct flow flow;
+    struct ovs_list next_ct_states = OVS_LIST_INITIALIZER(&next_ct_states);
 
-    error = parse_flow_and_packet(argc, argv, &ofproto, &flow, &packet);
+    error = parse_flow_and_packet(argc, argv, &ofproto, &flow, &packet,
+                                  &next_ct_states);
     if (!error) {
         struct ds result;
 
         ds_init(&result);
-        ofproto_trace(ofproto, &flow, packet, NULL, 0, &result);
+        ofproto_trace(ofproto, &flow, packet, NULL, 0, &next_ct_states,
+                      &result);
         unixctl_command_reply(conn, ds_cstr(&result));
         ds_destroy(&result);
         dp_packet_delete(packet);
@@ -350,6 +433,7 @@ ofproto_unixctl_trace(struct unixctl_conn *conn, int argc, const char *argv[],
         unixctl_command_reply_error(conn, error);
         free(error);
     }
+    free_ct_states(&next_ct_states);
 }
 
 static void
@@ -364,6 +448,7 @@ ofproto_unixctl_trace_actions(struct unixctl_conn *conn, int argc,
     struct ds result;
     struct match match;
     uint16_t in_port;
+    struct ovs_list next_ct_states = OVS_LIST_INITIALIZER(&next_ct_states);
 
     /* Three kinds of error return values! */
     enum ofperr retval;
@@ -393,7 +478,8 @@ ofproto_unixctl_trace_actions(struct unixctl_conn *conn, int argc,
         enforce_consistency = false;
     }
 
-    error = parse_flow_and_packet(argc, argv, &ofproto, &match.flow, &packet);
+    error = parse_flow_and_packet(argc, argv, &ofproto, &match.flow, &packet,
+                                  &next_ct_states);
     if (error) {
         unixctl_command_reply_error(conn, error);
         free(error);
@@ -441,13 +527,14 @@ ofproto_unixctl_trace_actions(struct unixctl_conn *conn, int argc,
     }
 
     ofproto_trace(ofproto, &match.flow, packet,
-                  ofpacts.data, ofpacts.size, &result);
+                  ofpacts.data, ofpacts.size, &next_ct_states, &result);
     unixctl_command_reply(conn, ds_cstr(&result));
 
 exit:
     ds_destroy(&result);
     dp_packet_delete(packet);
     ofpbuf_uninit(&ofpacts);
+    free_ct_states(&next_ct_states);
 }
 
 static void
@@ -539,7 +626,7 @@ static void
 ofproto_trace(struct ofproto_dpif *ofproto, const struct flow *flow,
               const struct dp_packet *packet,
               const struct ofpact ofpacts[], size_t ofpacts_len,
-              struct ds *output)
+              struct ovs_list *next_ct_states, struct ds *output)
 {
     struct ovs_list recirc_queue = OVS_LIST_INITIALIZER(&recirc_queue);
     ofproto_trace__(ofproto, flow, packet, &recirc_queue,
@@ -551,10 +638,22 @@ ofproto_trace(struct ofproto_dpif *ofproto, const struct flow *flow,
         ds_put_char_multiple(output, '=', 79);
         ds_put_format(output, "\nrecirc(%#"PRIx32")",
                       recirc_node->recirc_id);
+
         if (recirc_node->type == OFT_RECIRC_CONNTRACK) {
-            recirc_node->flow.ct_state = CS_TRACKED | CS_NEW;
+            uint32_t ct_state;
-            ds_put_cstr(output, " - resume conntrack processing with "
+            if (ovs_list_is_empty(next_ct_states)) {
-                                "default ct_state=trk|new");
+                ct_state = CS_TRACKED | CS_NEW;
+                ds_put_cstr(output, " - resume conntrack with default "
+                            "ct_state=trk|new (use --ct-next to customize)");
+            } else {
+                ct_state = oftrace_pop_ct_state(next_ct_states);
+                struct ds s = DS_EMPTY_INITIALIZER;
+                format_flags(&s, ct_state_to_string, ct_state, '|');
+                ds_put_format(output, " - resume conntrack with ct_state=%s",
+                              ds_cstr(&s));
+                ds_destroy(&s);
+            }
+            recirc_node->flow.ct_state = ct_state;
         }
         ds_put_char(output, '\n');
         ds_put_char_multiple(output, '=', 79);
@@ -577,10 +676,11 @@ ofproto_dpif_trace_init(void)
 
     unixctl_command_register(
         "ofproto/trace",
-        "{[dp_name] odp_flow | bridge br_flow} [-generate|packet]",
+        "{[dp_name] odp_flow | bridge br_flow} [OPTIONS...] "
-        1, 3, ofproto_unixctl_trace, NULL);
+        "[-generate|packet]", 1, INT_MAX, ofproto_unixctl_trace, NULL);
     unixctl_command_register(
         "ofproto/trace-packet-out",
-        "[-consistent] {[dp_name] odp_flow | bridge br_flow} [-generate|packet] actions",
+        "[-consistent] {[dp_name] odp_flow | bridge br_flow} [OPTIONS...] "
-        2, 6, ofproto_unixctl_trace_actions, NULL);
+        "[-generate|packet] actions",
+        2, INT_MAX, ofproto_unixctl_trace_actions, NULL);
 }

ofproto/ofproto-dpif-trace.h

@@ -72,6 +72,12 @@ struct oftrace_recirc_node {
     struct dp_packet *packet;
 };
 
+/* A node within a next_ct_states list. */
+struct oftrace_next_ct_state {
+    struct ovs_list node;       /* In next_ct_states. */
+    uint32_t state;
+};
+
 void ofproto_dpif_trace_init(void);
 
 struct oftrace_node *oftrace_report(struct ovs_list *, enum oftrace_node_type,

ofproto/ofproto-unixctl.man

@@ -6,10 +6,10 @@ These commands manage the core OpenFlow switch implementation (called
 Lists the names of the running ofproto instances.  These are the names
 that may be used on \fBofproto/trace\fR.
 .
-.IP "\fBofproto/trace\fR [\fIdpname\fR] \fIodp_flow\fR [\fB\-generate \fR| \fIpacket\fR]"
+.IP "\fBofproto/trace\fR [\fIdpname\fR] \fIodp_flow\fR [\fIOPTIONS\fR] [\fB\-generate \fR| \fIpacket\fR]"
-.IQ "\fBofproto/trace\fR \fIbridge\fR \fIbr_flow\fR [\fB\-generate \fR| \fIpacket\fR]"
+.IQ "\fBofproto/trace\fR \fIbridge\fR \fIbr_flow\fR [\fIOPTIONS\fR] [\fB\-generate \fR| \fIpacket\fR]"
-.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR] [\fIdpname\fR] \fIodp_flow\fR [\fB\-generate \fR| \fIpacket\fR] \fIactions\fR"
+.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR] [\fIdpname\fR] \fIodp_flow\fR [\fIOPTIONS\fR] [\fB\-generate \fR| \fIpacket\fR] \fIactions\fR"
-.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR] \fIbridge\fR \fIbr_flow\fR [\fB\-generate \fR| \fIpacket\fR] \fIactions\fR"
+.IQ "\fBofproto/trace\-packet\-out\fR [\fB\-consistent\fR] \fIbridge\fR \fIbr_flow\fR [\fIOPTIONS\fR] [\fB\-generate \fR| \fIpacket\fR] \fIactions\fR"
 Traces the path of an imaginary packet through \fIswitch\fR and
 reports the path that it took.  The initial treatment of the packet
 varies based on the command:
@@ -48,6 +48,52 @@ wildcards.)  \fIbridge\fR names of the bridge through which
 .RE
 .
 .IP
+.RS
+\fBofproto/trace\fR supports the following options:
+.
+.IP "--ct-next \fIflags\fR"
+When the traced flow triggers conntrack actions, \fBofproto/trace\fR
+will automatically trace the forked packet processing pipeline with
+user specified ct_state.  This option sets the ct_state flags that the
+conntrack module will report. The \fIflags\fR must be a comma- or
+space-separated list of the following connection tracking flags:
+.
+.RS
+.IP \(bu
+\fBtrk\fR: Include to indicate connection tracking has taken place.
+.
+.IP \(bu
+\fBnew\fR: Include to indicate a new flow.
+.
+.IP \(bu
+\fBest\fR: Include to indicate an established flow.
+.
+.IP \(bu
+\fBrel\fR: Include to indicate a related flow.
+.
+.IP \(bu
+\fBrpl\fR: Include to indicate a reply flow.
+.
+.IP \(bu
+\fBinv\fR: Include to indicate a connection entry in a bad state.
+.
+.IP \(bu
+\fBdnat\fR: Include to indicate a packet whose destination IP address has been
+changed.
+.
+.IP \(bu
+\fBsnat\fR: Include to indicate a packet whose source IP address has been
+changed.
+.
+.RE
+.
+.IP
+When --ct-next is unspecified, or when there are fewer --ct-next options than
+ct actions, the \fIflags\fR default to trk,new.
+.
+.RE
+.
+.IP
 Most commonly, one specifies only a flow, using one of the forms
 above, but sometimes one might need to specify an actual packet
 instead of just a flow:

tests/completion.at

@@ -171,7 +171,7 @@ AT_CLEANUP
 
 
 # complex completion check - ofproto/trace
-# ofproto/trace {[dp_name] odp_flow | bridge br_flow} [-generate|packet]
+# ofproto/trace {[dp_name] odp_flow | bridge br_flow} [OPTIONS] [-generate|packet]
 # test expansion on 'dp|dp_name' and 'bridge'
 AT_SETUP([appctl-bashcomp - complex completion check 3])
 AT_SKIP_IF([test -z ${BASH_VERSION+x}])
@@ -209,7 +209,8 @@ AT_CHECK_UNQUOTED([GET_AVAIL(${INPUT})], [0])
 # set odp_flow to some random string, should go to the next level.
 INPUT="$(bash ovs-appctl-bashcomp.bash debug ovs-appctl ofproto/trace ovs-dummy "in_port(123),mac(),ip,tcp" TAB 2>&1)"
 MATCH="$(GET_COMP_STR([-generate], [-generate])
-GET_COMP_STR([packet], []))"
+GET_COMP_STR([packet], [])
+GET_COMP_STR([OPTIONS...], []))"
 AT_CHECK_UNQUOTED([GET_EXPAN(${INPUT})],
 [0], [dnl
 ${MATCH}
@@ -242,7 +243,8 @@ AT_CHECK_UNQUOTED([GET_AVAIL(${INPUT})], [0])
 # set argument to some random string, should go to the odp_flow path.
 INPUT="$(bash ovs-appctl-bashcomp.bash debug ovs-appctl ofproto/trace "in_port(123),mac(),ip,tcp" TAB 2>&1)"
 MATCH="$(GET_COMP_STR([-generate], [-generate])
-GET_COMP_STR([packet], []))"
+GET_COMP_STR([packet], [])
+GET_COMP_STR([OPTIONS...], []))"
 AT_CHECK_UNQUOTED([GET_EXPAN(${INPUT})],
 [0], [dnl
 ${MATCH}

tests/ofproto-dpif.at

@@ -5249,14 +5249,6 @@ Trailing garbage in packet data
 ovs-appctl: ovs-vswitchd: server returned an error
 ])
 
-# Test incorrect command: ofproto/trace with 4 arguments
-AT_CHECK([ovs-appctl ofproto/trace \
-  arg1, arg2, arg3, arg4], [2], [stdout],[stderr])
-AT_CHECK([tail -2 stderr], [0], [dnl
-"ofproto/trace" command takes at most 3 arguments
-ovs-appctl: ovs-vswitchd: server returned an error
-])
-
 # Test incorrect command: ofproto/trace with 0 argument
 AT_CHECK([ovs-appctl ofproto/trace ], [2], [stdout],[stderr])
 AT_CHECK([tail -2 stderr], [0], [dnl
@@ -9754,13 +9746,14 @@ AT_CLEANUP
 AT_SETUP([ofproto-dpif - conntrack - ofproto/trace])
 OVS_VSWITCHD_START
 
-add_of_ports br0 1 2
+add_of_ports br0 1 2 3 4
 
 AT_DATA([flows.txt], [dnl
 dnl Table 0
 dnl
 table=0,priority=100,arp,action=normal
 table=0,priority=10,udp,action=ct(table=1,zone=0)
+table=0,priority=10,tcp,action=ct(table=2,zone=1)
 table=0,priority=1,action=drop
 dnl
 dnl Table 1
@@ -9769,6 +9762,18 @@ table=1,priority=10,in_port=1,ct_state=+trk+new,udp,action=ct(commit,zone=0),2
 table=1,priority=10,in_port=1,ct_state=+trk+est,udp,action=2
 table=1,priority=10,in_port=2,ct_state=+trk+est,udp,action=1
 table=1,priority=1,action=drop
+dnl
+dnl Table 2
+dnl
+table=2,priority=10,in_port=1,tcp,ct_state=+trk+new,tcp,action=ct(commit,zone=1),ct(table=3,zone=2)
+table=2,priority=10,in_port=1,tcp,ct_state=+trk+est,tcp,action=ct(table=3,zone=2)
+table=2,priority=1,action=drop
+dnl
+dnl Table 3
+dnl
+table=3,priority=10,in_port=1,tcp,ct_state=+trk+new,tcp,action=ct(commit,zone=2),4
+table=3,priority=10,in_port=1,tcp,ct_state=+trk+est,tcp,action=3
+table=2,priority=1,action=drop
 ])
 
 AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
@@ -9783,6 +9788,16 @@ AT_CHECK([tail -1 stdout], [0],
   [Datapath actions: ct(commit),2
 ])
 
+AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,tcp'], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0],
+  [Datapath actions: ct(commit,zone=2),4
+])
+
+AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,tcp' --ct-next 'trk,est' --ct-next 'trk,est' ], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0],
+  [Datapath actions: 3
+])
+
 OVS_VSWITCHD_STOP
 AT_CLEANUP