Re: [PATCH v6 1/7] xen/events: fix race with set_global_virq_handler()

[Juergen Gross]

On 07.01.25 16:23, Jan Beulich wrote:

On 07.01.2025 11:17, Juergen Gross wrote:

--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -979,6 +979,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;

          return -EINVAL;
@@ -992,14 +993,23 @@ int set_global_virq_handler(struct domain *d, uint32_t 
virq)
          return -EINVAL;

-    old = global_virq_handlers[virq];
-    global_virq_handlers[virq] = d;
+
+    if ( d->is_dying != DOMDYING_alive )
+    {
+        old = d;
+        rc = -EINVAL;
+    }

if (virq >= NR_VIRQS) spin_lock(&global_virq_handlers_lock);

While I can see how this eliminates the zombie domain aspect, this doesn't
fully eliminate the race. Doing so would require (also) using the domain's
event lock. Assuming we're okay with the remaining race, imo a code comment
would be needed to state this (including the fact that it's then
unpredictable whether this operation might still succeed for a domain
already having d->is_dying != DOMDYING_alive).

AFAIU you mean that it is still possible to set a domain to handle a virq
when it is in the process of going down, especially if is_dying is set just
after it has been tested to be DOMDYING_alive?

I don't see this being a problem, as the same would happen if the domain
would go down just a millisecond later. This is something we will never be
able to handle.

And after all the call of clear_global_virq_handlers() will now reset the
handling domain to the hardware domain in all cases.

Plus the way you do it the early success path remains; ideally that case
would also fail for an already dying domain.

Same again: clear_global_virq_handlers() will reset the handling domain.

+    else
+    {
+        old = global_virq_handlers[virq];
+        global_virq_handlers[virq] = d;
+    }
      spin_unlock(&global_virq_handlers_lock);

Nit: Much like you do at the top of your addition, a new blank line at the
bottom would be nice.

Will add that if a respin is needed.


Juergen

OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

OpenPGP_signature.asc
Description: OpenPGP digital signature