From 4b7fde2335dc11f0758c1d786345a6f4806bb146 Mon Sep 17 00:00:00 2001 From: Steven Luong Date: Thu, 3 Apr 2025 10:22:10 -0700 Subject: [PATCH] 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 --- src/vnet/bonding/device.c | 77 +++++++++++++++++++++++++++-------------------- src/vnet/bonding/node.h | 1 + 2 files changed, 45 insertions(+), 33 deletions(-) diff --git a/src/vnet/bonding/device.c b/src/vnet/bonding/device.c index a0b93fccde1..81661c15eec 100644 --- 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 c6602ef01b9..c6efa5b2e72 100644 --- 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 -- 2.16.6