]> git.proxmox.com Git - ovs.git/commitdiff
ofproto-dpif-trace: Improve NAT tracing.
authorDumitru Ceara <dceara@redhat.com>
Fri, 10 Jan 2020 09:34:43 +0000 (10:34 +0100)
committerBen Pfaff <blp@ovn.org>
Tue, 16 Jun 2020 22:07:31 +0000 (15:07 -0700)
When ofproto/trace detects a recirc action it resumes execution at the
specified next table. However, if the ct action performs SNAT/DNAT,
e.g., ct(commit,nat(src=1.1.1.1:4000),table=42), the src/dst IPs and
ports in the oftrace_recirc_node->flow field are not updated. This leads
to misleading outputs from ofproto/trace as real packets would actually
first get NATed and might match different flows when recirculated.

Assume the first IP/port from the NAT src/dst action will be used by
conntrack for the translation and update the oftrace_recirc_node->flow
accordingly. This is not entirely correct as conntrack might choose a
different IP/port but the result is more realistic than before.

This fix covers new connections. However, for reply traffic that executes
actions of the form ct(nat, table=42) we still don't update the flow as
we don't have any information about conntrack state when tracing.

Also move the oftrace_recirc_node processing out of ofproto_trace()
and to its own function, ofproto_trace_recirc_node() for better
readability/

Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
ofproto/ofproto-dpif-trace.c
ofproto/ofproto-dpif-trace.h
ofproto/ofproto-dpif-xlate.c
tests/ofproto-dpif.at

index 8ae8a221a897083c0b9eabe079cc63c1ad39bf1c..78a54c715dc731b3e28568b48ad42a9f95453d08 100644 (file)
@@ -86,6 +86,7 @@ oftrace_node_destroy(struct oftrace_node *node)
 bool
 oftrace_add_recirc_node(struct ovs_list *recirc_queue,
                         enum oftrace_recirc_type type, const struct flow *flow,
+                        const struct ofpact_nat *ofn,
                         const struct dp_packet *packet, uint32_t recirc_id,
                         const uint16_t zone)
 {
@@ -101,6 +102,7 @@ oftrace_add_recirc_node(struct ovs_list *recirc_queue,
     node->flow = *flow;
     node->flow.recirc_id = recirc_id;
     node->flow.ct_zone = zone;
+    node->nat_act = ofn;
     node->packet = packet ? dp_packet_clone(packet) : NULL;
 
     return true;
@@ -179,6 +181,25 @@ oftrace_node_print_details(struct ds *output,
     }
 }
 
+static void
+oftrace_print_ip_flow(const struct flow *flow, int af, struct ds *output)
+{
+    if (af == AF_INET) {
+        ds_put_format(output, "nw_src="IP_FMT",tp_src=%"PRIu16","
+                      "nw_dst="IP_FMT",tp_dst=%"PRIu16,
+                      IP_ARGS(flow->nw_src), ntohs(flow->tp_src),
+                      IP_ARGS(flow->nw_dst), ntohs(flow->tp_dst));
+    } else if (af == AF_INET6) {
+        ds_put_cstr(output, "ipv6_src=");
+        ipv6_format_addr_bracket(&flow->ipv6_src, output, true);
+        ds_put_format(output, ",tp_src=%"PRIu16, ntohs(flow->tp_src));
+        ds_put_cstr(output, ",ipv6_dst=");
+        ipv6_format_addr_bracket(&flow->ipv6_dst, output, true);
+        ds_put_format(output, ",tp_dst=%"PRIu16, ntohs(flow->tp_dst));
+    }
+    ds_put_char(output, '\n');
+}
+
 /* Parses the 'argc' elements of 'argv', ignoring argv[0].  The following
  * forms are supported:
  *
@@ -637,6 +658,73 @@ execute_actions_except_outputs(struct dpif *dpif,
     ofpbuf_uninit(&pruned_actions);
 }
 
+static void
+ofproto_trace_recirc_node(struct oftrace_recirc_node *node,
+                          struct ovs_list *next_ct_states,
+                          struct ds *output)
+{
+    ds_put_cstr(output, "\n\n");
+    ds_put_char_multiple(output, '=', 79);
+    ds_put_format(output, "\nrecirc(%#"PRIx32")", node->recirc_id);
+
+    if (next_ct_states && node->type == OFT_RECIRC_CONNTRACK) {
+        uint32_t ct_state;
+        if (ovs_list_is_empty(next_ct_states)) {
+            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);
+        }
+        node->flow.ct_state = ct_state;
+    }
+    ds_put_char(output, '\n');
+
+    /* If there's any snat/dnat information assume we always translate to
+     * the first IP/port to make sure we don't match on incorrect flows later
+     * on.
+     */
+    if (node->nat_act) {
+        const struct ofpact_nat *ofn = node->nat_act;
+
+        ds_put_cstr(output, "Replacing src/dst IP/ports to simulate NAT:\n");
+        ds_put_cstr(output, " Initial flow: ");
+        oftrace_print_ip_flow(&node->flow, ofn->range_af, output);
+
+        if (ofn->flags & NX_NAT_F_SRC) {
+            if (ofn->range_af == AF_INET) {
+                node->flow.nw_src = ofn->range.addr.ipv4.min;
+            } else if (ofn->range_af == AF_INET6) {
+                node->flow.ipv6_src = ofn->range.addr.ipv6.min;
+            }
+
+            if (ofn->range_af != AF_UNSPEC && ofn->range.proto.min) {
+                node->flow.tp_src = htons(ofn->range.proto.min);
+            }
+        }
+        if (ofn->flags & NX_NAT_F_DST) {
+            if (ofn->range_af == AF_INET) {
+                node->flow.nw_dst = ofn->range.addr.ipv4.min;
+            } else if (ofn->range_af == AF_INET6) {
+                node->flow.ipv6_dst = ofn->range.addr.ipv6.min;
+            }
+
+            if (ofn->range_af != AF_UNSPEC && ofn->range.proto.min) {
+                node->flow.tp_dst = htons(ofn->range.proto.min);
+            }
+        }
+        ds_put_cstr(output, " Modified flow: ");
+        oftrace_print_ip_flow(&node->flow, ofn->range_af, output);
+    }
+    ds_put_char_multiple(output, '=', 79);
+    ds_put_cstr(output, "\n\n");
+}
+
 static void
 ofproto_trace__(struct ofproto_dpif *ofproto, const struct flow *flow,
                 const struct dp_packet *packet, struct ovs_list *recirc_queue,
@@ -729,31 +817,7 @@ ofproto_trace(struct ofproto_dpif *ofproto, const struct flow *flow,
 
     struct oftrace_recirc_node *recirc_node;
     LIST_FOR_EACH_POP (recirc_node, node, &recirc_queue) {
-        ds_put_cstr(output, "\n\n");
-        ds_put_char_multiple(output, '=', 79);
-        ds_put_format(output, "\nrecirc(%#"PRIx32")",
-                      recirc_node->recirc_id);
-
-        if (next_ct_states && recirc_node->type == OFT_RECIRC_CONNTRACK) {
-            uint32_t ct_state;
-            if (ovs_list_is_empty(next_ct_states)) {
-                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);
-        ds_put_cstr(output, "\n\n");
-
+        ofproto_trace_recirc_node(recirc_node, next_ct_states, output);
         ofproto_trace__(ofproto, &recirc_node->flow, recirc_node->packet,
                         &recirc_queue, ofpacts, ofpacts_len, output);
         oftrace_recirc_node_destroy(recirc_node);
index 63dbb50bad5718145fd972c7d9276fdbfd692781..4b04f1756f0f0dcdb9245e6820c7599a43374b98 100644 (file)
@@ -73,6 +73,7 @@ struct oftrace_recirc_node {
     uint32_t recirc_id;
     struct flow flow;
     struct dp_packet *packet;
+    const struct ofpact_nat *nat_act;
 };
 
 /* A node within a next_ct_states list. */
@@ -91,6 +92,7 @@ struct oftrace_node *oftrace_report(struct ovs_list *, enum oftrace_node_type,
                                     const char *text);
 bool oftrace_add_recirc_node(struct ovs_list *recirc_queue,
                              enum oftrace_recirc_type, const struct flow *,
+                             const struct ofpact_nat *,
                              const struct dp_packet *, uint32_t recirc_id,
                              const uint16_t zone);
 
index 80fba84cb1a5227ef4e04be2c79d125f092c39dc..e64c6d477427642a81580089ca350c815fe44e2c 100644 (file)
@@ -4999,7 +4999,8 @@ compose_recirculate_and_fork(struct xlate_ctx *ctx, uint8_t table,
     if (OVS_UNLIKELY(ctx->xin->trace) && recirc_id) {
         if (oftrace_add_recirc_node(ctx->xin->recirc_queue,
                                     OFT_RECIRC_CONNTRACK, &ctx->xin->flow,
-                                    ctx->xin->packet, recirc_id, zone)) {
+                                    ctx->ct_nat_action, ctx->xin->packet,
+                                    recirc_id, zone)) {
             xlate_report(ctx, OFT_DETAIL, "A clone of the packet is forked to "
                          "recirculate. The forked pipeline will be resumed at "
                          "table %u.", table);
@@ -6205,7 +6206,6 @@ compose_conntrack_action(struct xlate_ctx *ctx, struct ofpact_conntrack *ofc,
     put_ct_label(&ctx->xin->flow, ctx->odp_actions, ctx->wc);
     put_ct_helper(ctx, ctx->odp_actions, ofc);
     put_ct_nat(ctx);
-    ctx->ct_nat_action = NULL;
     nl_msg_end_nested(ctx->odp_actions, ct_offset);
 
     ctx->wc->masks.ct_mark = old_ct_mark_mask;
@@ -6216,6 +6216,8 @@ compose_conntrack_action(struct xlate_ctx *ctx, struct ofpact_conntrack *ofc,
         compose_recirculate_and_fork(ctx, ofc->recirc_table, zone);
     }
 
+    ctx->ct_nat_action = NULL;
+
     /* The ct_* fields are only available in the scope of the 'recirc_table'
      * call chain. */
     flow_clear_conntrack(&ctx->xin->flow);
index 41164d735d8a4d843af148ce7e157373dfb06738..a03a63ac0801e6755a0082f8215e5fe12c812105 100644 (file)
@@ -10684,6 +10684,42 @@ AT_CHECK([tail -1 stdout], [0],
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
+AT_SETUP([ofproto-dpif - nat - ofproto/trace])
+OVS_VSWITCHD_START
+
+add_of_ports br0 1 2 3
+
+flow="in_port=1,udp,nw_src=1.1.1.1,nw_dst=1.1.1.2,udp_src=100,udp_dst=200"
+AT_DATA([flows.txt], [dnl
+table=0,priority=100,ip,nw_src=1.1.1.1,ct_state=-trk,action=ct(commit,nat(src=10.0.0.1-10.0.0.42:1000-1042),table=0)
+table=0,priority=100,udp,ct_state=+trk,nw_src=10.0.0.1,nw_dst=1.1.1.2,tp_src=1000,tp_dst=200,action=ct(commit,nat(dst=20.0.0.1-20.0.0.42:2000-2042),table=0)
+table=0,priority=100,udp,ct_state=+trk,nw_src=10.0.0.1,nw_dst=20.0.0.1,tp_src=1000,tp_dst=2000,action=3
+table=0,priority=90,ip,ct_state=+trk,action=2
+])
+AT_CHECK([ovs-ofctl del-flows br0])
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+AT_CHECK([ovs-appctl ofproto/trace br0 "$flow"], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0],
+  [Datapath actions: 3
+])
+
+flow="in_port=1,udp6,ipv6_src=1::1,ipv6_dst=1::2,udp_src=100,udp_dst=200"
+AT_DATA([flows.txt], [dnl
+table=0,priority=100,ip6,ipv6_src=1::1,ct_state=-trk,action=ct(commit,nat(src=[[10::1]]-[[10::42]]:1000-1042),table=0)
+table=0,priority=100,udp6,ct_state=+trk,ipv6_src=10::1,ipv6_dst=1::2,tp_src=1000,tp_dst=200,action=ct(commit,nat(dst=[[20::1]]-[[20::42]]:2000-2042),table=0)
+table=0,priority=100,udp6,ct_state=+trk,ipv6_src=10::1,ipv6_dst=20::1,tp_src=1000,tp_dst=2000,action=3
+table=0,priority=90,ip6,ct_state=+trk,action=2
+])
+AT_CHECK([ovs-ofctl del-flows br0])
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+AT_CHECK([ovs-appctl ofproto/trace br0 "$flow"], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0],
+  [Datapath actions: 3
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
 AT_SETUP([ofproto - set mtu])
 OVS_VSWITCHD_START