xen/events: fix race with set_global_virq_handler()

There is a possible race scenario between set_global_virq_handler()
and clear_global_virq_handlers() targeting the same domain, which
might result in that domain ending as a zombie domain.

In case set_global_virq_handler() is being called for a domain which
is just dying, it might happen that clear_global_virq_handlers() is
running first, resulting in set_global_virq_handler() taking a new
reference for that domain and entering in the global_virq_handlers[]
array afterwards. The reference will never be dropped, thus the domain
will never be freed completely.

This can be fixed by checking the is_dying state of the domain inside
the region guarded by global_virq_handlers_lock. In case the domain is
dying, handle it as if the domain wouldn't exist, which will be the
case in near future anyway.

Fixes: 87521589aa6a ("xen: allow global VIRQ handlers to be delegated to other domains")
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
master commit: 4d8acc9c1cf14233dda21dd3a7791b5a84b0f6c3
master date: 2025-01-09 17:34:01 +0100

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index e6ec556603f7dd9a2b4a624a7a66e8a2f4d871d5..3caf74ae776d3c0895d409f0c3c5e68a6f91ce38 100644 (file)
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -974,6 +974,7 @@ void send_global_virq(uint32_t virq)
 int set_global_virq_handler(struct domain *d, uint32_t virq)
 {
     struct domain *old;
+    int rc = 0;
 
     if (virq >= NR_VIRQS)
         return -EINVAL;
@@ -987,14 +988,32 @@ int set_global_virq_handler(struct domain *d, uint32_t virq)
         return -EINVAL;
 
     spin_lock(&global_virq_handlers_lock);
-    old = global_virq_handlers[virq];
-    global_virq_handlers[virq] = d;
+
+    /*
+     * Note that this check won't guarantee that a domain just going down can't
+     * be set as the handling domain of a virq, as the is_dying indicator might
+     * change just after testing it.
+     * This isn't going to be a major problem, as clear_global_virq_handlers()
+     * is guaranteed to run afterwards and it will reset the handling domain
+     * for the virq to the hardware domain.
+     */
+    if ( d->is_dying != DOMDYING_alive )
+    {
+        old = d;
+        rc = -EINVAL;
+    }
+    else
+    {
+        old = global_virq_handlers[virq];
+        global_virq_handlers[virq] = d;
+    }
+
     spin_unlock(&global_virq_handlers_lock);
 
     if (old != NULL)
         put_domain(old);
 
-    return 0;
+    return rc;
 }
 
 static void clear_global_virq_handlers(struct domain *d)