ARP learning fixes (VPP-843)

learn ARP peers if, 1) it's a reply to a local address, 2) we are sending a response to a request.
send proxy ARP responses only in the interface the request was sent.

Change-Id: I22b949c65122824233076492b7dd537daca07bc2
Signed-off-by: Neale Ranns <nranns@cisco.com>
(cherry picked from commit d5b6aa139856a1447f7bc5377058202110eaa4cf)

4 files changed, 98 insertions, 90 deletions

diff --git a/src/vnet/ethernet/arp.c b/src/vnet/ethernet/arp.c
index 2367014e144..5ad0c3e3c2d 100644
--- a/src/vnet/ethernet/arp.c
+++ b/src/vnet/ethernet/arp.c
@@ -562,6 +562,7 @@ vnet_arp_set_ip4_over_ethernet_internal (vnet_main_t * vnm,
 
       e->sw_if_index = sw_if_index;
       e->ip4_address = a->ip4;
+      e->fib_entry_index = FIB_NODE_INDEX_INVALID;
       clib_memcpy (e->ethernet_address,
 		   a->ethernet, sizeof (e->ethernet_address));
 
@@ -839,66 +840,39 @@ unset_random_arp_entry (void)
 
 static int
 arp_unnumbered (vlib_buffer_t * p0,
-		u32 pi0, ethernet_header_t * eth0, u32 sw_if_index)
+		u32 pi0,
+		ethernet_header_t * eth0,
+		u32 input_sw_if_index, u32 conn_sw_if_index)
 {
   vlib_main_t *vm = vlib_get_main ();
   vnet_main_t *vnm = vnet_get_main ();
   vnet_interface_main_t *vim = &vnm->interface_main;
   vnet_sw_interface_t *si;
   vnet_hw_interface_t *hi;
-  u32 unnum_src_sw_if_index;
-  u32 *broadcast_swifs = 0;
   u32 *buffers = 0;
-  u32 n_alloc = 0;
   vlib_buffer_t *b0;
   int i;
   u8 dst_mac_address[6];
   i16 header_size;
   ethernet_arp_header_t *arp0;
 
-  /* Save the dst mac address */
-  clib_memcpy (dst_mac_address, eth0->dst_address, sizeof (dst_mac_address));
+  /* verify that the input interface is unnumbered to the connected.
+   * the connected interface is the interface on which the subnet is
+   * configured */
+  si = &vim->sw_interfaces[input_sw_if_index];
 
-  /* Figure out which sw_if_index supplied the address */
-  unnum_src_sw_if_index = sw_if_index;
-
-  /* Track down all users of the unnumbered source */
-  /* *INDENT-OFF* */
-  pool_foreach (si, vim->sw_interfaces,
-  ({
-    if (si->flags & VNET_SW_INTERFACE_FLAG_UNNUMBERED &&
-	(si->unnumbered_sw_if_index == unnum_src_sw_if_index))
-      {
-	vec_add1 (broadcast_swifs, si->sw_if_index);
-      }
-  }));
-  /* *INDENT-ON* */
-
-  /* If there are no interfaces un-unmbered to this interface,
-     we are done  here. */
-  if (0 == vec_len (broadcast_swifs))
-    return 0;
-
-  /* Allocate buffering if we need it */
-  if (vec_len (broadcast_swifs) > 1)
+  if (!(si->flags & VNET_SW_INTERFACE_FLAG_UNNUMBERED &&
+	(si->unnumbered_sw_if_index == conn_sw_if_index)))
     {
-      vec_validate (buffers, vec_len (broadcast_swifs) - 2);
-      n_alloc = vlib_buffer_alloc (vm, buffers, vec_len (buffers));
-      _vec_len (buffers) = n_alloc;
-      for (i = 0; i < n_alloc; i++)
-	{
-	  b0 = vlib_get_buffer (vm, buffers[i]);
-
-	  /* xerox (partially built) ARP pkt */
-	  clib_memcpy (b0->data, p0->data,
-		       p0->current_length + p0->current_data);
-	  b0->current_data = p0->current_data;
-	  b0->current_length = p0->current_length;
-	  vnet_buffer (b0)->sw_if_index[VLIB_RX] =
-	    vnet_buffer (p0)->sw_if_index[VLIB_RX];
-	}
+      /* the input interface is not unnumbered to the interface on which
+       * the sub-net is configured that covers the ARP request.
+       * So this is not the case for unnumbered.. */
+      return 0;
     }
 
+  /* Save the dst mac address */
+  clib_memcpy (dst_mac_address, eth0->dst_address, sizeof (dst_mac_address));
+
   vec_insert (buffers, 1, 0);
   buffers[0] = pi0;
 
@@ -907,8 +881,8 @@ arp_unnumbered (vlib_buffer_t * p0,
       b0 = vlib_get_buffer (vm, buffers[i]);
       arp0 = vlib_buffer_get_current (b0);
 
-      hi = vnet_get_sup_hw_interface (vnm, broadcast_swifs[i]);
-      si = vnet_get_sw_interface (vnm, broadcast_swifs[i]);
+      hi = vnet_get_sup_hw_interface (vnm, input_sw_if_index);
+      si = vnet_get_sw_interface (vnm, input_sw_if_index);
 
       /* For decoration, most likely */
       vnet_buffer (b0)->sw_if_index[VLIB_TX] = hi->sw_if_index;
@@ -978,14 +952,25 @@ arp_unnumbered (vlib_buffer_t * p0,
     }
 
   /* The regular path outputs the original pkt.. */
-  vnet_buffer (p0)->sw_if_index[VLIB_TX] = broadcast_swifs[0];
+  vnet_buffer (p0)->sw_if_index[VLIB_TX] = input_sw_if_index;
 
-  vec_free (broadcast_swifs);
   vec_free (buffers);
 
   return !0;
 }
 
+static u32
+arp_learn (vnet_main_t * vnm,
+	   ethernet_arp_main_t * am, u32 sw_if_index, void *addr)
+{
+  if (am->limit_arp_cache_size &&
+      pool_elts (am->ip4_entry_pool) >= am->limit_arp_cache_size)
+    unset_random_arp_entry ();
+
+  vnet_arp_set_ip4_over_ethernet (vnm, sw_if_index, addr, 0, 0);
+  return (ETHERNET_ARP_ERROR_l3_src_address_learned);
+}
+
 static uword
 arp_input (vlib_main_t * vm, vlib_node_runtime_t * node, vlib_frame_t * frame)
 {
@@ -1133,28 +1118,14 @@ arp_input (vlib_main_t * vm, vlib_node_runtime_t * node, vlib_frame_t * frame)
 	      goto drop2;
 	    }
 
-	  /* Learn or update sender's mapping only for requests or unicasts
-	     that don't match local interface address. */
-	  if (ethernet_address_cast (eth0->dst_address) ==
-	      ETHERNET_ADDRESS_UNICAST || is_request0)
-	    {
-	      if (am->limit_arp_cache_size &&
-		  pool_elts (am->ip4_entry_pool) >= am->limit_arp_cache_size)
-		unset_random_arp_entry ();
-
-	      vnet_arp_set_ip4_over_ethernet (vnm, sw_if_index0,
-					      &arp0->ip4_over_ethernet[0],
-					      0 /* is_static */ , 0);
-	      error0 = ETHERNET_ARP_ERROR_l3_src_address_learned;
-	    }
-
-	  /* Only send a reply for requests sent which match a local interface. */
-	  if (!(is_request0 && dst_is_local0))
+	  /* Learn or update sender's mapping only for replies to addresses
+	   * that are local to the subnet */
+	  if (arp0->opcode ==
+	      clib_host_to_net_u16 (ETHERNET_ARP_OPCODE_reply) &&
+	      dst_is_local0)
 	    {
-	      error0 =
-		(arp0->opcode ==
-		 clib_host_to_net_u16 (ETHERNET_ARP_OPCODE_reply) ?
-		 ETHERNET_ARP_ERROR_replies_received : error0);
+	      error0 = arp_learn (vnm, am, sw_if_index0,
+				  &arp0->ip4_over_ethernet[0]);
 	      goto drop1;
 	    }
 
@@ -1188,7 +1159,8 @@ arp_input (vlib_main_t * vm, vlib_node_runtime_t * node, vlib_frame_t * frame)
 	    {
 	      if (is_unnum0)
 		{
-		  if (!arp_unnumbered (p0, pi0, eth0, conn_sw_if_index0))
+		  if (!arp_unnumbered (p0, pi0, eth0,
+				       sw_if_index0, conn_sw_if_index0))
 		    goto drop2;
 		}
 	      else
@@ -1242,6 +1214,10 @@ arp_input (vlib_main_t * vm, vlib_node_runtime_t * node, vlib_frame_t * frame)
 		}
 	    }
 
+	  /* We are going to reply to this request, so learn the sender */
+	  error0 = arp_learn (vnm, am, sw_if_index0,
+			      &arp0->ip4_over_ethernet[1]);
+
 	  vlib_validate_buffer_enqueue_x1 (vm, node, next_index, to_next,
 					   n_left_to_next, pi0, next0);
 
@@ -1697,9 +1673,8 @@ arp_entry_free (ethernet_arp_interface_t * eai, ethernet_arp_ip4_entry_t * e)
 {
   ethernet_arp_main_t *am = &ethernet_arp_main;
 
-  /* it's safe to delete the ADJ source on the FIB entry, even if it
-   * was added */
-  fib_table_entry_delete_index (e->fib_entry_index, FIB_SOURCE_ADJ);
+  if (FIB_NODE_INDEX_INVALID != e->fib_entry_index)
+    fib_table_entry_delete_index (e->fib_entry_index, FIB_SOURCE_ADJ);
   hash_unset (eai->arp_entries, e->ip4_address.as_u32);
   pool_put (am->ip4_entry_pool, e);
 }
diff --git a/src/vnet/ip/ip6_neighbor.c b/src/vnet/ip/ip6_neighbor.c
index 92708f14d5b..edc670af3b0 100644
--- a/src/vnet/ip/ip6_neighbor.c
+++ b/src/vnet/ip/ip6_neighbor.c
@@ -273,7 +273,8 @@ ip6_neighbor_sw_interface_up_down (vnet_main_t * vnm,
 	{
 	  n = pool_elt_at_index (nm->neighbor_pool, to_delete[i]);
 	  mhash_unset (&nm->neighbor_index_by_key, &n->key, 0);
-	  fib_table_entry_delete_index (n->fib_entry_index, FIB_SOURCE_ADJ);
+	  if (FIB_NODE_INDEX_INVALID != n->fib_entry_index)
+	    fib_table_entry_delete_index (n->fib_entry_index, FIB_SOURCE_ADJ);
 	  pool_put (nm->neighbor_pool, n);
 	}
 
@@ -609,6 +610,7 @@ vnet_set_ip6_ethernet_neighbor (vlib_main_t * vm,
       mhash_set (&nm->neighbor_index_by_key, &k, n - nm->neighbor_pool,
 		 /* old value */ 0);
       n->key = k;
+      n->fib_entry_index = FIB_NODE_INDEX_INVALID;
 
       clib_memcpy (n->link_layer_address,
 		   link_layer_address, n_bytes_link_layer_address);
@@ -746,7 +748,9 @@ vnet_unset_ip6_ethernet_neighbor (vlib_main_t * vm,
   adj_nbr_walk_nh6 (sw_if_index,
 		    &n->key.ip6_address, ip6_nd_mk_incomplete_walk, NULL);
 
-  fib_table_entry_delete_index (n->fib_entry_index, FIB_SOURCE_ADJ);
+
+  if (FIB_NODE_INDEX_INVALID != n->fib_entry_index)
+    fib_table_entry_delete_index (n->fib_entry_index, FIB_SOURCE_ADJ);
   pool_put (nm->neighbor_pool, n);
 
 out:
diff --git a/src/vnet/ip/lookup.h b/src/vnet/ip/lookup.h
index 48360b5b41f..0fc2a992a0f 100644
--- a/src/vnet/ip/lookup.h
+++ b/src/vnet/ip/lookup.h
@@ -416,7 +416,7 @@ ip_interface_address_get_address (ip_lookup_main_t * lm,
 /* *INDENT-OFF* */
 #define foreach_ip_interface_address(lm,a,sw_if_index,loop,body)        \
 do {                                                                    \
-    vnet_main_t *_vnm = vnet_get_main();                                     \
+    vnet_main_t *_vnm = vnet_get_main();                                \
     u32 _sw_if_index = sw_if_index;                                     \
     vnet_sw_interface_t *_swif;                                         \
     _swif = vnet_get_sw_interface (_vnm, _sw_if_index);                 \
@@ -424,13 +424,20 @@ do {                                                                    \
     /*                                                                  \
      * Loop => honor unnumbered interface addressing.                   \
      */                                                                 \
-    if (loop && _swif->flags & VNET_SW_INTERFACE_FLAG_UNNUMBERED)       \
-      _sw_if_index = _swif->unnumbered_sw_if_index;                     \
-    u32 _ia =                                                           \
-      (vec_len((lm)->if_address_pool_index_by_sw_if_index)              \
-       > (_sw_if_index))                                                \
-        ? vec_elt ((lm)->if_address_pool_index_by_sw_if_index,          \
-                   (_sw_if_index)) : (u32)~0;                           \
+    if (_swif->flags & VNET_SW_INTERFACE_FLAG_UNNUMBERED)               \
+      {                                                                 \
+        if (loop)                                                       \
+          _sw_if_index = _swif->unnumbered_sw_if_index;                 \
+        else                                                            \
+          /* the interface is unnumbered, by the caller does not want   \
+           * unnumbered interfaces considered/honoured */               \
+          break;                                                        \
+      }                                                                 \
+    u32 _ia = ((vec_len((lm)->if_address_pool_index_by_sw_if_index)     \
+                > (_sw_if_index)) ?                                     \
+               vec_elt ((lm)->if_address_pool_index_by_sw_if_index,     \
+                        (_sw_if_index)) :                               \
+               (u32)~0);                                                \
     ip_interface_address_t * _a;                                        \
     while (_ia != ~0)                                                   \
     {                                                                   \
diff --git a/test/test_neighbor.py b/test/test_neighbor.py
index f2b1cfa47ee..48c6a291248 100644
--- a/test/test_neighbor.py
+++ b/test/test_neighbor.py
@@ -112,8 +112,11 @@ class ARPTestCase(VppTestCase):
         intf.add_stream(pkts)
         self.pg_enable_capture(self.pg_interfaces)
         self.pg_start()
+        timeout = 1
         for i in self.pg_interfaces:
+            i.get_capture(0, timeout=timeout)
             i.assert_nothing_captured(remark=remark)
+            timeout = 0.1
 
     def test_arp(self):
         """ ARP """
@@ -438,7 +441,9 @@ class ARPTestCase(VppTestCase):
         # ERROR Cases
         #  1 - don't respond to ARP request for address not within the
         #      interface's sub-net
-        #  1a - nor within the unnumbered subnet
+        #  1b - nor within the unnumbered subnet
+        #  1c - nor within the subnet of a different interface
+        #
         p = (Ether(dst="ff:ff:ff:ff:ff:ff", src=self.pg0.remote_mac) /
              ARP(op="who-has",
                  hwsrc=self.pg0.remote_mac,
@@ -446,6 +451,10 @@ class ARPTestCase(VppTestCase):
                  psrc=self.pg0.remote_ip4))
         self.send_and_assert_no_replies(self.pg0, p,
                                         "ARP req for non-local destination")
+        self.assertFalse(find_nbr(self,
+                                  self.pg0.sw_if_index,
+                                  "10.10.10.3"))
+
         p = (Ether(dst="ff:ff:ff:ff:ff:ff", src=self.pg2.remote_mac) /
              ARP(op="who-has",
                  hwsrc=self.pg2.remote_mac,
@@ -455,6 +464,17 @@ class ARPTestCase(VppTestCase):
             self.pg0, p,
             "ARP req for non-local destination - unnum")
 
+        p = (Ether(dst="ff:ff:ff:ff:ff:ff", src=self.pg0.remote_mac) /
+             ARP(op="who-has",
+                 hwsrc=self.pg0.remote_mac,
+                 pdst=self.pg1.local_ip4,
+                 psrc=self.pg1.remote_ip4))
+        self.send_and_assert_no_replies(self.pg0, p,
+                                        "ARP req diff sub-net")
+        self.assertFalse(find_nbr(self,
+                                  self.pg0.sw_if_index,
+                                  self.pg1.remote_ip4))
+
         #
         #  2 - don't respond to ARP request from an address not within the
         #      interface's sub-net
@@ -514,15 +534,11 @@ class ARPTestCase(VppTestCase):
     def test_proxy_arp(self):
         """ Proxy ARP """
 
+        self.pg1.generate_remote_hosts(2)
+
         #
         # Proxy ARP rewquest packets for each interface
         #
-        arp_req_pg2 = (Ether(src=self.pg2.remote_mac,
-                             dst="ff:ff:ff:ff:ff:ff") /
-                       ARP(op="who-has",
-                           hwsrc=self.pg2.remote_mac,
-                           pdst="10.10.10.3",
-                           psrc=self.pg1.remote_ip4))
         arp_req_pg0 = (Ether(src=self.pg0.remote_mac,
                              dst="ff:ff:ff:ff:ff:ff") /
                        ARP(op="who-has",
@@ -535,6 +551,12 @@ class ARPTestCase(VppTestCase):
                            hwsrc=self.pg1.remote_mac,
                            pdst="10.10.10.3",
                            psrc=self.pg1.remote_ip4))
+        arp_req_pg2 = (Ether(src=self.pg2.remote_mac,
+                             dst="ff:ff:ff:ff:ff:ff") /
+                       ARP(op="who-has",
+                           hwsrc=self.pg2.remote_mac,
+                           pdst="10.10.10.3",
+                           psrc=self.pg1.remote_hosts[1].ip4))
         arp_req_pg3 = (Ether(src=self.pg3.remote_mac,
                              dst="ff:ff:ff:ff:ff:ff") /
                        ARP(op="who-has",
@@ -607,7 +629,7 @@ class ARPTestCase(VppTestCase):
                              self.pg2.local_mac,
                              self.pg2.remote_mac,
                              "10.10.10.3",
-                             self.pg1.remote_ip4)
+                             self.pg1.remote_hosts[1].ip4)
 
         #
         # A request for an address out of the configured range