diff mbox

[ovs-dev] tunneling: Track recursion levels across ARP generation.

Message ID 1442375322-75770-1-git-send-email-jesse@nicira.com
State Accepted
Headers show

Commit Message

Jesse Gross Sept. 16, 2015, 3:48 a.m. UTC 
If a packet is output to a tunnel port when userspace tunneling is
enabled, it will cause an ARP packet to be generated if the destination
is unknown. This ARP packet is injected into the physical bridge as
a new packet, where it is flooded.

If there is a loop (such as if the tunnel destination is the same bridge),
the result will be infinite recursion. Even though we currently track
recursion limits, they are not effective here since each ARP packet is
considered to be a new translation. This changes the behavior so that
each ARP flow translation is initialized with the recursion counter of
the previous flow. Note that the problem only applies to ARP - data
packets in a loop will hit an existing recursion counter in the datapath.

An additional side effect of this change is that ARP packets are no
longer unconditionally flooded in the new bridge. They will now follow any
flow rules in the new bridge that might apply to them, the same as with
the kernel datapath.

Reported-by: David Evans <davidjoshuaevans@gmail.com>
Signed-off-by: Jesse Gross <jesse@nicira.com>
---
 ofproto/ofproto-dpif-xlate.c | 28 +++++++++++++++-------------
 ofproto/ofproto-dpif-xlate.h | 14 ++++++++++++++
 ofproto/ofproto-dpif.c       | 28 +++++++++++++++++++++-------
 ofproto/ofproto-dpif.h       |  4 ++++
 4 files changed, 54 insertions(+), 20 deletions(-)

Comments

Pravin B Shelar Sept. 17, 2015, 9:42 p.m. UTC | #1 
On Tue, Sep 15, 2015 at 8:48 PM, Jesse Gross <jesse@nicira.com> wrote:
> If a packet is output to a tunnel port when userspace tunneling is
> enabled, it will cause an ARP packet to be generated if the destination
> is unknown. This ARP packet is injected into the physical bridge as
> a new packet, where it is flooded.
>
> If there is a loop (such as if the tunnel destination is the same bridge),
> the result will be infinite recursion. Even though we currently track
> recursion limits, they are not effective here since each ARP packet is
> considered to be a new translation. This changes the behavior so that
> each ARP flow translation is initialized with the recursion counter of
> the previous flow. Note that the problem only applies to ARP - data
> packets in a loop will hit an existing recursion counter in the datapath.
>
> An additional side effect of this change is that ARP packets are no
> longer unconditionally flooded in the new bridge. They will now follow any
> flow rules in the new bridge that might apply to them, the same as with
> the kernel datapath.
>
> Reported-by: David Evans <davidjoshuaevans@gmail.com>
> Signed-off-by: Jesse Gross <jesse@nicira.com>

LGTM
Acked-by: Pravin B Shelar <pshelar@nicira.com>
Jesse Gross Sept. 18, 2015, 12:12 a.m. UTC | #2 
On Thu, Sep 17, 2015 at 2:42 PM, Pravin Shelar <pshelar@nicira.com> wrote:
> On Tue, Sep 15, 2015 at 8:48 PM, Jesse Gross <jesse@nicira.com> wrote:
>> If a packet is output to a tunnel port when userspace tunneling is
>> enabled, it will cause an ARP packet to be generated if the destination
>> is unknown. This ARP packet is injected into the physical bridge as
>> a new packet, where it is flooded.
>>
>> If there is a loop (such as if the tunnel destination is the same bridge),
>> the result will be infinite recursion. Even though we currently track
>> recursion limits, they are not effective here since each ARP packet is
>> considered to be a new translation. This changes the behavior so that
>> each ARP flow translation is initialized with the recursion counter of
>> the previous flow. Note that the problem only applies to ARP - data
>> packets in a loop will hit an existing recursion counter in the datapath.
>>
>> An additional side effect of this change is that ARP packets are no
>> longer unconditionally flooded in the new bridge. They will now follow any
>> flow rules in the new bridge that might apply to them, the same as with
>> the kernel datapath.
>>
>> Reported-by: David Evans <davidjoshuaevans@gmail.com>
>> Signed-off-by: Jesse Gross <jesse@nicira.com>
>
> LGTM
> Acked-by: Pravin B Shelar <pshelar@nicira.com>

Thanks, applied to master. (I only did master because I think there
have been enough things that have come up in the past week that I
think people looking to use userspace tunneling need to be using
master.)
diff mbox

Patch

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 81838be..4ed73a3 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2690,36 +2690,36 @@  tnl_route_lookup_flow(const struct flow *oflow,
 }
 
 static int
-xlate_flood_packet(struct xbridge *xbridge, struct dp_packet *packet)
+compose_table_xlate(struct xlate_ctx *ctx, const struct xport *out_dev,
+                    struct dp_packet *packet)
 {
+    struct xbridge *xbridge = out_dev->xbridge;
     struct ofpact_output output;
     struct flow flow;
 
     ofpact_init(&output.ofpact, OFPACT_OUTPUT, sizeof output);
-    /* Use OFPP_NONE as the in_port to avoid special packet processing. */
     flow_extract(packet, &flow);
-    flow.in_port.ofp_port = OFPP_NONE;
-    output.port = OFPP_FLOOD;
+    flow.in_port.ofp_port = out_dev->ofp_port;
+    output.port = OFPP_TABLE;
     output.max_len = 0;
 
-    return ofproto_dpif_execute_actions(xbridge->ofproto, &flow, NULL,
-                                        &output.ofpact, sizeof output,
-                                        packet);
+    return ofproto_dpif_execute_actions__(xbridge->ofproto, &flow, NULL,
+                                          &output.ofpact, sizeof output,
+                                          ctx->recurse, ctx->resubmits, packet);
 }
 
 static void
-tnl_send_arp_request(const struct xport *out_dev,
+tnl_send_arp_request(struct xlate_ctx *ctx, const struct xport *out_dev,
                      const struct eth_addr eth_src,
                      ovs_be32 ip_src, ovs_be32 ip_dst)
 {
-    struct xbridge *xbridge = out_dev->xbridge;
     struct dp_packet packet;
 
     dp_packet_init(&packet, 0);
     compose_arp(&packet, ARP_OP_REQUEST,
                 eth_src, eth_addr_zero, true, ip_src, ip_dst);
 
-    xlate_flood_packet(xbridge, &packet);
+    compose_table_xlate(ctx, out_dev, &packet);
     dp_packet_uninit(&packet);
 }
 
@@ -2760,7 +2760,7 @@  build_tunnel_send(struct xlate_ctx *ctx, const struct xport *xport,
         xlate_report(ctx, "ARP cache miss for "IP_FMT" on bridge %s, "
                      "sending ARP request",
                      IP_ARGS(d_ip), out_dev->xbridge->name);
-        tnl_send_arp_request(out_dev, smac, s_ip, d_ip);
+        tnl_send_arp_request(ctx, out_dev, smac, s_ip, d_ip);
         return err;
     }
     if (ctx->xin->xcache) {
@@ -4521,6 +4521,8 @@  xlate_in_init(struct xlate_in *xin, struct ofproto_dpif *ofproto,
     xin->resubmit_hook = NULL;
     xin->report_hook = NULL;
     xin->resubmit_stats = NULL;
+    xin->recurse = 0;
+    xin->resubmits = 0;
     xin->wc = wc;
     xin->odp_actions = odp_actions;
 
@@ -4768,8 +4770,8 @@  xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
         .wc = xin->wc ? xin->wc : &scratch_wc,
         .odp_actions = xin->odp_actions ? xin->odp_actions : &scratch_actions,
 
-        .recurse = 0,
-        .resubmits = 0,
+        .recurse = xin->recurse,
+        .resubmits = xin->resubmits,
         .in_group = false,
         .in_action_set = false,
 
diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
index 91b913b..585650c 100644
--- a/ofproto/ofproto-dpif-xlate.h
+++ b/ofproto/ofproto-dpif-xlate.h
@@ -163,6 +163,20 @@  struct xlate_in {
      * calling xlate_in_init(). */
     const struct dpif_flow_stats *resubmit_stats;
 
+    /* Recursion and resubmission levels carried over from a pre-existing
+     * translation of a related flow. An example of when this can occur is
+     * the translation of an ARP packet that was generated as the result of
+     * outputting to a tunnel port. In this case, the original flow going to
+     * the tunnel is the related flow. Since the two flows are different, they
+     * should not use the same xlate_ctx structure. However, we still need
+     * limit the maximum recursion across the entire translation.
+     *
+     * These fields are normally set to zero, so the client has to set them
+     * manually after calling xlate_in_init(). In that case, they should be
+     * copied from the same-named fields in the related flow's xlate_ctx. */
+    int recurse;
+    int resubmits;
+
     /* If nonnull, flow translation populates this cache with references to all
      * modules that are affected by translation. This 'xlate_cache' may be
      * passed to xlate_push_stats() to perform the same function as
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 1279907..cffedb9 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -3636,14 +3636,13 @@  rule_expire(struct rule_dpif *rule)
     }
 }
 
-/* Executes, within 'ofproto', the actions in 'rule' or 'ofpacts' on 'packet'.
- * 'flow' must reflect the data in 'packet'. */
 int
-ofproto_dpif_execute_actions(struct ofproto_dpif *ofproto,
-                             const struct flow *flow,
-                             struct rule_dpif *rule,
-                             const struct ofpact *ofpacts, size_t ofpacts_len,
-                             struct dp_packet *packet)
+ofproto_dpif_execute_actions__(struct ofproto_dpif *ofproto,
+                               const struct flow *flow,
+                               struct rule_dpif *rule,
+                               const struct ofpact *ofpacts, size_t ofpacts_len,
+                               int recurse, int resubmits,
+                               struct dp_packet *packet)
 {
     struct dpif_flow_stats stats;
     struct xlate_out xout;
@@ -3667,6 +3666,8 @@  ofproto_dpif_execute_actions(struct ofproto_dpif *ofproto,
     xin.ofpacts = ofpacts;
     xin.ofpacts_len = ofpacts_len;
     xin.resubmit_stats = &stats;
+    xin.recurse = recurse;
+    xin.resubmits = resubmits;
     xlate_actions(&xin, &xout);
 
     execute.actions = odp_actions.data;
@@ -3692,6 +3693,19 @@  ofproto_dpif_execute_actions(struct ofproto_dpif *ofproto,
     return error;
 }
 
+/* Executes, within 'ofproto', the actions in 'rule' or 'ofpacts' on 'packet'.
+ * 'flow' must reflect the data in 'packet'. */
+int
+ofproto_dpif_execute_actions(struct ofproto_dpif *ofproto,
+                             const struct flow *flow,
+                             struct rule_dpif *rule,
+                             const struct ofpact *ofpacts, size_t ofpacts_len,
+                             struct dp_packet *packet)
+{
+    return ofproto_dpif_execute_actions__(ofproto, flow, rule, ofpacts,
+                                          ofpacts_len, 0, 0, packet);
+}
+
 void
 rule_dpif_credit_stats(struct rule_dpif *rule,
                        const struct dpif_flow_stats *stats)
diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
index 69ca54c..2397fb4 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -153,6 +153,10 @@  bool vsp_adjust_flow(const struct ofproto_dpif *, struct flow *,
 int ofproto_dpif_execute_actions(struct ofproto_dpif *, const struct flow *,
                                  struct rule_dpif *, const struct ofpact *,
                                  size_t ofpacts_len, struct dp_packet *);
+int ofproto_dpif_execute_actions__(struct ofproto_dpif *, const struct flow *,
+                                   struct rule_dpif *, const struct ofpact *,
+                                   size_t ofpacts_len, int recurse,
+                                   int resubmits, struct dp_packet *);
 void ofproto_dpif_send_packet_in(struct ofproto_dpif *,
                                  struct ofproto_packet_in *);
 bool ofproto_dpif_wants_packet_in_on_miss(struct ofproto_dpif *);