espressif/_bleio: guard buffer state shared with the NimBLE host task

CharacteristicBuffer and PacketBuffer ringbufs are filled from the
nimble_host task, which preempts the VM task at arbitrary points, but the
non-atomic ringbuf operations were unguarded: the comment claiming the BLE
host task "won't interrupt us" has been wrong since the code was written
(the host task runs at a much higher priority on the same core).
PacketBuffer's pending outgoing buffers have the same problem: write() on
the VM task appends to them while queue_next_write() on the host task
consumes them.

Guard these operations with interrupts disabled, mirroring the nordic
port's critical sections around the same code, in the same places.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Author: dhalbert

ports/espressif/common-hal/_bleio/CharacteristicBuffer.c

@@ -16,12 +16,19 @@
 #include "shared-bindings/_bleio/__init__.h"
 #include "shared-bindings/_bleio/Connection.h"
 #include "shared-bindings/_bleio/CharacteristicBuffer.h"
+#include "shared-bindings/microcontroller/__init__.h"
 
 #include "supervisor/shared/tick.h"
 
 #include "common-hal/_bleio/ble_events.h"
 
+// The ringbuf is filled from the nimble_host task, which preempts the VM task
+// at arbitrary points, and ringbuf operations are not atomic, so guard them
+// with interrupts disabled.
+
+// Runs on the nimble_host task.
 void bleio_characteristic_buffer_extend(bleio_characteristic_buffer_obj_t *self, const uint8_t *data, size_t len) {
+    common_hal_mcu_disable_interrupts();
     if (self->watch_for_interrupt_char) {
         for (uint16_t i = 0; i < len; i++) {
             if (data[i] == mp_interrupt_char) {
@@ -34,6 +41,7 @@ void bleio_characteristic_buffer_extend(bleio_characteristic_buffer_obj_t *self,
     } else {
         ringbuf_put_n(&self->ringbuf, data, len);
     }
+    common_hal_mcu_enable_interrupts();
 }
 
 void _common_hal_bleio_characteristic_buffer_construct(bleio_characteristic_buffer_obj_t *self,
@@ -70,20 +78,23 @@ uint32_t common_hal_bleio_characteristic_buffer_read(bleio_characteristic_buffer
         }
     }
 
+    common_hal_mcu_disable_interrupts();
     uint32_t num_bytes_read = ringbuf_get_n(&self->ringbuf, data, len);
+    common_hal_mcu_enable_interrupts();
     return num_bytes_read;
 }
 
-// NOTE: The nRF port has protection around these operations because the ringbuf
-// is filled from an interrupt. On ESP the ringbuf is filled from the BLE host
-// task that won't interrupt us.
-
 uint32_t common_hal_bleio_characteristic_buffer_rx_characters_available(bleio_characteristic_buffer_obj_t *self) {
-    return ringbuf_num_filled(&self->ringbuf);
+    common_hal_mcu_disable_interrupts();
+    uint32_t count = ringbuf_num_filled(&self->ringbuf);
+    common_hal_mcu_enable_interrupts();
+    return count;
 }
 
 void common_hal_bleio_characteristic_buffer_clear_rx_buffer(bleio_characteristic_buffer_obj_t *self) {
+    common_hal_mcu_disable_interrupts();
     ringbuf_clear(&self->ringbuf);
+    common_hal_mcu_enable_interrupts();
 }
 
 bool common_hal_bleio_characteristic_buffer_deinited(bleio_characteristic_buffer_obj_t *self) {

ports/espressif/common-hal/_bleio/PacketBuffer.c

@@ -15,6 +15,7 @@
 #include "shared-bindings/_bleio/__init__.h"
 #include "shared-bindings/_bleio/Connection.h"
 #include "shared-bindings/_bleio/PacketBuffer.h"
+#include "shared-bindings/microcontroller/__init__.h"
 
 #include "supervisor/shared/tick.h"
 #include "supervisor/shared/bluetooth/serial.h"
@@ -23,6 +24,11 @@
 
 #include "host/ble_att.h"
 
+// The ringbuf and the pending outgoing buffers are shared with the nimble_host
+// task, which preempts the VM task at arbitrary points, and ringbuf operations
+// are not atomic, so guard the shared accesses with interrupts disabled.
+
+// Runs on the nimble_host task.
 void bleio_packet_buffer_extend(bleio_packet_buffer_obj_t *self, uint16_t conn_handle, const uint8_t *data, size_t len) {
     if (self->conn_handle != conn_handle) {
         return;
@@ -34,6 +40,8 @@ void bleio_packet_buffer_extend(bleio_packet_buffer_obj_t *self, uint16_t conn_h
         return;
     }
 
+    common_hal_mcu_disable_interrupts();
+
     // Make room for the new value by dropping the oldest packets first.
     while (ringbuf_num_empty(&self->ringbuf) < len + sizeof(uint16_t)) {
         uint16_t packet_length;
@@ -45,6 +53,8 @@ void bleio_packet_buffer_extend(bleio_packet_buffer_obj_t *self, uint16_t conn_h
     }
     ringbuf_put_n(&self->ringbuf, (uint8_t *)&len, sizeof(uint16_t));
     ringbuf_put_n(&self->ringbuf, data, len);
+
+    common_hal_mcu_enable_interrupts();
 }
 
 static int packet_buffer_on_ble_client_evt(struct ble_gap_event *event, void *param);
@@ -264,6 +274,8 @@ mp_int_t common_hal_bleio_packet_buffer_readinto(bleio_packet_buffer_obj_t *self
         return 0;
     }
 
+    common_hal_mcu_disable_interrupts();
+
     // Get packet length, which is in first two bytes of packet.
     uint16_t packet_length;
     ringbuf_get_n(&self->ringbuf, (uint8_t *)&packet_length, sizeof(uint16_t));
@@ -282,6 +294,8 @@ mp_int_t common_hal_bleio_packet_buffer_readinto(bleio_packet_buffer_obj_t *self
         ret = packet_length;
     }
 
+    common_hal_mcu_enable_interrupts();
+
     return ret;
 }
 
@@ -324,6 +338,10 @@ mp_int_t common_hal_bleio_packet_buffer_write(bleio_packet_buffer_obj_t *self, c
 
     size_t num_bytes_written = 0;
 
+    // The nimble_host task may modify pending_size, pending_index, and
+    // packet_queued via queue_next_write(), so guard the append.
+    common_hal_mcu_disable_interrupts();
+
     uint32_t *pending = self->outgoing[self->pending_index];
 
     if (self->pending_size == 0) {
@@ -335,6 +353,8 @@ mp_int_t common_hal_bleio_packet_buffer_write(bleio_packet_buffer_obj_t *self, c
     self->pending_size += len;
     num_bytes_written += len;
 
+    common_hal_mcu_enable_interrupts();
+
     // If no writes are queued then sneak in this data.
     if (!self->packet_queued) {
         // This will queue up the packet even if it can't send immediately.