| Message ID | 1580290087-20636-1-git-send-email-vrd@amazon.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] x86: irq: Do not BUG_ON multiple unbind calls for shared pirqs | expand |
Hi, On 29/01/2020 09:28, 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. > > Make __pirq_guest_unbind safe to be called multiple times by letting xen > continue if a shared pirq has already been unbound from this guest. The > PIRQ will be cleaned up from the domain's pirq_tree during the destruction > in complete_domain_destroy anyways. > > Signed-off-by: Varad Gautam <vrd@amazon.de> > Reviewed-by: Paul Durrant <paul@xen.org> > CC: Jan Beulich <jbeulich@suse.com> > CC: Julien Grall <julien@xen.org> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Andrew Cooper <andrew.cooper3@citrix.com> > > v2: Split the check on action->nr_guests > 0 and make it an ASSERT. > v3: Style fixups. > --- > xen/arch/x86/irq.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c > index 310ac00..4b172eb 100644 > --- a/xen/arch/x86/irq.c > +++ b/xen/arch/x86/irq.c > @@ -1681,7 +1681,20 @@ 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 ) > + { > + ASSERT(action->nr_guests > 0); > + /* > + * 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 ) > + return NULL; > + BUG(); Please see my comment on v2 about the BUG(). I am still unconvinced this is the right error handling here. Cheers,
Hello, Thanks for the patch! Next time could you please try to reply to the previous questions before sending a new version: https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg00257.html On Wed, Jan 29, 2020 at 10:28:07AM +0100, 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. > > Make __pirq_guest_unbind safe to be called multiple times by letting xen > continue if a shared pirq has already been unbound from this guest. The > PIRQ will be cleaned up from the domain's pirq_tree during the destruction > in complete_domain_destroy anyways. So AFAICT this is because pt_pirq_softirq_active() returns true in pci_clean_dpci_irq() and hence the iteration is stopped and hvm_domain_irq(d)->dpci is not set to NULL. Would it be possible to clean the already processed IRQs from the domain pirq_tree? pci_clean_dpci_irq() already seems to free part of this structure, and would be nicer IMO if we didn't leave cleaned up stuff behind on ERESTART. Thanks, Roger.
On 29.01.2020 11:30, Roger Pau Monné wrote: > Hello, > > Thanks for the patch! Next time could you please try to reply to the > previous questions before sending a new version: > > https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg00257.html > > On Wed, Jan 29, 2020 at 10:28:07AM +0100, 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. >> >> Make __pirq_guest_unbind safe to be called multiple times by letting xen >> continue if a shared pirq has already been unbound from this guest. The >> PIRQ will be cleaned up from the domain's pirq_tree during the destruction >> in complete_domain_destroy anyways. > > So AFAICT this is because pt_pirq_softirq_active() returns true in > pci_clean_dpci_irq() and hence the iteration is stopped and > hvm_domain_irq(d)->dpci is not set to NULL. > > Would it be possible to clean the already processed IRQs from the > domain pirq_tree? This might work, perhaps by way of invoking unmap_domain_pirq() right after pirq_guest_unbind(), as long as hvm_dirq_assist() (as called from dpci_softirq()) can be made skip all actual work it means to do in such a case. Unfortunately the two ->masked fields acted upon are different between __pirq_guest_unbind() and hvm_dirq_assist(). Jan
On 29.01.2020 12:47, Jan Beulich wrote: > On 29.01.2020 11:30, Roger Pau Monné wrote: >> Hello, >> >> Thanks for the patch! Next time could you please try to reply to the >> previous questions before sending a new version: >> >> https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg00257.html >> >> On Wed, Jan 29, 2020 at 10:28:07AM +0100, 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. >>> >>> Make __pirq_guest_unbind safe to be called multiple times by letting xen >>> continue if a shared pirq has already been unbound from this guest. The >>> PIRQ will be cleaned up from the domain's pirq_tree during the destruction >>> in complete_domain_destroy anyways. >> >> So AFAICT this is because pt_pirq_softirq_active() returns true in >> pci_clean_dpci_irq() and hence the iteration is stopped and >> hvm_domain_irq(d)->dpci is not set to NULL. >> >> Would it be possible to clean the already processed IRQs from the >> domain pirq_tree? > > This might work, perhaps by way of invoking unmap_domain_pirq() > right after pirq_guest_unbind(), as long as hvm_dirq_assist() (as > called from dpci_softirq()) can be made skip all actual work it > means to do in such a case. Unfortunately the two ->masked fields > acted upon are different between __pirq_guest_unbind() and > hvm_dirq_assist(). Ping? Unless I hear back soon, I'm afraid I'm going to drop this patch from my "pending" mail folder, as not being agreed whether to stick to the current version or whether to go this alternative route. A more "natural" approach to fixing the issue would be quite nice, after all. Jan
> -----Original Message----- > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich > Sent: 05 March 2020 09:37 > To: Gautam, Varad <vrd@amazon.de> > Cc: xen-devel@lists.xenproject.org; Andrew Cooper <andrew.cooper3@citrix.com>; Julien Grall > <julien@xen.org>; Roger Pau Monné <roger.pau@citrix.com> > Subject: Re: [Xen-devel] [PATCH v3] x86: irq: Do not BUG_ON multiple unbind calls for shared pirqs > > On 29.01.2020 12:47, Jan Beulich wrote: > > On 29.01.2020 11:30, Roger Pau Monné wrote: > >> Hello, > >> > >> Thanks for the patch! Next time could you please try to reply to the > >> previous questions before sending a new version: > >> > >> https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg00257.html > >> > >> On Wed, Jan 29, 2020 at 10:28:07AM +0100, 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. > >>> > >>> Make __pirq_guest_unbind safe to be called multiple times by letting xen > >>> continue if a shared pirq has already been unbound from this guest. The > >>> PIRQ will be cleaned up from the domain's pirq_tree during the destruction > >>> in complete_domain_destroy anyways. > >> > >> So AFAICT this is because pt_pirq_softirq_active() returns true in > >> pci_clean_dpci_irq() and hence the iteration is stopped and > >> hvm_domain_irq(d)->dpci is not set to NULL. > >> > >> Would it be possible to clean the already processed IRQs from the > >> domain pirq_tree? > > > > This might work, perhaps by way of invoking unmap_domain_pirq() > > right after pirq_guest_unbind(), as long as hvm_dirq_assist() (as > > called from dpci_softirq()) can be made skip all actual work it > > means to do in such a case. Unfortunately the two ->masked fields > > acted upon are different between __pirq_guest_unbind() and > > hvm_dirq_assist(). > > Ping? Unless I hear back soon, I'm afraid I'm going to drop this > patch from my "pending" mail folder, as not being agreed whether > to stick to the current version or whether to go this alternative > route. A more "natural" approach to fixing the issue would be > quite nice, after all. I'll try to pick this up tomorrow as I helped diagnose the problem being fixed. Paul > > Jan > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel
> -----Original Message----- > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Durrant, Paul > Sent: 05 March 2020 17:37 > To: Jan Beulich <jbeulich@suse.com>; Gautam, Varad <vrd@amazon.de> > Cc: xen-devel@lists.xenproject.org; Roger Pau Monné <roger.pau@citrix.com>; Julien Grall > <julien@xen.org>; Andrew Cooper <andrew.cooper3@citrix.com> > Subject: Re: [Xen-devel] [PATCH v3] x86: irq: Do not BUG_ON multiple unbind calls for shared pirqs > > > -----Original Message----- > > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich > > Sent: 05 March 2020 09:37 > > To: Gautam, Varad <vrd@amazon.de> > > Cc: xen-devel@lists.xenproject.org; Andrew Cooper <andrew.cooper3@citrix.com>; Julien Grall > > <julien@xen.org>; Roger Pau Monné <roger.pau@citrix.com> > > Subject: Re: [Xen-devel] [PATCH v3] x86: irq: Do not BUG_ON multiple unbind calls for shared pirqs > > > > On 29.01.2020 12:47, Jan Beulich wrote: > > > On 29.01.2020 11:30, Roger Pau Monné wrote: > > >> Hello, > > >> > > >> Thanks for the patch! Next time could you please try to reply to the > > >> previous questions before sending a new version: > > >> > > >> https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg00257.html > > >> > > >> On Wed, Jan 29, 2020 at 10:28:07AM +0100, 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. > > >>> > > >>> Make __pirq_guest_unbind safe to be called multiple times by letting xen > > >>> continue if a shared pirq has already been unbound from this guest. The > > >>> PIRQ will be cleaned up from the domain's pirq_tree during the destruction > > >>> in complete_domain_destroy anyways. > > >> > > >> So AFAICT this is because pt_pirq_softirq_active() returns true in > > >> pci_clean_dpci_irq() and hence the iteration is stopped and > > >> hvm_domain_irq(d)->dpci is not set to NULL. > > >> > > >> Would it be possible to clean the already processed IRQs from the > > >> domain pirq_tree? > > > > > > This might work, perhaps by way of invoking unmap_domain_pirq() > > > right after pirq_guest_unbind(), as long as hvm_dirq_assist() (as > > > called from dpci_softirq()) can be made skip all actual work it > > > means to do in such a case. Unfortunately the two ->masked fields > > > acted upon are different between __pirq_guest_unbind() and > > > hvm_dirq_assist(). > > > > Ping? Unless I hear back soon, I'm afraid I'm going to drop this > > patch from my "pending" mail folder, as not being agreed whether > > to stick to the current version or whether to go this alternative > > route. A more "natural" approach to fixing the issue would be > > quite nice, after all. > > I'll try to pick this up tomorrow as I helped diagnose the problem being fixed. > I'm looking at this now and I am finding the code very confusing, but I think we could achieve the cleanup by passing back the irq index out of __pirq_guest_unbind() such that pirq_guest_unbind() can call clean_domain_irq_pirq(). I still haven't got much of a clue as to how all the data structures hang together though. Paul
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c index 310ac00..4b172eb 100644 --- a/xen/arch/x86/irq.c +++ b/xen/arch/x86/irq.c @@ -1681,7 +1681,20 @@ 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 ) + { + ASSERT(action->nr_guests > 0); + /* + * 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 ) + return NULL; + BUG(); + } + memmove(&action->guest[i], &action->guest[i+1], (action->nr_guests-i-1) * sizeof(action->guest[0])); action->nr_guests--;
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. Make __pirq_guest_unbind safe to be called multiple times by letting xen continue if a shared pirq has already been unbound from this guest. The PIRQ will be cleaned up from the domain's pirq_tree during the destruction in complete_domain_destroy anyways. Signed-off-by: Varad Gautam <vrd@amazon.de> Reviewed-by: Paul Durrant <paul@xen.org> CC: Jan Beulich <jbeulich@suse.com> CC: Julien Grall <julien@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Andrew Cooper <andrew.cooper3@citrix.com> v2: Split the check on action->nr_guests > 0 and make it an ASSERT. v3: Style fixups. --- xen/arch/x86/irq.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)