Bluetooth: Controller: Fix Tx Ack FIFO index corruption under race
Fix Tx Ack FIFO's first index being advanced beyond a
recorded ack_last value in a node_rx when under race
between ll_rx_get() being pre-empted while executing the
`tx_cmplt_get()` and a call to `ll_rx_put()` in an
interrupt service routine.
Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
(cherry picked from commit f6495d8b43933f6d281311a57e2acb14ae752786)
diff --git a/subsys/bluetooth/controller/ll_sw/ull.c b/subsys/bluetooth/controller/ll_sw/ull.c
index 41df97a..6ff1f77 100644
--- a/subsys/bluetooth/controller/ll_sw/ull.c
+++ b/subsys/bluetooth/controller/ll_sw/ull.c
@@ -959,8 +959,8 @@
uint8_t ll_rx_get(void **node_rx, uint16_t *handle)
{
struct node_rx_pdu *rx;
- memq_link_t *link;
uint8_t cmplt = 0U;
+ memq_link_t *link;
#if defined(CONFIG_BT_CONN) || \
(defined(CONFIG_BT_OBSERVER) && defined(CONFIG_BT_CTLR_ADV_EXT)) || \
@@ -975,6 +975,20 @@
*node_rx = NULL;
+#if defined(CONFIG_BT_CONN) || defined(CONFIG_BT_CTLR_ADV_ISO)
+ /* Save the tx_ack FIFO's last index to avoid the value being changed if there were no
+ * Rx PDUs and we were pre-empted before calling `tx_cmplt_get()`, that may advance the
+ * first index beyond ack_last value recorded in node_rx enqueued by `ll_rx_put()` call
+ * when we are in the `else` clause below.
+ */
+ uint8_t tx_ack_last = mfifo_fifo_tx_ack.l;
+
+ /* Ensure that the value is fetched before call to memq_peek, i.e. compiler shall not
+ * reorder memory write before above read.
+ */
+ cpu_dmb();
+#endif /* CONFIG_BT_CONN || CONFIG_BT_CTLR_ADV_ISO */
+
link = memq_peek(memq_ll_rx.head, memq_ll_rx.tail, (void **)&rx);
if (link) {
#if defined(CONFIG_BT_CONN) || defined(CONFIG_BT_CTLR_ADV_ISO)
@@ -1055,8 +1069,11 @@
#if defined(CONFIG_BT_CONN) || defined(CONFIG_BT_CTLR_ADV_ISO)
}
} else {
- cmplt = tx_cmplt_get(handle, &mfifo_fifo_tx_ack.f,
- mfifo_fifo_tx_ack.l);
+ /* Use the saved ack last value, before call was done to memq_peek, to ensure we
+ * do not advance the first index `f` beyond the value that was the last index `l`
+ * when memq_peek was called.
+ */
+ cmplt = tx_cmplt_get(handle, &mfifo_fifo_tx_ack.f, tx_ack_last);
#endif /* CONFIG_BT_CONN || CONFIG_BT_CTLR_ADV_ISO */
}
