vrrp: improve RFC compliance for ARP/ND

Type: fix

The ARP/ND feature nodes reply to requests for a VR virtual IP address
when a VR is in the master state. If the VR is in the backup state, the
request is passed to the next node on the feature arc.

This can cause an incorrect response to be sent. If some other feature
(e.g. NAT) causes a virtual IP address to be configured as a "local"
address on the system, a later node on the feature arc may respond to
an ARP/ND request with the real MAC address of the interface.

RFC 5798 says that a router must respond to ARP/ND requests for VR
virtual IP addresses with the VR virtual MAC address. And it says a
router must not respond to ARP/ND requests for VR virtual IP addresses
when the VR is in the backup state. Ensure that ARP/ND requests for
VR virtual IP addresses are dropped when in the backup state rather
than allowing them to continue on the feature arc where another node
may end up responding.

In order to do this, enable/disable the feature nodes when leaving
or entering the init state instead of the master state.

Change-Id: I416f83e125cbf91deb90c3b6eb00ba3207de24ad
Signed-off-by: Matthew Smith <mgsmith@netgate.com>

diff --git a/src/plugins/vrrp/node.c b/src/plugins/vrrp/node.c
index a916bcc..0cd5b59 100644 (file)
--- a/src/plugins/vrrp/node.c
+++ b/src/plugins/vrrp/node.c
@@ -334,11 +334,18 @@ vrrp_arp_nd_next (vlib_buffer_t * b, u32 * next_index, u32 * vr_index,
   if (*vr_index == ~0)
     return;
 
-  /* only reply if the VR is in the master state */
   vr = vrrp_vr_lookup_index (*vr_index);
   if (!vr || vr->runtime.state != VRRP_VR_STATE_MASTER)
-    return;
+    {
+      /* RFC 5798 - section 6.4.2 - Backup "MUST NOT respond" to ARP/ND.
+       * So we must drop the request rather than allowing it to continue
+       * on the feature arc.
+       */
+      *next_index = VRRP_ARP_INPUT_NEXT_DROP;
+      return;
+    }
 
+  /* RFC 5798 section 6.4.3: Master "MUST respond" to ARP/ND. */
   eth = ethernet_buffer_get_header (b);
   rewrite = ethernet_build_rewrite (vnm, sw_if_index, link_type,
                                    eth->src_address);

diff --git a/src/plugins/vrrp/vrrp.c b/src/plugins/vrrp/vrrp.c
index 564cdb0..eb988d3 100644 (file)
--- a/src/plugins/vrrp/vrrp.c
+++ b/src/plugins/vrrp/vrrp.c
@@ -144,11 +144,7 @@ vrrp_vr_transition_intf (vrrp_vr_t * vr, vrrp_vr_state_t new_state)
   u8 is_ipv6 = vrrp_vr_is_ipv6 (vr);
   u32 *vr_index;
   int n_master_accept = 0;
-
-  /* only need to do something if entering or leaving master state */
-  if ((vr->runtime.state != VRRP_VR_STATE_MASTER) &&
-      (new_state != VRRP_VR_STATE_MASTER))
-    return;
+  int n_started = 0;
 
   if (is_ipv6)
     {
@@ -167,46 +163,53 @@ vrrp_vr_transition_intf (vrrp_vr_t * vr, vrrp_vr_state_t new_state)
 
   intf = vrrp_intf_get (vr->config.sw_if_index);
 
-  if (new_state == VRRP_VR_STATE_MASTER)
-    {
-      intf->n_master_vrs[is_ipv6]++;
-      if (intf->n_master_vrs[is_ipv6] == 1)
-       vnet_feature_enable_disable (arc_name, node_name,
-                                    vr->config.sw_if_index, 1, NULL, 0);
-    }
-  else
-    {
-      if (intf->n_master_vrs[is_ipv6] == 1)
-       vnet_feature_enable_disable (arc_name, node_name,
-                                    vr->config.sw_if_index, 0, NULL, 0);
-      /* If the count were already 0, leave it at 0 */
-      if (intf->n_master_vrs[is_ipv6])
-       intf->n_master_vrs[is_ipv6]--;
-    }
+  /* Check other VRs on this intf to see if features need to be toggled */
+  vec_foreach (vr_index, intf->vr_indices[is_ipv6])
+  {
+    vrrp_vr_t *intf_vr = vrrp_vr_lookup_index (*vr_index);
 
-  /* accept mode enabled, count the other master VRs w/ accept mode on intf */
-  if (vrrp_vr_accept_mode_enabled (vr))
-    {
-      vec_foreach (vr_index, intf->vr_indices[is_ipv6])
-      {
-       vrrp_vr_t *intf_vr = vrrp_vr_lookup_index (*vr_index);
+    if (intf_vr == vr)
+      continue;
 
-       if (intf_vr == vr)
-         continue;
+    if (intf_vr->runtime.state == VRRP_VR_STATE_INIT)
+      continue;
 
-       if (vrrp_vr_accept_mode_enabled (intf_vr) &&
-           (intf_vr->runtime.state == VRRP_VR_STATE_MASTER))
-         n_master_accept++;
-      }
+    n_started++;
+
+    if ((intf_vr->runtime.state == VRRP_VR_STATE_MASTER) &&
+       vrrp_vr_accept_mode_enabled (intf_vr))
+      n_master_accept++;
+  }
 
-      /* If no others, enable or disable the feature based on new state */
-      if (!n_master_accept)
+  /* If entering/leaving init state, start/stop ARP or ND feature if no other
+   * VRs are active on the interface.
+   */
+  if (((vr->runtime.state == VRRP_VR_STATE_INIT) ||
+       (new_state == VRRP_VR_STATE_INIT)) && (n_started == 0))
+    vnet_feature_enable_disable (arc_name, node_name,
+                                vr->config.sw_if_index,
+                                (new_state != VRRP_VR_STATE_INIT), NULL, 0);
+
+  /* Special housekeeping when entering/leaving master mode */
+  if ((vr->runtime.state == VRRP_VR_STATE_MASTER) ||
+      (new_state == VRRP_VR_STATE_MASTER))
+    {
+      /* Maintain count of master state VRs on interface */
+      if (new_state == VRRP_VR_STATE_MASTER)
+       intf->n_master_vrs[is_ipv6]++;
+      else if (intf->n_master_vrs[is_ipv6] > 0)
+       intf->n_master_vrs[is_ipv6]--;
+
+      /* If accept mode is enabled and no other master on intf has accept
+       * mode enabled, enable/disable feature node to avoid spurious drops by
+       * spoofing check.
+       */
+      if (vrrp_vr_accept_mode_enabled (vr) && !n_master_accept)
        vnet_feature_enable_disable (mc_arc_name, mc_node_name,
                                     vr->config.sw_if_index,
                                     (new_state == VRRP_VR_STATE_MASTER),
                                     NULL, 0);
     }
-
 }
 
 /* If accept mode enabled, add/remove VR addresses from interface */