bonding lacp: deleting virtual interface which was enslaved may cause crash

Virtual interfaces may be part of the bonding like physical interfaces. The
difference is virtual interfaces may disappear dynamically. As an example,
the following CLI sequence may crash the debug image

create vhost-user socket /tmp/sock1
create bond mode lacp
bond add BondEthernet0 VirtualEthernet0/0/0
delete vhost-user VirtualEhernet0/0/0

Notice the virtual interface is deleted without first doing bond delete.
The proper order is to first remove the slave interface from the bond prior
to deleting the virtual interface as shown below. But we should handle it
anyway.

create vhost-user socket /tmp/sock1
create bond mode lacp
bond add BondEthernet0 VirtualEthernet0/0/0
bond del VirtualEthernet0/0/0   <-----
delete vhost-user VirtualEhernet0/0/0

The fix is to register for VNET_SW_INTERFACE_ADD_DEL_FUNCTION and remove
the slave interface from the bond if the to-be-deleted interface is part of
the bond. We check the interface that it is actually up before we send
the lacp pdu. Up means both hw and sw admin up.

Type: fix

Signed-off-by: Steven Luong <sluong@cisco.com>
Change-Id: If4d2da074338b16aab0df54e00d719e55c45221a

diff --git a/src/plugins/lacp/cli.c b/src/plugins/lacp/cli.c
index b8ff199..92a890d 100644 (file)
--- a/src/plugins/lacp/cli.c
+++ b/src/plugins/lacp/cli.c
@@ -30,7 +30,7 @@ lacp_dump_ifs (lacp_interface_details_t ** out_lacpifs)
 
   /* *INDENT-OFF* */
   pool_foreach (sif, bm->neighbors,
-    if ((sif->port_enabled == 0) || (sif->lacp_enabled == 0))
+    if (sif->lacp_enabled == 0)
       continue;
     vec_add2(r_lacpifs, lacpif, 1);
     clib_memset (lacpif, 0, sizeof (*lacpif));
@@ -88,7 +88,7 @@ show_lacp (vlib_main_t * vm, u32 * sw_if_indices)
   for (i = 0; i < vec_len (sw_if_indices); i++)
     {
       sif = bond_get_slave_by_sw_if_index (sw_if_indices[i]);
-      if (!sif || (sif->port_enabled == 0) || (sif->lacp_enabled == 0))
+      if (!sif || (sif->lacp_enabled == 0))
        continue;
       bif = bond_get_master_by_dev_instance (sif->bif_dev_instance);
       vlib_cli_output (vm,
@@ -157,7 +157,7 @@ show_lacp_details (vlib_main_t * vm, u32 * sw_if_indices)
   for (i = 0; i < vec_len (sw_if_indices); i++)
     {
       sif = bond_get_slave_by_sw_if_index (sw_if_indices[i]);
-      if (!sif || (sif->port_enabled == 0) || (sif->lacp_enabled == 0))
+      if (!sif || (sif->lacp_enabled == 0))
        continue;
       vlib_cli_output (vm, "  %U", format_vnet_sw_if_index_name,
                       vnet_get_main (), sif->sw_if_index);
@@ -186,6 +186,7 @@ show_lacp_details (vlib_main_t * vm, u32 * sw_if_indices)
                         now - sif->last_marker_pdu_sent_time);
       vlib_cli_output (vm, "    debug: %d", sif->debug);
       vlib_cli_output (vm, "    loopback port: %d", sif->loopback_port);
+      vlib_cli_output (vm, "    port_enabled: %d", sif->port_enabled);
       vlib_cli_output (vm, "    port moved: %d", sif->port_moved);
       vlib_cli_output (vm, "    ready_n: %d", sif->ready_n);
       vlib_cli_output (vm, "    ready: %d", sif->ready);

diff --git a/src/plugins/lacp/lacp.c b/src/plugins/lacp/lacp.c
index 57d8bb0..dba6cb1 100644 (file)
--- a/src/plugins/lacp/lacp.c
+++ b/src/plugins/lacp/lacp.c
@@ -107,7 +107,7 @@ lacp_pick_packet_template (slave_if_t * sif)
 void
 lacp_send_lacp_pdu (vlib_main_t * vm, slave_if_t * sif)
 {
-  if (sif->mode != BOND_MODE_LACP)
+  if ((sif->mode != BOND_MODE_LACP) || (sif->port_enabled == 0))
     {
       lacp_stop_timer (&sif->periodic_timer);
       return;
@@ -374,16 +374,18 @@ lacp_sw_interface_up_down (vnet_main_t * vnm, u32 sw_if_index, u32 flags)
   sif = bond_get_slave_by_sw_if_index (sw_if_index);
   if (sif)
     {
-      sif->port_enabled = flags & VNET_SW_INTERFACE_FLAG_ADMIN_UP;
+      if (sif->lacp_enabled == 0)
+       return 0;
+
+      /* port_enabled is both admin up and hw link up */
+      sif->port_enabled = ((flags & VNET_SW_INTERFACE_FLAG_ADMIN_UP) &&
+                          vnet_sw_interface_is_link_up (vnm, sw_if_index));
       if (sif->port_enabled == 0)
        {
-         if (sif->lacp_enabled)
-           {
-             lacp_init_neighbor (sif, sif->actor_admin.system,
-                                 ntohs (sif->actor_admin.port_number),
-                                 ntohs (sif->actor_admin.key));
-             lacp_init_state_machines (vm, sif);
-           }
+         lacp_init_neighbor (sif, sif->actor_admin.system,
+                             ntohs (sif->actor_admin.port_number),
+                             ntohs (sif->actor_admin.key));
+         lacp_init_state_machines (vm, sif);
        }
     }
 
@@ -404,15 +406,19 @@ lacp_hw_interface_up_down (vnet_main_t * vnm, u32 hw_if_index, u32 flags)
   sif = bond_get_slave_by_sw_if_index (sw->sw_if_index);
   if (sif)
     {
-      if (!(flags & VNET_HW_INTERFACE_FLAG_LINK_UP))
+      if (sif->lacp_enabled == 0)
+       return 0;
+
+      /* port_enabled is both admin up and hw link up */
+      sif->port_enabled = ((flags & VNET_HW_INTERFACE_FLAG_LINK_UP) &&
+                          vnet_sw_interface_is_admin_up (vnm,
+                                                         sw->sw_if_index));
+      if (sif->port_enabled == 0)
        {
-         if (sif->lacp_enabled)
-           {
-             lacp_init_neighbor (sif, sif->actor_admin.system,
-                                 ntohs (sif->actor_admin.port_number),
-                                 ntohs (sif->actor_admin.key));
-             lacp_init_state_machines (vm, sif);
-           }
+         lacp_init_neighbor (sif, sif->actor_admin.system,
+                             ntohs (sif->actor_admin.port_number),
+                             ntohs (sif->actor_admin.key));
+         lacp_init_state_machines (vm, sif);
        }
     }
 

diff --git a/src/vnet/bonding/cli.c b/src/vnet/bonding/cli.c
index 9d3b942..f45a31c 100644 (file)
--- a/src/vnet/bonding/cli.c
+++ b/src/vnet/bonding/cli.c
@@ -576,7 +576,8 @@ bond_enslave (vlib_main_t * vm, bond_enslave_args_t * args)
   pool_get (bm->neighbors, sif);
   clib_memset (sif, 0, sizeof (*sif));
   sw = pool_elt_at_index (im->sw_interfaces, args->slave);
-  sif->port_enabled = sw->flags & VNET_SW_INTERFACE_FLAG_ADMIN_UP;
+  /* port_enabled is both admin up and hw link up */
+  sif->port_enabled = vnet_sw_interface_is_up (vnm, sw->sw_if_index);
   sif->sw_if_index = sw->sw_if_index;
   sif->hw_if_index = sw->hw_if_index;
   sif->packet_template_index = (u8) ~ 0;
@@ -642,8 +643,7 @@ bond_enslave (vlib_main_t * vm, bond_enslave_args_t * args)
       if (bm->lacp_enable_disable)
        (*bm->lacp_enable_disable) (vm, bif, sif, 1);
     }
-  else if (sif->port_enabled &&
-          (sif_hw->flags & VNET_HW_INTERFACE_FLAG_LINK_UP))
+  else if (sif->port_enabled)
     {
       bond_enable_collecting_distributing (vm, sif);
     }

diff --git a/src/vnet/bonding/device.c b/src/vnet/bonding/device.c
index e977195..77a53b6 100644 (file)
--- a/src/vnet/bonding/device.c
+++ b/src/vnet/bonding/device.c
@@ -777,9 +777,10 @@ bond_process (vlib_main_t * vm, vlib_node_runtime_t * rt, vlib_frame_t * f)
       for (i = 0; i < vec_len (event_data); i++)
        {
          hw_if_index = event_data[i];
-         /* walk hw interface to process all subinterfaces */
-         vnet_hw_interface_walk_sw (vnm, hw_if_index,
-                                    bond_active_interface_switch_cb, 0);
+         if (vnet_get_hw_interface_or_null (vnm, hw_if_index))
+           /* walk hw interface to process all subinterfaces */
+           vnet_hw_interface_walk_sw (vnm, hw_if_index,
+                                      bond_active_interface_switch_cb, 0);
        }
       vec_reset_length (event_data);
     }
@@ -808,6 +809,25 @@ VNET_DEVICE_CLASS (bond_dev_class) = {
 
 /* *INDENT-ON* */
 
+static clib_error_t *
+bond_slave_interface_add_del (vnet_main_t * vnm, u32 sw_if_index, u32 is_add)
+{
+  bond_main_t *bm = &bond_main;
+  slave_if_t *sif;
+  bond_detach_slave_args_t args = { 0 };
+
+  if (is_add)
+    return 0;
+  sif = bond_get_slave_by_sw_if_index (sw_if_index);
+  if (!sif)
+    return 0;
+  args.slave = sw_if_index;
+  bond_detach_slave (bm->vlib_main, &args);
+  return args.error;
+}
+
+VNET_SW_INTERFACE_ADD_DEL_FUNCTION (bond_slave_interface_add_del);
+
 /*
  * fd.io coding-style-patch-verification: ON
  *

diff --git a/src/vnet/bonding/node.c b/src/vnet/bonding/node.c
index 6fc7471..ce5aefa 100644 (file)
--- a/src/vnet/bonding/node.c
+++ b/src/vnet/bonding/node.c
@@ -394,23 +394,16 @@ bond_sw_interface_up_down (vnet_main_t * vnm, u32 sw_if_index, u32 flags)
   sif = bond_get_slave_by_sw_if_index (sw_if_index);
   if (sif)
     {
-      sif->port_enabled = flags & VNET_SW_INTERFACE_FLAG_ADMIN_UP;
       if (sif->lacp_enabled)
        return 0;
 
+      /* port_enabled is both admin up and hw link up */
+      sif->port_enabled = ((flags & VNET_SW_INTERFACE_FLAG_ADMIN_UP) &&
+                          vnet_sw_interface_is_link_up (vnm, sw_if_index));
       if (sif->port_enabled == 0)
-       {
-         bond_disable_collecting_distributing (vm, sif);
-       }
+       bond_disable_collecting_distributing (vm, sif);
       else
-       {
-         vnet_main_t *vnm = vnet_get_main ();
-         vnet_hw_interface_t *hw =
-           vnet_get_sup_hw_interface (vnm, sw_if_index);
-
-         if (hw->flags & VNET_HW_INTERFACE_FLAG_LINK_UP)
-           bond_enable_collecting_distributing (vm, sif);
-       }
+       bond_enable_collecting_distributing (vm, sif);
     }
 
   return 0;
@@ -433,14 +426,14 @@ bond_hw_interface_up_down (vnet_main_t * vnm, u32 hw_if_index, u32 flags)
       if (sif->lacp_enabled)
        return 0;
 
-      if (!(flags & VNET_HW_INTERFACE_FLAG_LINK_UP))
-       {
-         bond_disable_collecting_distributing (vm, sif);
-       }
-      else if (sif->port_enabled)
-       {
-         bond_enable_collecting_distributing (vm, sif);
-       }
+      /* port_enabled is both admin up and hw link up */
+      sif->port_enabled = ((flags & VNET_HW_INTERFACE_FLAG_LINK_UP) &&
+                          vnet_sw_interface_is_admin_up (vnm,
+                                                         sw->sw_if_index));
+      if (sif->port_enabled == 0)
+       bond_disable_collecting_distributing (vm, sif);
+      else
+       bond_enable_collecting_distributing (vm, sif);
     }
 
   return 0;