ARP: ignore non-connected routes and non-interface sources when determing if source is connected

Change-Id: I39fb0ec44cc322eaa12c0ff0700fc405d3982bfc
Signed-off-by: Neale Ranns <nranns@cisco.com>

3 files changed, 174 insertions, 35 deletions

diff --git a/src/vnet/ethernet/arp.c b/src/vnet/ethernet/arp.c
index 619628b3..d5dc9cce 100644
--- a/src/vnet/ethernet/arp.c
+++ b/src/vnet/ethernet/arp.c
@@ -22,6 +22,7 @@
 #include <vnet/l2/l2_input.h>
 #include <vppinfra/mhash.h>
 #include <vnet/fib/ip4_fib.h>
+#include <vnet/fib/fib_entry_src.h>
 #include <vnet/adj/adj_nbr.h>
 #include <vnet/adj/adj_mcast.h>
 #include <vnet/mpls/mpls.h>
@@ -955,52 +956,118 @@ arp_input (vlib_main_t * vm, vlib_node_runtime_t * node, vlib_frame_t * frame)
 					  32);
 	  dst_flags = fib_entry_get_flags (dst_fei);
 
-	  src_fei = ip4_fib_table_lookup (ip4_fib_get (fib_index0),
-					  &arp0->ip4_over_ethernet[0].ip4,
-					  32);
-	  src_flags = fib_entry_get_flags (src_fei);
-
 	  conn_sw_if_index0 = fib_entry_get_resolving_interface (dst_fei);
 
-	  if (!(FIB_ENTRY_FLAG_CONNECTED & dst_flags))
-	    {
-	      error0 = ETHERNET_ARP_ERROR_l3_dst_address_not_local;
-	      goto drop1;
-	    }
-
 	  /* Honor unnumbered interface, if any */
 	  is_unnum0 = sw_if_index0 != conn_sw_if_index0;
 
-	  /* Source must also be local to subnet of matching interface address. */
-	  if (!((FIB_ENTRY_FLAG_ATTACHED & src_flags) ||
-		(FIB_ENTRY_FLAG_CONNECTED & src_flags)))
+	  {
+	    /*
+	     * we're looking for FIB entries that indicate the source
+	     * is attached. There may be more specific non-attached
+	     * routes tht match the source, but these do not influence
+	     * whether we respond to an ARP request, i.e. they do not
+	     * influence whether we are the correct way for the sender
+	     * to reach us, they only affect how we reach the sender.
+	     */
+	    fib_entry_t *src_fib_entry;
+	    fib_entry_src_t *src;
+	    fib_source_t source;
+	    fib_prefix_t pfx;
+	    int attached;
+	    int mask;
+
+	    mask = 32;
+	    attached = 0;
+
+	    do
+	      {
+		src_fei = ip4_fib_table_lookup (ip4_fib_get (fib_index0),
+						&arp0->
+						ip4_over_ethernet[0].ip4,
+						mask);
+		src_fib_entry = fib_entry_get (src_fei);
+
+		/*
+		 * It's possible that the source that provides the
+		 * flags we need, or the flags we must not have,
+		 * is not the best source, so check then all.
+		 */
+                /* *INDENT-OFF* */
+                FOR_EACH_SRC_ADDED(src_fib_entry, src, source,
+                ({
+                  src_flags = fib_entry_get_flags_for_source (src_fei, source);
+
+                  /* Reject requests/replies with our local interface
+                     address. */
+                  if (FIB_ENTRY_FLAG_LOCAL & src_flags)
+                    {
+                      error0 = ETHERNET_ARP_ERROR_l3_src_address_is_local;
+                      goto drop2;
+                    }
+                  /* A Source must also be local to subnet of matching
+                   * interface address. */
+                  if ((FIB_ENTRY_FLAG_ATTACHED & src_flags) ||
+                      (FIB_ENTRY_FLAG_CONNECTED & src_flags))
+                    {
+                      attached = 1;
+                      break;
+                    }
+                  /*
+                   * else
+                   *  The packet was sent from an address that is not
+                   *  connected nor attached i.e. it is not from an
+                   *  address that is covered by a link's sub-net,
+                   *  nor is it a already learned host resp.
+                   */
+                }));
+                /* *INDENT-ON* */
+
+		/*
+		 * shorter mask lookup for the next iteration.
+		 */
+		fib_entry_get_prefix (src_fei, &pfx);
+		mask = pfx.fp_len - 1;
+
+		/*
+		 * continue until we hit the default route or we find
+		 * the attached we are looking for. The most likely
+		 * outcome is we find the attached with the first source
+		 * on the first lookup.
+		 */
+	      }
+	    while (!attached &&
+		   !fib_entry_is_sourced (src_fei, FIB_SOURCE_DEFAULT_ROUTE));
+
+	    if (!attached)
+	      {
+		/*
+		 * the matching route is a not attached, i.e. it was
+		 * added as a result of routing, rather than interface/ARP
+		 * configuration. If the matching route is not a host route
+		 * (i.e. a /32)
+		 */
+		error0 = ETHERNET_ARP_ERROR_l3_src_address_not_local;
+		goto drop2;
+	      }
+	  }
+
+	  if (!(FIB_ENTRY_FLAG_CONNECTED & dst_flags))
 	    {
-	      /*
-	       * The packet was sent from an address that is not connected nor attached
-	       * i.e. it is not from an address that is covered by a link's sub-net,
-	       * nor is it a already learned host resp.
-	       */
-	      error0 = ETHERNET_ARP_ERROR_l3_src_address_not_local;
-	      goto drop2;
+	      error0 = ETHERNET_ARP_ERROR_l3_dst_address_not_local;
+	      goto drop1;
 	    }
 
 	  if (sw_if_index0 != fib_entry_get_resolving_interface (src_fei))
 	    {
 	      /*
 	       * The interface the ARP was received on is not the interface
-	       * on which the covering prefix is configured. Maybe this is a case
-	       * for unnumbered.
+	       * on which the covering prefix is configured. Maybe this is a
+	       * case for unnumbered.
 	       */
 	      is_unnum0 = 1;
 	    }
 
-	  /* Reject requests/replies with our local interface address. */
-	  if (FIB_ENTRY_FLAG_LOCAL & src_flags)
-	    {
-	      error0 = ETHERNET_ARP_ERROR_l3_src_address_is_local;
-	      goto drop2;
-	    }
-
 	  dst_is_local0 = (FIB_ENTRY_FLAG_LOCAL & dst_flags);
 	  fib_entry_get_prefix (dst_fei, &pfx0);
 	  if_addr0 = &pfx0.fp_addr.ip4;
diff --git a/src/vnet/fib/fib_entry.c b/src/vnet/fib/fib_entry.c
index cdebfbce..1143f05a 100644
--- a/src/vnet/fib/fib_entry.c
+++ b/src/vnet/fib/fib_entry.c
@@ -462,11 +462,15 @@ fib_entry_get_adj (fib_node_index_t fib_entry_index)
     const dpo_id_t *dpo;
 
     dpo = fib_entry_contribute_ip_forwarding(fib_entry_index);
-    dpo = load_balance_get_bucket(dpo->dpoi_index, 0);
 
-    if (dpo_is_adj(dpo))
+    if (dpo_id_is_valid(dpo))
     {
-	return (dpo->dpoi_index);
+        dpo = load_balance_get_bucket(dpo->dpoi_index, 0);
+
+        if (dpo_is_adj(dpo))
+        {
+            return (dpo->dpoi_index);
+        }
     }
     return (ADJ_INDEX_INVALID);
 }
diff --git a/test/test_neighbor.py b/test/test_neighbor.py
index 1167b26b..b4a6878d 100644
--- a/test/test_neighbor.py
+++ b/test/test_neighbor.py
@@ -142,7 +142,7 @@ class ARPTestCase(VppTestCase):
         #
         # Generate some hosts on the LAN
         #
-        self.pg1.generate_remote_hosts(10)
+        self.pg1.generate_remote_hosts(11)
 
         #
         # Send IP traffic to one of these unresolved hosts.
@@ -496,6 +496,65 @@ class ARPTestCase(VppTestCase):
                              self.pg1._remote_hosts[9].ip4)
 
         #
+        # Add a hierachy of routes for a host in the sub-net.
+        # Should still get an ARP resp since the cover is attached
+        #
+        p = (Ether(dst="ff:ff:ff:ff:ff:ff", src=self.pg1.remote_mac) /
+             ARP(op="who-has",
+                 hwsrc=self.pg1.remote_mac,
+                 pdst=self.pg1.local_ip4,
+                 psrc=self.pg1.remote_hosts[10].ip4))
+
+        r1 = VppIpRoute(self, self.pg1.remote_hosts[10].ip4, 30,
+                        [VppRoutePath(self.pg1.remote_hosts[10].ip4,
+                                      self.pg1.sw_if_index)])
+        r1.add_vpp_config()
+
+        self.pg1.add_stream(p)
+        self.pg_enable_capture(self.pg_interfaces)
+        self.pg_start()
+        rx = self.pg1.get_capture(1)
+        self.verify_arp_resp(rx[0],
+                             self.pg1.local_mac,
+                             self.pg1.remote_mac,
+                             self.pg1.local_ip4,
+                             self.pg1.remote_hosts[10].ip4)
+
+        r2 = VppIpRoute(self, self.pg1.remote_hosts[10].ip4, 32,
+                        [VppRoutePath(self.pg1.remote_hosts[10].ip4,
+                                      self.pg1.sw_if_index)])
+        r2.add_vpp_config()
+
+        self.pg1.add_stream(p)
+        self.pg_enable_capture(self.pg_interfaces)
+        self.pg_start()
+        rx = self.pg1.get_capture(1)
+        self.verify_arp_resp(rx[0],
+                             self.pg1.local_mac,
+                             self.pg1.remote_mac,
+                             self.pg1.local_ip4,
+                             self.pg1.remote_hosts[10].ip4)
+
+        #
+        # add an ARP entry that's not on the sub-net and so whose
+        # adj-fib fails the refinement check. then send an ARP request
+        # from that source
+        #
+        a1 = VppNeighbor(self,
+                         self.pg0.sw_if_index,
+                         self.pg0.remote_mac,
+                         "100.100.100.50")
+        a1.add_vpp_config()
+
+        p = (Ether(dst="ff:ff:ff:ff:ff:ff", src=self.pg0.remote_mac) /
+             ARP(op="who-has",
+                 hwsrc=self.pg0.remote_mac,
+                 psrc="100.100.100.50",
+                 pdst=self.pg0.remote_ip4))
+        self.send_and_assert_no_replies(self.pg0, p,
+                                        "ARP req for from failed adj-fib")
+
+        #
         # ERROR Cases
         #  1 - don't respond to ARP request for address not within the
         #      interface's sub-net
@@ -536,7 +595,8 @@ class ARPTestCase(VppTestCase):
         #
         #  2 - don't respond to ARP request from an address not within the
         #      interface's sub-net
-        #
+        #   2b - to a prxied address
+        #   2c - not within a differents interface's sub-net
         p = (Ether(dst="ff:ff:ff:ff:ff:ff", src=self.pg0.remote_mac) /
              ARP(op="who-has",
                  hwsrc=self.pg0.remote_mac,
@@ -552,6 +612,13 @@ class ARPTestCase(VppTestCase):
         self.send_and_assert_no_replies(
             self.pg0, p,
             "ARP req for non-local source - unnum")
+        p = (Ether(dst="ff:ff:ff:ff:ff:ff", src=self.pg0.remote_mac) /
+             ARP(op="who-has",
+                 hwsrc=self.pg0.remote_mac,
+                 psrc=self.pg1.remote_ip4,
+                 pdst=self.pg0.local_ip4))
+        self.send_and_assert_no_replies(self.pg0, p,
+                                        "ARP req for non-local source 2c")
 
         #
         #  3 - don't respond to ARP request from an address that belongs to
@@ -588,6 +655,7 @@ class ARPTestCase(VppTestCase):
         # need this to flush the adj-fibs
         self.pg2.unset_unnumbered(self.pg1.sw_if_index)
         self.pg2.admin_down()
+        self.pg1.admin_down()
 
     def test_proxy_arp(self):
         """ Proxy ARP """