mesa: fix redox winsys batch pool class math + serialize

The batch BO pool in redox_drm_cs.c had three memory-safety bugs: 1. Class math: the pool is documented to cover 'sizes 4 KiB to 512 KiB' across REDOX_DRM_BATCH_POOL_SIZE_CLASSES (8) slots, but the implementation computed the class as ceil(log2(byte_count)) from the raw byte count without applying the 4 KiB base. A 32 KiB batch request (which should map to class 3) ended up at class 15, which is >= 8, so the pool returned NULL and every GPU submission allocated a fresh BO — the pool was completely non-functional. 2. Undersized cached BOs: the BO was allocated with width0 = byte_count (exact request size) but cached against a class index that expected the full class capacity. A subsequent request that mapped to the same class but with a larger byte_count could receive a BO smaller than the write and silently overflow the heap. 3. No synchronization: the pool was shared across pipe_contexts but accessed without any mutex — concurrent submissions from multiple threads would race on rws->batch_pool[cls]. 4. Resource leak: batch_bo_release returned silently if the entry couldn't be cached, leaking the BO. Fixes: - Replace the per-call class math with a shared helper batch_bo_size_class() that returns cls = log2_ceil(byte_count) - 12 (with clamping for sub-4-KiB requests). Oversized requests return REDOX_DRM_BATCH_POOL_SIZE_CLASSES to signal 'fresh allocation, not pool'. - Add batch_bo_class_capacity() returning 4096 << cls. - Acquire now allocates BO at the full class capacity (not the request size), so any cached entry can serve any smaller request in the same class on recycle. - Acquire defensively verifies width0 >= byte_count before returning a cached entry; undersized entries are destroyed. - Release destroys entries that don't match the class capacity, and destroys redundant entries when a slot is already occupied (rather than leaking). - Pool is guarded by a new mtx_t batch_pool_mutex field on redox_drm_winsys (initialized in redox_drm_initialize, destroyed in redox_drm_destroy). The related 'format = 0 -> 0-byte GEM object' issue in redox_resource_byte_count is a separate bug (batches need a non-NONE format to get real storage); tracked as a follow-up. Verified by a standalone host gcc test of the class math covering 15 boundary cases (0, 1, 4096, 4097, 8192, 32768, 524288, 524289, 1048576, etc.); all match expected class assignments.
2026-07-27 22:17:03 +09:00
parent dd6dd99cea
commit 16f74ab87c
3 changed files with 166 additions and 56 deletions
@@ -84,70 +84,168 @@ struct RedoxCsWaitWire {
   uint64_t completed_seqno;  /* response: highest completed seqno */
 };

-/* Acquire a batch BO of the given size from the ring-buffer pool,
- * or allocate a fresh one. Returns NULL on allocation failure.
+/* ---- Batch BO pool ----------------------------------------------------- */
+
+/* The pool keeps one BO per power-of-2 size class, covering
+ *   cls 0:   4 KiB   (4096    = 1 << 12)
+ *   cls 1:   8 KiB   (8192    = 1 << 13)
+ *   ...
+ *   cls 7: 512 KiB   (524288  = 1 << 19)
 *
- * The pool keeps one BO per power-of-2 size class. Larger requests
- * are always freshly allocated (the pool only caches sizes 4 KiB to
- * 512 KiB). On release, the BO is recycled back to its size class
- * slot only if the slot is empty.
+ * Requests smaller than 4 KiB are clamped up to class 0 so that tiny
+ * batches still benefit from recycling.  Requests larger than 512 KiB
+ * are always freshly allocated (never pooled) and destroyed on release.
+ *
+ * Every BO placed in a pool slot is sized to the FULL class capacity
+ * (4 KiB << cls), never the exact request size.  This guarantees any
+ * cached entry can serve any request that maps to that class — the
+ * root cause of the previous heap-overflow bug was caching a
+ * request-sized BO and later serving it to a larger request in the
+ * same class.
+ */
+
+/* 4 KiB = 1 << REDOX_DRM_BATCH_POOL_BASE_BITS. */
+#define REDOX_DRM_BATCH_POOL_BASE_BITS 12
+
+/* Compute the batch pool size class for byte_count.
+ *
+ * Returns a class index in [0, REDOX_DRM_BATCH_POOL_SIZE_CLASSES-1]
+ * for poolable sizes, or REDOX_DRM_BATCH_POOL_SIZE_CLASSES if the
+ * request exceeds the largest class (512 KiB) and must be freshly
+ * allocated without pooling.
+ *
+ * Requests of 0..4096 bytes map to class 0.
+ */
+static unsigned
+batch_bo_size_class(size_t byte_count)
+{
+   /* Clamp sub-4-KiB requests up to the smallest class. byte_count
+    * is guaranteed > 0 by all callers, but a 0 argument is handled
+    * safely (returns class 0). */
+   if (byte_count <= (size_t)4096)
+      return 0;
+
+   /* ceil(log2(byte_count)): the number of bits needed to represent
+    * (byte_count - 1), which is the bit-position of the highest set
+    * bit of (byte_count - 1) plus one.  This is the smallest power
+    * of two >= byte_count. */
+   unsigned bits = 0;
+   size_t v = byte_count - 1;
+   while (v > 0) {
+      bits++;
+      v >>= 1;
+   }
+
+   if (bits < REDOX_DRM_BATCH_POOL_BASE_BITS)
+      return 0;
+   return bits - REDOX_DRM_BATCH_POOL_BASE_BITS;
+}
+
+/* Capacity (in bytes) of a given size class. */
+static size_t
+batch_bo_class_capacity(unsigned cls)
+{
+   return (size_t)4096 << cls;
+}
+
+/* Acquire a batch BO of at least byte_count bytes from the ring-buffer
+ * pool, or allocate a fresh one.  Returns NULL on allocation failure.
+ *
+ * Pool-served BOs are sized to the full class capacity (4 KiB << cls).
+ * Oversized requests (> 512 KiB) bypass the pool entirely and get an
+ * exact-sized fresh allocation.
 */
 static struct pipe_resource *
 batch_bo_acquire(struct redox_drm_winsys *rws, size_t byte_count)
 {
-    if (byte_count == 0 || !rws)
-       return NULL;
+   if (byte_count == 0 || !rws)
+      return NULL;

-    if (byte_count > (1U << (REDOX_DRM_BATCH_POOL_SIZE_CLASSES - 1 + 12)))
-       return NULL;
+   unsigned cls = batch_bo_size_class(byte_count);

-    unsigned cls = 0;
-    size_t sz = byte_count - 1;
-    while (sz > 0) {
-       cls++;
-       sz >>= 1;
-    }
-    if (cls >= 12)
-       cls = 12;
-    if (cls >= REDOX_DRM_BATCH_POOL_SIZE_CLASSES)
-       return NULL;
+   /* Requests exceeding the largest pool class (512 KiB) always get
+    * a fresh, exact-sized allocation — they are never pooled. */
+   if (cls >= REDOX_DRM_BATCH_POOL_SIZE_CLASSES) {
+      struct pipe_resource templ = {0};
+      templ.width0 = (uint32_t)byte_count;
+      templ.height0 = 1;
+      templ.depth0 = 1;
+      templ.target = PIPE_BUFFER;
+      return redox_drm_bo_create(rws, &templ);
+   }

-    if (rws->batch_pool[cls]) {
-       struct pipe_resource *pr = rws->batch_pool[cls];
-       rws->batch_pool[cls] = NULL;
-       return pr;
-    }
+   size_t capacity = batch_bo_class_capacity(cls);

-    struct pipe_resource templ = {0};
-    templ.width0 = byte_count;
-    templ.height0 = 1;
-    templ.depth0 = 1;
-    templ.target = PIPE_BUFFER;
-    return redox_drm_bo_create(rws, &templ);
+   /* Try the per-class cache slot under the pool lock. */
+   struct pipe_resource *cached = NULL;
+   mtx_lock(&rws->batch_pool_mutex);
+   if (rws->batch_pool[cls]) {
+      cached = rws->batch_pool[cls];
+      rws->batch_pool[cls] = NULL;
+   }
+   mtx_unlock(&rws->batch_pool_mutex);
+
+   if (cached) {
+      /* Defensive: verify the cached BO can actually hold the
+       * request.  A correctly maintained pool always satisfies
+       * this, but corruption or a logic error must never lead to
+       * an undersized write (heap overflow). */
+      if (cached->width0 >= byte_count)
+         return cached;
+
+      debug_printf("redox: batch pool: undersized cached BO "
+                   "(width0=%u, need=%zu) — destroying\n",
+                   cached->width0, byte_count);
+      redox_drm_bo_destroy(rws, cached);
+      /* Fall through to allocate a fresh BO. */
+   }
+
+   /* Allocate a fresh BO at the full class capacity so the recycled
+    * entry can serve any smaller request that maps to this class. */
+   struct pipe_resource templ = {0};
+   templ.width0 = (uint32_t)capacity;
+   templ.height0 = 1;
+   templ.depth0 = 1;
+   templ.target = PIPE_BUFFER;
+   return redox_drm_bo_create(rws, &templ);
 }

+/* Release a batch BO back to the ring-buffer pool, or destroy it if it
+ * cannot be cached.
+ *
+ * Only BOs whose width0 exactly matches a class capacity (4 KiB << cls)
+ * are recyclable.  Oversized fresh allocations and BOs with non-standard
+ * widths are destroyed rather than leaked.
+ */
 static void
 batch_bo_release(struct redox_drm_winsys *rws, struct pipe_resource *pr)
 {
-    if (!pr || !rws)
-       return;
+   if (!pr || !rws)
+      return;

-    size_t sz = pr->width0;
-    if (sz == 0 || sz > (1U << (REDOX_DRM_BATCH_POOL_SIZE_CLASSES - 1 + 12)))
-       return;
-    unsigned cls = 0;
-    size_t s = sz - 1;
-    while (s > 0) {
-       cls++;
-       s >>= 1;
-    }
-    if (cls >= 12)
-       cls = 12;
-    if (cls >= REDOX_DRM_BATCH_POOL_SIZE_CLASSES)
-       return;
-    if (rws->batch_pool[cls])
-       return;
-    rws->batch_pool[cls] = pr;
+   unsigned cls = batch_bo_size_class(pr->width0);
+
+   /* Only BOs whose width0 matches the full class capacity are
+    * recyclable.  Oversized fresh allocations (cls >= SIZE_CLASSES)
+    * and BOs whose width0 doesn't align to a class boundary are
+    * destroyed instead of being cached — caching them would risk
+    * undersized serves on the next acquire. */
+   if (cls >= REDOX_DRM_BATCH_POOL_SIZE_CLASSES ||
+       pr->width0 != batch_bo_class_capacity(cls)) {
+      redox_drm_bo_destroy(rws, pr);
+      return;
+   }
+
+   mtx_lock(&rws->batch_pool_mutex);
+   if (rws->batch_pool[cls]) {
+      /* Slot already occupied — destroy the redundant BO rather
+       * than leaking it. */
+      mtx_unlock(&rws->batch_pool_mutex);
+      redox_drm_bo_destroy(rws, pr);
+      return;
+   }
+   rws->batch_pool[cls] = pr;
+   mtx_unlock(&rws->batch_pool_mutex);
 }

 uint64_t
@@ -401,6 +401,8 @@ redox_drm_initialize(struct redox_drm_winsys *rws)
   for (unsigned i = 0; i < REDOX_DRM_BATCH_POOL_SIZE_CLASSES; i++)
      rws->batch_pool[i] = NULL;

+   (void) mtx_init(&rws->batch_pool_mutex, mtx_plain);
+
   rws->cs = redox_drm_cs_create(rws);
   if (!rws->cs) {
      debug_printf("redox: failed to create cs state\n");
@@ -415,14 +417,16 @@ redox_drm_destroy(struct redox_drm_winsys *rws)
   if (!rws)
      return;

-   for (unsigned i = 0; i < REDOX_DRM_BATCH_POOL_SIZE_CLASSES; i++) {
-      if (rws->batch_pool[i]) {
-         redox_drm_bo_destroy(rws, rws->batch_pool[i]);
-         rws->batch_pool[i] = NULL;
-      }
-   }
+    for (unsigned i = 0; i < REDOX_DRM_BATCH_POOL_SIZE_CLASSES; i++) {
+       if (rws->batch_pool[i]) {
+          redox_drm_bo_destroy(rws, rws->batch_pool[i]);
+          rws->batch_pool[i] = NULL;
+       }
+    }

-   if (rws->cs) {
+    mtx_destroy(&rws->batch_pool_mutex);
+
+    if (rws->cs) {
      redox_drm_cs_destroy(rws->cs);
      rws->cs = NULL;
   }
@@ -11,6 +11,8 @@
 #include "util/u_inlines.h"
 #include "winsys/sw_winsys.h"

+#include "c11/threads.h"  /* mtx_t for batch_pool_mutex */
+
 struct redox_drm_cs;
 struct redox_drm_surface;
 struct pipe_resource;
@@ -32,6 +34,12 @@ struct redox_drm_winsys {
    */
   struct redox_drm_cs *cs;

+   /* Protects batch_pool[] against concurrent acquire/release from
+    * multiple pipe_contexts sharing this winsys. Mesa drivers may
+    * create several contexts per screen, and each can flush (submit)
+    * independently from different threads. */
+   mtx_t batch_pool_mutex;
+
   /* Batch BO ring-buffer pool. batch_pool[i] caches the largest
    * recently-recycled BO whose size class == i. Acquire pulls from
    * the pool if a matching size class is cached; release puts a BO