ARP/API:protect against identical registrations

Change-Id: Ia3acf87d3e07a7d41c047869de504e1972334b55
Signed-off-by: Eyal Bari <ebari@cisco.com>

3 files changed, 49 insertions, 62 deletions

diff --git a/src/vnet/api_errno.h b/src/vnet/api_errno.h
index 74d39bdbe68..f3ffd2a6b3e 100644
--- a/src/vnet/api_errno.h
+++ b/src/vnet/api_errno.h
@@ -104,7 +104,8 @@ _(BFD_EAGAIN, -111, "BFD object cannot be manipulated at this time")	\
 _(INVALID_GPE_MODE, -112, "Invalid GPE mode")                           \
 _(LISP_GPE_ENTRIES_PRESENT, -113, "LISP GPE entries are present")       \
 _(ADDRESS_FOUND_FOR_INTERFACE, -114, "Address found for interface")	\
-_(SESSION_CONNECT_FAIL, -115, "Session failed to connect")
+_(SESSION_CONNECT_FAIL, -115, "Session failed to connect")              \
+_(ENTRY_ALREADY_EXISTS, -116, "Entry already exists")
 
 typedef enum
 {
diff --git a/src/vnet/ethernet/arp.c b/src/vnet/ethernet/arp.c
index 711b7867fab..09ecd0ccebc 100644
--- a/src/vnet/ethernet/arp.c
+++ b/src/vnet/ethernet/arp.c
@@ -702,76 +702,59 @@ vnet_add_del_ip4_arp_change_event (vnet_main_t * vnm,
 {
   ethernet_arp_main_t *am = &ethernet_arp_main;
   ip4_address_t *address = address_arg;
-  uword *p;
+
+  /* Try to find an existing entry */
+  u32 *first = (u32 *) hash_get (am->mac_changes_by_address, address->as_u32);
+  u32 *p = first;
   pending_resolution_t *mc;
-  void (*fp) (u32, u8 *) = data_callback;
+  while (p && *p != ~0)
+    {
+      mc = pool_elt_at_index (am->mac_changes, *p);
+      if (mc->node_index == node_index && mc->type_opaque == type_opaque
+	  && mc->pid == pid)
+	break;
+      p = &mc->next_index;
+    }
 
+  int found = p && *p != ~0;
   if (is_add)
     {
-      pool_get (am->mac_changes, mc);
+      if (found)
+	return VNET_API_ERROR_ENTRY_ALREADY_EXISTS;
 
-      mc->next_index = ~0;
-      mc->node_index = node_index;
-      mc->type_opaque = type_opaque;
-      mc->data = data;
-      mc->data_callback = data_callback;
-      mc->pid = pid;
+      pool_get (am->mac_changes, mc);
+      *mc = (pending_resolution_t)
+      {
+      .next_index = ~0,.node_index = node_index,.type_opaque =
+	  type_opaque,.data = data,.data_callback = data_callback,.pid =
+	  pid,};
 
-      p = hash_get (am->mac_changes_by_address, address->as_u32);
+      /* Insert new resolution at the end of the list */
+      u32 new_idx = mc - am->mac_changes;
       if (p)
-	{
-	  /* Insert new resolution at the head of the list */
-	  mc->next_index = p[0];
-	  hash_unset (am->mac_changes_by_address, address->as_u32);
-	}
-
-      hash_set (am->mac_changes_by_address, address->as_u32,
-		mc - am->mac_changes);
-      return 0;
+	p[0] = new_idx;
+      else
+	hash_set (am->mac_changes_by_address, address->as_u32, new_idx);
     }
   else
     {
-      u32 index;
-      pending_resolution_t *mc_last = 0;
-
-      p = hash_get (am->mac_changes_by_address, address->as_u32);
-      if (p == 0)
+      if (!found)
 	return VNET_API_ERROR_NO_SUCH_ENTRY;
 
-      index = p[0];
+      /* Clients may need to clean up pool entries, too */
+      void (*fp) (u32, u8 *) = data_callback;
+      if (fp)
+	(*fp) (mc->data, 0 /* no new mac addrs */ );
 
-      while (index != (u32) ~ 0)
-	{
-	  mc = pool_elt_at_index (am->mac_changes, index);
-	  if (mc->node_index == node_index &&
-	      mc->type_opaque == type_opaque && mc->pid == pid)
-	    {
-	      /* Clients may need to clean up pool entries, too */
-	      if (fp)
-		(*fp) (mc->data, 0 /* no new mac addrs */ );
-	      if (index == p[0])
-		{
-		  hash_unset (am->mac_changes_by_address, address->as_u32);
-		  if (mc->next_index != ~0)
-		    hash_set (am->mac_changes_by_address, address->as_u32,
-			      mc->next_index);
-		  pool_put (am->mac_changes, mc);
-		  return 0;
-		}
-	      else
-		{
-		  ASSERT (mc_last);
-		  mc_last->next_index = mc->next_index;
-		  pool_put (am->mac_changes, mc);
-		  return 0;
-		}
-	    }
-	  mc_last = mc;
-	  index = mc->next_index;
-	}
+      /* Remove the entry from the list and delete the entry */
+      *p = mc->next_index;
+      pool_put (am->mac_changes, mc);
 
-      return VNET_API_ERROR_NO_SUCH_ENTRY;
+      /* Remove from hash if we deleted the last entry */
+      if (*p == ~0 && p == first)
+	hash_unset (am->mac_changes_by_address, address->as_u32);
     }
+  return 0;
 }
 
 /* Either we drop the packet or we send a reply to the sender. */
diff --git a/src/vpp/api/api.c b/src/vpp/api/api.c
index f169d7fcfda..14ccd8648b4 100644
--- a/src/vpp/api/api.c
+++ b/src/vpp/api/api.c
@@ -1574,6 +1574,14 @@ vl_api_want_ip4_arp_events_t_handler (vl_api_want_ip4_arp_events_t * mp)
 
   if (mp->enable_disable)
     {
+      rv = vnet_add_del_ip4_arp_change_event
+	(vnm, arp_change_data_callback,
+	 mp->pid, &mp->address /* addr, in net byte order */ ,
+	 vpe_resolver_process_node.index,
+	 IP4_ARP_EVENT, event - am->arp_events, 1 /* is_add */ );
+
+      if (rv)
+	goto out;
       pool_get (am->arp_events, event);
       memset (event, 0, sizeof (*event));
 
@@ -1584,12 +1592,6 @@ vl_api_want_ip4_arp_events_t_handler (vl_api_want_ip4_arp_events_t * mp)
       event->pid = mp->pid;
       if (mp->address == 0)
 	event->mac_ip = 1;
-
-      rv = vnet_add_del_ip4_arp_change_event
-	(vnm, arp_change_data_callback,
-	 mp->pid, &mp->address /* addr, in net byte order */ ,
-	 vpe_resolver_process_node.index,
-	 IP4_ARP_EVENT, event - am->arp_events, 1 /* is_add */ );
     }
   else
     {
@@ -1599,6 +1601,7 @@ vl_api_want_ip4_arp_events_t_handler (vl_api_want_ip4_arp_events_t * mp)
 	 vpe_resolver_process_node.index,
 	 IP4_ARP_EVENT, ~0 /* pool index */ , 0 /* is_add */ );
     }
+out:
   REPLY_MACRO (VL_API_WANT_IP4_ARP_EVENTS_REPLY);
 }