Re: Question regarding Locking in the MMIO Handling Framework on Arm

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

Hello Jullien,

On 7/9/26 2:09 PM, Julien Grall wrote:
> Hi Oleksii,
>
> Thanks for the report.
>
> On 09/07/2026 11:49, Oleksii Kurochko wrote:
> > I have a question regarding the locking in the MMIO handling framework 
> > on Arm.
> > Is it sufficient to have read_lock() only in find_mmio_handler()? If
> > register_mmio_handler() is executed in parallel with 
> > find_mmio_handler() (which I assume was the reason for introducing the 
> > rwlock), aren't we still exposed to a race condition?
> When the read-write lock was introduced, we didn't sort the entries. So 
> it was fine at the time. This bug was introduced by commit 8047e090f4 
> ("xen/arm: io: Use binary search for mmio handler lookup").
> With this change, then we...
>
> > find_mmio_handler() returns a pointer to a handler, but the object it 
> > points to could be changed by a subsequent call to 
> > register_mmio_handler().
> > If register_mmio_handler() runs between find_mmio_handler() and 
> > handle_{write,read}(), we could end up operating on a different 
> > handler than the one that was originally found.
> > In other words, shouldn't we acquire the read_lock() in 
> > try_handle_mmio() and keep it held for the entire duration of 
> > try_handle_mmio()?
> ... either need to keep the lock for longer
I assume that register_mmio_handler() won't be called very often
after domain creation so it looks to me that this fix is simpler then ...

 or rework the code to
> allocate the handler structure.
... this. IIUC, you suggestion is to do something like:

diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index 9707cadcf80e..a587135a6907 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -238,24 +238,24 @@ void register_mmio_handler(struct domain *d,
                            paddr_t addr, paddr_t size, void *priv)
 {
     struct vmmio *vmmio = &d->arch.vmmio;
-    struct mmio_handler *handler;
+    struct mmio_handler *handler = xzalloc(struct mmio_handler);

-    BUG_ON(vmmio->num_entries >= vmmio->max_num_entries);
-
-    write_lock(&vmmio->lock);
-
-    handler = &vmmio->handlers[vmmio->num_entries];
+    BUG_ON(!handler);

-    handler->ops = ops;
+    /* Fully initialize *before* publishing. */
+    handler->ops  = ops;
     handler->addr = addr;
     handler->size = size;
     handler->priv = priv;

-    vmmio->num_entries++;
+    write_lock(&vmmio->lock);
+
+    BUG_ON(vmmio->num_entries >= vmmio->max_num_entries);
+
+    vmmio->handlers[vmmio->num_entries++] = handler;

-    /* Sort mmio handlers in ascending order based on base address */
-    sort(vmmio->handlers, vmmio->num_entries, sizeof(struct mmio_handler),
-         cmp_mmio_handler, swap_mmio_handler);
+    sort(vmmio->handlers, vmmio->num_entries, sizeof(*vmmio->handlers),
+         cmp_mmio_handler, swap_mmio_handler /* now swaps pointers */);

     write_unlock(&vmmio->lock);
 }

(of course, whith updating of handler field to "struct mmio_handler 
**handlers;").


Also, I thought about just use local variable for handler (probably that 
what you meant):
diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index 9707cadcf80e..393018e02a83 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -107,19 +107,33 @@ static void swap_mmio_handler(void *_a, void *_b)
     SWAP(*a, *b);
 }

-static const struct mmio_handler *find_mmio_handler(struct domain *d,
-                                                    paddr_t gpa)
+static bool find_mmio_handler(struct domain *d, paddr_t gpa,
+                              struct mmio_handler *out)
 {
     struct vmmio *vmmio = &d->arch.vmmio;
     struct mmio_handler key = {.addr = gpa};
     const struct mmio_handler *handler;
+    bool found = false;

     read_lock(&vmmio->lock);
+
     handler = bsearch(&key, vmmio->handlers, vmmio->num_entries,
                       sizeof(*handler), cmp_mmio_handler);
+    if ( handler )
+    {
+        /*
+         * Copy the handler while holding the lock: a concurrent
+         * register_mmio_handler() re-sorts the array in place, so the
+         * entry pointed to by the bsearch() result may change once the
+         * lock is released.
+         */
+        *out = *handler;
+        found = true;
+    }
+
     read_unlock(&vmmio->lock);

-    return handler;
+    return found;
 }

 void try_decode_instruction(const struct cpu_user_regs *regs,
@@ -187,7 +201,7 @@ enum io_state try_handle_mmio(struct cpu_user_regs 
*regs,
                               mmio_info_t *info)
 {
     struct vcpu *v = current;
-    const struct mmio_handler *handler = NULL;
+    struct mmio_handler handler;
     int rc;

     ASSERT(info->dabt.ec == HSR_EC_DATA_ABORT_LOWER_EL);
@@ -198,8 +212,7 @@ enum io_state try_handle_mmio(struct cpu_user_regs 
*regs,
         return IO_ABORT;
     }

-    handler = find_mmio_handler(v->domain, info->gpa);
-    if ( !handler )
+    if ( !find_mmio_handler(v->domain, info->gpa, &handler) )
     {
         bool trap_unmapped = v->domain->options &
 
XEN_DOMCTL_CDF_trap_unmapped_accesses;
@@ -209,7 +222,7 @@ enum io_state try_handle_mmio(struct cpu_user_regs 
*regs,
         else if ( rc == IO_UNHANDLED && !trap_unmapped )
         {
             /* Fallback to the unmapped handler. */
-            handler = &unmapped_handler;
+            handler = unmapped_handler;
         } else {
             return rc;
         }
@@ -228,9 +241,9 @@ enum io_state try_handle_mmio(struct cpu_user_regs 
*regs,
      * instruction on the emulated MMIO region.
      */
     if ( info->dabt.write )
-        return handle_write(handler, v, info);
+        return handle_write(&handler, v, info);
     else
-        return handle_read(handler, v, info);
+        return handle_read(&handler, v, info);
 }


What I am also thinking about if read-write lock is enough here. For 
example, if we will have hypothetical MMIO unregistered (why we don't 
have it now? Will we ever need it?) then we could be in a trouble:
CPU0 (vCPU trapping MMIO)               CPU1 (hypothetical unregister)

try_handle_mmio()
  find_mmio_handler()
    read_lock()
    bsearch() -> finds handler H
    read_unlock()          <-- protection ends here
                                          unregister_mmio_handler()
                                           write_lock()
                                           remove H's pointer from array
                                           write_unlock()
                                         xfree(H)   <-- H's memory freed
  handle_write(H, ...)
    H->ops->write(...)     <-- use-after-free: reads freed memory,
                               calls through a dangling ops pointer

If we should care about that hypothetical unregister case then it is 
better just to go with solution of "to keep the lock for longer" 
basically while handler is found and used in try_handle_mmio().
~ Oleksii