[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v2] x86: irq: Do not BUG_ON multiple unbind calls for shared pirqs



Hey Jan,

On 12/18/19 2:42 PM, Jan Beulich wrote:
On 18.12.2019 11:53, Varad Gautam wrote:
XEN_DOMCTL_destroydomain creates a continuation if domain_kill -ERESTARTS.
In that scenario, it is possible to receive multiple _pirq_guest_unbind
calls for the same pirq from domain_kill, if the pirq has not yet been
removed from the domain's pirq_tree, as:
   domain_kill()
     -> domain_relinquish_resources()
       -> pci_release_devices()
         -> pci_clean_dpci_irq()
           -> pirq_guest_unbind()
             -> __pirq_guest_unbind()

For a shared pirq (nr_guests > 1), the first call would zap the current
domain from the pirq's guests[] list, but the action handler is never freed
as there are other guests using this pirq. As a result, on the second call,
__pirq_guest_unbind searches for the current domain which has been removed
from the guests[] list, and hits a BUG_ON.
There must be more to this, seeing the cleanup_domain_irq_pirq()
invocation at the end of pirq_guest_unbind(), which ought to be
reached in the case you describe.


The calls to __pirq_guest_unbind and cleanup_domain_irq_pirq from pirq_guest_unbind are going to be mutually exclusive, since irq defaults to 0.


--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -1863,7 +1863,16 @@ static irq_guest_action_t *__pirq_guest_unbind(
for ( i = 0; (i < action->nr_guests) && (action->guest[i] != d); i++ )
          continue;
-    BUG_ON(i == action->nr_guests);
+    if ( i == action->nr_guests ) {
Brace on its own line please.

+        ASSERT(action->nr_guests > 0) ;
Stray blank.

+        /* In case the pirq was shared, unbound for this domain in an earlier 
call, but still
+         * existed on the domain's pirq_tree, we still reach here if there are 
any later
+         * unbind calls on the same pirq. Return if such an unbind happens. */
+        if ( action->shareable )
Long lines and malformed comment.


Handled all style fixups in v3.


Do you perhaps also want to check that you take this path only
for dying guests?


The patch is about making pirq_guest_unbind() resilient/safe to multiple calls. At present, this happens when domain_kill -ERESTARTs for dying guests. It might also happen in other cases - and shouldn't cause xen to crash.


Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel





Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.