bonding: ASSERT fails for bif->active_members in TX path

Elements in bif->active_members are used without protection in the TX DP path.
However, elements in bif->active_members may freed by another thread asynchronously
and it may cause the ASSERT to fail when accessing elements.
The fix is to take a snapshot of the active_members and use that in the TX DP path.

Type: fix

Change-Id: I081cc81cc4d072807d6cbbcf5d21feb97f500362
Signed-off-by: Steven Luong <[email protected]>

diff --git a/src/vnet/bonding/device.c b/src/vnet/bonding/device.c
index a0b93fc..81661c1 100644 (file)
--- a/src/vnet/bonding/device.c
+++ b/src/vnet/bonding/device.c
@@ -192,13 +192,13 @@ bond_lb_broadcast (vlib_main_t *vm, bond_if_t *bif, vlib_buffer_t *b0,
 
   for (port = 1; port < n_members; port++)
     {
-      sw_if_index = *vec_elt_at_index (bif->active_members, port);
-      c0 = vlib_buffer_copy (vm, b0);
-      if (PREDICT_TRUE (c0 != 0))
-       {
-         vnet_buffer (c0)->sw_if_index[VLIB_TX] = sw_if_index;
-         bond_tx_add_to_queue (ptd, port, vlib_get_buffer_index (vm, c0));
-       }
+    sw_if_index = *vec_elt_at_index (ptd->active_members, port);
+    c0 = vlib_buffer_copy (vm, b0);
+    if (PREDICT_TRUE (c0 != 0))
+      {
+       vnet_buffer (c0)->sw_if_index[VLIB_TX] = sw_if_index;
+       bond_tx_add_to_queue (ptd, port, vlib_get_buffer_index (vm, c0));
+      }
     }
 
   return 0;
@@ -351,8 +351,8 @@ bond_hash_to_port (u32 * h, u32 n_left, u32 n_members,
 }
 
 static_always_inline void
-bond_update_sw_if_index (bond_per_thread_data_t * ptd, bond_if_t * bif,
-                        u32 * bi, vlib_buffer_t ** b, u32 * data, u32 n_left,
+bond_update_sw_if_index (bond_per_thread_data_t *ptd, bond_if_t *bif, u32 *bi,
+                        vlib_buffer_t **b, u32 *data, u32 n_left,
                         int single_sw_if_index)
 {
   u32 sw_if_index = data[0];
@@ -381,13 +381,13 @@ bond_update_sw_if_index (bond_per_thread_data_t * ptd, bond_if_t * bif,
       else
        {
          vnet_buffer (b[0])->sw_if_index[VLIB_TX] =
-           *vec_elt_at_index (bif->active_members, h[0]);
+           *vec_elt_at_index (ptd->active_members, h[0]);
          vnet_buffer (b[1])->sw_if_index[VLIB_TX] =
-           *vec_elt_at_index (bif->active_members, h[1]);
+           *vec_elt_at_index (ptd->active_members, h[1]);
          vnet_buffer (b[2])->sw_if_index[VLIB_TX] =
-           *vec_elt_at_index (bif->active_members, h[2]);
+           *vec_elt_at_index (ptd->active_members, h[2]);
          vnet_buffer (b[3])->sw_if_index[VLIB_TX] =
-           *vec_elt_at_index (bif->active_members, h[3]);
+           *vec_elt_at_index (ptd->active_members, h[3]);
 
          bond_tx_add_to_queue (ptd, h[0], bi[0]);
          bond_tx_add_to_queue (ptd, h[1], bi[1]);
@@ -410,7 +410,7 @@ bond_update_sw_if_index (bond_per_thread_data_t * ptd, bond_if_t * bif,
       else
        {
          vnet_buffer (b[0])->sw_if_index[VLIB_TX] =
-           *vec_elt_at_index (bif->active_members, h[0]);
+           *vec_elt_at_index (ptd->active_members, h[0]);
          bond_tx_add_to_queue (ptd, h[0], bi[0]);
        }
 
@@ -422,8 +422,9 @@ bond_update_sw_if_index (bond_per_thread_data_t * ptd, bond_if_t * bif,
 }
 
 static_always_inline void
-bond_tx_trace (vlib_main_t * vm, vlib_node_runtime_t * node, bond_if_t * bif,
-              vlib_buffer_t ** b, u32 n_left, u32 * h)
+bond_tx_trace (vlib_main_t *vm, vlib_node_runtime_t *node,
+              bond_per_thread_data_t *ptd, vlib_buffer_t **b, u32 n_left,
+              u32 *h)
 {
   uword n_trace = vlib_get_trace_count (vm, node);
 
@@ -441,15 +442,12 @@ bond_tx_trace (vlib_main_t * vm, vlib_node_runtime_t * node, bond_if_t * bif,
          t0->ethernet = *eth;
          t0->sw_if_index = vnet_buffer (b[0])->sw_if_index[VLIB_TX];
          if (!h)
-           {
-             t0->bond_sw_if_index =
-               *vec_elt_at_index (bif->active_members, 0);
-           }
+         t0->bond_sw_if_index = *vec_elt_at_index (ptd->active_members, 0);
          else
            {
-             t0->bond_sw_if_index =
-               *vec_elt_at_index (bif->active_members, h[0]);
-             h++;
+           t0->bond_sw_if_index =
+             *vec_elt_at_index (ptd->active_members, h[0]);
+           h++;
            }
        }
       b++;
@@ -473,7 +471,7 @@ VNET_DEVICE_CLASS_TX_FN (bond_dev_class) (vlib_main_t * vm,
   vnet_main_t *vnm = vnet_get_main ();
   bond_per_thread_data_t *ptd = vec_elt_at_index (bm->per_thread_data,
                                                  thread_index);
-  u32 p, sw_if_index;
+  u32 p, sw_if_index, n_numa_members;
 
   if (PREDICT_FALSE (bif->admin_up == 0))
     {
@@ -487,9 +485,10 @@ VNET_DEVICE_CLASS_TX_FN (bond_dev_class) (vlib_main_t * vm,
       return frame->n_vectors;
     }
 
-  n_members = vec_len (bif->active_members);
-  if (PREDICT_FALSE (n_members == 0))
+  clib_spinlock_lock_if_init (&bif->lockp);
+  if (PREDICT_FALSE (vec_len (bif->active_members) == 0))
     {
+      clib_spinlock_unlock_if_init (&bif->lockp);
       vlib_buffer_free (vm, vlib_frame_vector_args (frame), frame->n_vectors);
       vlib_increment_simple_counter (vnet_main.interface_main.sw_if_counters +
                                     VNET_INTERFACE_COUNTER_DROP,
@@ -500,14 +499,25 @@ VNET_DEVICE_CLASS_TX_FN (bond_dev_class) (vlib_main_t * vm,
       return frame->n_vectors;
     }
 
+  /*
+   * Take a snapshot of the active members as members may be freed
+   * asynchronously
+   */
+  vec_validate (ptd->active_members, vec_len (bif->active_members) - 1);
+  vec_copy (ptd->active_members, bif->active_members);
+  n_numa_members = bif->n_numa_members;
+  clib_spinlock_unlock_if_init (&bif->lockp);
+
+  n_members = vec_len (ptd->active_members);
+
   vlib_get_buffers (vm, from, bufs, n_left);
 
   /* active-backup mode, ship everything to first sw if index */
   if ((bif->lb == BOND_LB_AB) || PREDICT_FALSE (n_members == 1))
     {
-      sw_if_index = *vec_elt_at_index (bif->active_members, 0);
+      sw_if_index = *vec_elt_at_index (ptd->active_members, 0);
 
-      bond_tx_trace (vm, node, bif, bufs, frame->n_vectors, 0);
+      bond_tx_trace (vm, node, ptd, bufs, frame->n_vectors, 0);
       bond_update_sw_if_index (ptd, bif, from, bufs, &sw_if_index, n_left,
                               /* single_sw_if_index */ 1);
       goto done;
@@ -515,10 +525,10 @@ VNET_DEVICE_CLASS_TX_FN (bond_dev_class) (vlib_main_t * vm,
 
   if (bif->lb == BOND_LB_BC)
     {
-      sw_if_index = *vec_elt_at_index (bif->active_members, 0);
+      sw_if_index = *vec_elt_at_index (ptd->active_members, 0);
 
       bond_tx_no_hash (vm, bif, bufs, hashes, n_left, n_members, BOND_LB_BC);
-      bond_tx_trace (vm, node, bif, bufs, frame->n_vectors, 0);
+      bond_tx_trace (vm, node, ptd, bufs, frame->n_vectors, 0);
       bond_update_sw_if_index (ptd, bif, from, bufs, &sw_if_index, n_left,
                               /* single_sw_if_index */ 1);
       goto done;
@@ -527,7 +537,7 @@ VNET_DEVICE_CLASS_TX_FN (bond_dev_class) (vlib_main_t * vm,
   /* if have at least one member on local numa node, only members on local numa
      node will transmit pkts when bif->local_numa_only is enabled */
   if (bif->n_numa_members >= 1)
-    n_members = bif->n_numa_members;
+    n_members = n_numa_members;
 
   if (bif->lb == BOND_LB_RR)
     bond_tx_no_hash (vm, bif, bufs, hashes, n_left, n_members, BOND_LB_RR);
@@ -541,7 +551,7 @@ VNET_DEVICE_CLASS_TX_FN (bond_dev_class) (vlib_main_t * vm,
   else
     bond_hash_to_port (h, frame->n_vectors, n_members, 0);
 
-  bond_tx_trace (vm, node, bif, bufs, frame->n_vectors, h);
+  bond_tx_trace (vm, node, ptd, bufs, frame->n_vectors, h);
 
   bond_update_sw_if_index (ptd, bif, from, bufs, hashes, frame->n_vectors,
                           /* single_sw_if_index */ 0);
@@ -552,7 +562,7 @@ done:
       vlib_frame_t *f;
       u32 *to_next;
 
-      sw_if_index = *vec_elt_at_index (bif->active_members, p);
+      sw_if_index = *vec_elt_at_index (ptd->active_members, p);
       if (PREDICT_TRUE (ptd->per_port_queue[p].n_buffers))
        {
          f = vnet_get_frame_to_sw_interface (vnm, sw_if_index);
@@ -564,6 +574,7 @@ done:
          ptd->per_port_queue[p].n_buffers = 0;
        }
     }
+  vec_reset_length (ptd->active_members);
   return frame->n_vectors;
 }
 

diff --git a/src/vnet/bonding/node.h b/src/vnet/bonding/node.h
index c6602ef..c6efa5b 100644 (file)
--- a/src/vnet/bonding/node.h
+++ b/src/vnet/bonding/node.h
@@ -165,6 +165,7 @@ typedef struct
 {
   bond_per_port_queue_t *per_port_queue;
   void **data;
+  u32 *active_members;
 } bond_per_thread_data_t;
 
 typedef struct