* [PATCH v2 0/2] xen: fix bugs in error conditions
@ 2018-02-27 15:55 Amit Shah
2018-02-27 15:55 ` [PATCH v2 1/2] xen: fix out-of-bounds irq unbind for MSI message groups Amit Shah
` (3 more replies)
0 siblings, 4 replies; 20+ messages in thread
From: Amit Shah @ 2018-02-27 15:55 UTC (permalink / raw)
To: xen-devel
Cc: linux-kernel, Amit Shah, Roger Pau Monné,
Boris Ostrovsky, Eduardo Valentin, Juergen Gross,
Thomas Gleixner, K. Y. Srinivasan, Liu Shuo, Anoob Soman
Hello,
These bugs were found during code review. Details in the commits.
Please review and apply.
v2:
- fix up patch 2 properly (Roger Pau Monné)
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Eduardo Valentin <eduval@amazon.com>
CC: Juergen Gross <jgross@suse.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: "K. Y. Srinivasan" <kys@microsoft.com>
CC: Liu Shuo <shuo.a.liu@intel.com>
CC: Anoob Soman <anoob.soman@citrix.com>
Amit Shah (2):
xen: fix out-of-bounds irq unbind for MSI message groups
xen: events: free irqs in error condition
drivers/xen/events/events_base.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
--
2.7.3.AMZN
Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH v2 1/2] xen: fix out-of-bounds irq unbind for MSI message groups 2018-02-27 15:55 [PATCH v2 0/2] xen: fix bugs in error conditions Amit Shah @ 2018-02-27 15:55 ` Amit Shah 2018-02-27 16:58 ` Roger Pau Monné 2018-02-27 16:58 ` Roger Pau Monné 2018-02-27 15:55 ` Amit Shah ` (2 subsequent siblings) 3 siblings, 2 replies; 20+ messages in thread From: Amit Shah @ 2018-02-27 15:55 UTC (permalink / raw) To: xen-devel Cc: linux-kernel, Amit Shah, stable, Roger Pau Monné, Boris Ostrovsky, Eduardo Valentin, Juergen Gross, Thomas Gleixner, K. Y. Srinivasan, Liu Shuo, Anoob Soman When an MSI descriptor was not available, the error path would try to unbind an irq that was never acquired - potentially unbinding an unrelated irq. Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message groups") Reported-by: Hooman Mirhadi <mirhadih@amazon.com> CC: <stable@vger.kernel.org> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com> CC: Eduardo Valentin <eduval@amazon.com> CC: Juergen Gross <jgross@suse.com> CC: Thomas Gleixner <tglx@linutronix.de> CC: "K. Y. Srinivasan" <kys@microsoft.com> CC: Liu Shuo <shuo.a.liu@intel.com> CC: Anoob Soman <anoob.soman@citrix.com> Signed-off-by: Amit Shah <aams@amazon.com> --- drivers/xen/events/events_base.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c index 1ab4bd1..c86d10e 100644 --- a/drivers/xen/events/events_base.c +++ b/drivers/xen/events/events_base.c @@ -755,8 +755,10 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc, mutex_unlock(&irq_mapping_update_lock); return irq; error_irq: - for (; i >= 0; i--) + while (i > 0) { + i--; __unbind_from_irq(irq + i); + } mutex_unlock(&irq_mapping_update_lock); return ret; } -- 2.7.3.AMZN Amazon Development Center Germany GmbH Berlin - Dresden - Aachen main office: Krausenstr. 38, 10117 Berlin Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger Ust-ID: DE289237879 Eingetragen am Amtsgericht Charlottenburg HRB 149173 B ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/2] xen: fix out-of-bounds irq unbind for MSI message groups 2018-02-27 15:55 ` [PATCH v2 1/2] xen: fix out-of-bounds irq unbind for MSI message groups Amit Shah @ 2018-02-27 16:58 ` Roger Pau Monné 2018-02-27 16:58 ` Roger Pau Monné 1 sibling, 0 replies; 20+ messages in thread From: Roger Pau Monné @ 2018-02-27 16:58 UTC (permalink / raw) To: Amit Shah Cc: Juergen Gross, Eduardo Valentin, linux-kernel, stable, Liu Shuo, xen-devel, Boris Ostrovsky, K. Y. Srinivasan, Thomas Gleixner, Anoob Soman On Tue, Feb 27, 2018 at 03:55:57PM +0000, Amit Shah wrote: > When an MSI descriptor was not available, the error path would try to > unbind an irq that was never acquired - potentially unbinding an > unrelated irq. Those IRQs have been allocated in the xen_allocate_irqs_dynamic call, so I think the "potentially unbinding an unrelated irq" part is wrong. The unbind call would be performed against an unbound IRQ, which is harmless AFAICT. > Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message groups") > Reported-by: Hooman Mirhadi <mirhadih@amazon.com> > CC: <stable@vger.kernel.org> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Boris Ostrovsky <boris.ostrovsky@oracle.com> > CC: Eduardo Valentin <eduval@amazon.com> > CC: Juergen Gross <jgross@suse.com> > CC: Thomas Gleixner <tglx@linutronix.de> > CC: "K. Y. Srinivasan" <kys@microsoft.com> > CC: Liu Shuo <shuo.a.liu@intel.com> > CC: Anoob Soman <anoob.soman@citrix.com> > Signed-off-by: Amit Shah <aams@amazon.com> > --- > drivers/xen/events/events_base.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c > index 1ab4bd1..c86d10e 100644 > --- a/drivers/xen/events/events_base.c > +++ b/drivers/xen/events/events_base.c > @@ -755,8 +755,10 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc, > mutex_unlock(&irq_mapping_update_lock); > return irq; > error_irq: > - for (; i >= 0; i--) > + while (i > 0) { while (i--) __unbind_from_irq(irq + i); Although please see reply to patch 2. Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/2] xen: fix out-of-bounds irq unbind for MSI message groups 2018-02-27 15:55 ` [PATCH v2 1/2] xen: fix out-of-bounds irq unbind for MSI message groups Amit Shah @ 2018-02-27 16:58 ` Roger Pau Monné 2018-02-27 16:58 ` Roger Pau Monné 1 sibling, 0 replies; 20+ messages in thread From: Roger Pau Monné @ 2018-02-27 16:58 UTC (permalink / raw) To: Amit Shah Cc: xen-devel, linux-kernel, stable, Boris Ostrovsky, Eduardo Valentin, Juergen Gross, Thomas Gleixner, K. Y. Srinivasan, Liu Shuo, Anoob Soman On Tue, Feb 27, 2018 at 03:55:57PM +0000, Amit Shah wrote: > When an MSI descriptor was not available, the error path would try to > unbind an irq that was never acquired - potentially unbinding an > unrelated irq. Those IRQs have been allocated in the xen_allocate_irqs_dynamic call, so I think the "potentially unbinding an unrelated irq" part is wrong. The unbind call would be performed against an unbound IRQ, which is harmless AFAICT. > Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message groups") > Reported-by: Hooman Mirhadi <mirhadih@amazon.com> > CC: <stable@vger.kernel.org> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Boris Ostrovsky <boris.ostrovsky@oracle.com> > CC: Eduardo Valentin <eduval@amazon.com> > CC: Juergen Gross <jgross@suse.com> > CC: Thomas Gleixner <tglx@linutronix.de> > CC: "K. Y. Srinivasan" <kys@microsoft.com> > CC: Liu Shuo <shuo.a.liu@intel.com> > CC: Anoob Soman <anoob.soman@citrix.com> > Signed-off-by: Amit Shah <aams@amazon.com> > --- > drivers/xen/events/events_base.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c > index 1ab4bd1..c86d10e 100644 > --- a/drivers/xen/events/events_base.c > +++ b/drivers/xen/events/events_base.c > @@ -755,8 +755,10 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc, > mutex_unlock(&irq_mapping_update_lock); > return irq; > error_irq: > - for (; i >= 0; i--) > + while (i > 0) { while (i--) __unbind_from_irq(irq + i); Although please see reply to patch 2. Roger. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/2] xen: fix out-of-bounds irq unbind for MSI message groups @ 2018-02-27 16:58 ` Roger Pau Monné 0 siblings, 0 replies; 20+ messages in thread From: Roger Pau Monné @ 2018-02-27 16:58 UTC (permalink / raw) To: Amit Shah Cc: xen-devel, linux-kernel, stable, Boris Ostrovsky, Eduardo Valentin, Juergen Gross, Thomas Gleixner, K. Y. Srinivasan, Liu Shuo, Anoob Soman On Tue, Feb 27, 2018 at 03:55:57PM +0000, Amit Shah wrote: > When an MSI descriptor was not available, the error path would try to > unbind an irq that was never acquired - potentially unbinding an > unrelated irq. Those IRQs have been allocated in the xen_allocate_irqs_dynamic call, so I think the "potentially unbinding an unrelated irq" part is wrong. The unbind call would be performed against an unbound IRQ, which is harmless AFAICT. > Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message groups") > Reported-by: Hooman Mirhadi <mirhadih@amazon.com> > CC: <stable@vger.kernel.org> > CC: Roger Pau Monn� <roger.pau@citrix.com> > CC: Boris Ostrovsky <boris.ostrovsky@oracle.com> > CC: Eduardo Valentin <eduval@amazon.com> > CC: Juergen Gross <jgross@suse.com> > CC: Thomas Gleixner <tglx@linutronix.de> > CC: "K. Y. Srinivasan" <kys@microsoft.com> > CC: Liu Shuo <shuo.a.liu@intel.com> > CC: Anoob Soman <anoob.soman@citrix.com> > Signed-off-by: Amit Shah <aams@amazon.com> > --- > drivers/xen/events/events_base.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c > index 1ab4bd1..c86d10e 100644 > --- a/drivers/xen/events/events_base.c > +++ b/drivers/xen/events/events_base.c > @@ -755,8 +755,10 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc, > mutex_unlock(&irq_mapping_update_lock); > return irq; > error_irq: > - for (; i >= 0; i--) > + while (i > 0) { while (i--) __unbind_from_irq(irq + i); Although please see reply to patch 2. Roger. ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 1/2] xen: fix out-of-bounds irq unbind for MSI message groups 2018-02-27 15:55 [PATCH v2 0/2] xen: fix bugs in error conditions Amit Shah 2018-02-27 15:55 ` [PATCH v2 1/2] xen: fix out-of-bounds irq unbind for MSI message groups Amit Shah @ 2018-02-27 15:55 ` Amit Shah 2018-02-27 15:55 ` [PATCH v2 2/2] xen: events: free irqs in error condition Amit Shah 2018-02-27 15:55 ` Amit Shah 3 siblings, 0 replies; 20+ messages in thread From: Amit Shah @ 2018-02-27 15:55 UTC (permalink / raw) To: xen-devel Cc: Juergen Gross, Eduardo Valentin, linux-kernel, stable, Liu Shuo, Boris Ostrovsky, K. Y. Srinivasan, Amit Shah, Thomas Gleixner, Anoob Soman, Roger Pau Monné When an MSI descriptor was not available, the error path would try to unbind an irq that was never acquired - potentially unbinding an unrelated irq. Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message groups") Reported-by: Hooman Mirhadi <mirhadih@amazon.com> CC: <stable@vger.kernel.org> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com> CC: Eduardo Valentin <eduval@amazon.com> CC: Juergen Gross <jgross@suse.com> CC: Thomas Gleixner <tglx@linutronix.de> CC: "K. Y. Srinivasan" <kys@microsoft.com> CC: Liu Shuo <shuo.a.liu@intel.com> CC: Anoob Soman <anoob.soman@citrix.com> Signed-off-by: Amit Shah <aams@amazon.com> --- drivers/xen/events/events_base.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c index 1ab4bd1..c86d10e 100644 --- a/drivers/xen/events/events_base.c +++ b/drivers/xen/events/events_base.c @@ -755,8 +755,10 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc, mutex_unlock(&irq_mapping_update_lock); return irq; error_irq: - for (; i >= 0; i--) + while (i > 0) { + i--; __unbind_from_irq(irq + i); + } mutex_unlock(&irq_mapping_update_lock); return ret; } -- 2.7.3.AMZN Amazon Development Center Germany GmbH Berlin - Dresden - Aachen main office: Krausenstr. 38, 10117 Berlin Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger Ust-ID: DE289237879 Eingetragen am Amtsgericht Charlottenburg HRB 149173 B _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 2/2] xen: events: free irqs in error condition 2018-02-27 15:55 [PATCH v2 0/2] xen: fix bugs in error conditions Amit Shah 2018-02-27 15:55 ` [PATCH v2 1/2] xen: fix out-of-bounds irq unbind for MSI message groups Amit Shah 2018-02-27 15:55 ` Amit Shah @ 2018-02-27 15:55 ` Amit Shah 2018-02-27 17:07 ` Roger Pau Monné 2018-02-27 17:07 ` Roger Pau Monné 2018-02-27 15:55 ` Amit Shah 3 siblings, 2 replies; 20+ messages in thread From: Amit Shah @ 2018-02-27 15:55 UTC (permalink / raw) To: xen-devel Cc: linux-kernel, Amit Shah, stable, Roger Pau Monné, Boris Ostrovsky, Eduardo Valentin, Juergen Gross, Thomas Gleixner, K. Y. Srinivasan, Liu Shuo, Anoob Soman In case of errors in irq setup for MSI, free up the allocated irqs. Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message groups") Reported-by: Hooman Mirhadi <mirhadih@amazon.com> CC: <stable@vger.kernel.org> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com> CC: Eduardo Valentin <eduval@amazon.com> CC: Juergen Gross <jgross@suse.com> CC: Thomas Gleixner <tglx@linutronix.de> CC: "K. Y. Srinivasan" <kys@microsoft.com> CC: Liu Shuo <shuo.a.liu@intel.com> CC: Anoob Soman <anoob.soman@citrix.com> Signed-off-by: Amit Shah <aams@amazon.com> --- drivers/xen/events/events_base.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c index c86d10e..a299586 100644 --- a/drivers/xen/events/events_base.c +++ b/drivers/xen/events/events_base.c @@ -750,11 +750,14 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc, ret = irq_set_msi_desc(irq, msidesc); if (ret < 0) - goto error_irq; + goto error_desc; out: mutex_unlock(&irq_mapping_update_lock); return irq; error_irq: + while (--nvec >= i) + xen_free_irq(irq + nvec); +error_desc: while (i > 0) { i--; __unbind_from_irq(irq + i); -- 2.7.3.AMZN Amazon Development Center Germany GmbH Berlin - Dresden - Aachen main office: Krausenstr. 38, 10117 Berlin Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger Ust-ID: DE289237879 Eingetragen am Amtsgericht Charlottenburg HRB 149173 B ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] xen: events: free irqs in error condition 2018-02-27 15:55 ` [PATCH v2 2/2] xen: events: free irqs in error condition Amit Shah @ 2018-02-27 17:07 ` Roger Pau Monné 2018-02-27 17:07 ` Roger Pau Monné 1 sibling, 0 replies; 20+ messages in thread From: Roger Pau Monné @ 2018-02-27 17:07 UTC (permalink / raw) To: Amit Shah Cc: Juergen Gross, Eduardo Valentin, linux-kernel, stable, Liu Shuo, xen-devel, Boris Ostrovsky, K. Y. Srinivasan, Thomas Gleixner, Anoob Soman On Tue, Feb 27, 2018 at 03:55:58PM +0000, Amit Shah wrote: > In case of errors in irq setup for MSI, free up the allocated irqs. > > Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message groups") > Reported-by: Hooman Mirhadi <mirhadih@amazon.com> > CC: <stable@vger.kernel.org> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Boris Ostrovsky <boris.ostrovsky@oracle.com> > CC: Eduardo Valentin <eduval@amazon.com> > CC: Juergen Gross <jgross@suse.com> > CC: Thomas Gleixner <tglx@linutronix.de> > CC: "K. Y. Srinivasan" <kys@microsoft.com> > CC: Liu Shuo <shuo.a.liu@intel.com> > CC: Anoob Soman <anoob.soman@citrix.com> > Signed-off-by: Amit Shah <aams@amazon.com> > --- > drivers/xen/events/events_base.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c > index c86d10e..a299586 100644 > --- a/drivers/xen/events/events_base.c > +++ b/drivers/xen/events/events_base.c > @@ -750,11 +750,14 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc, > > ret = irq_set_msi_desc(irq, msidesc); > if (ret < 0) > - goto error_irq; > + goto error_desc; > out: > mutex_unlock(&irq_mapping_update_lock); > return irq; > error_irq: > + while (--nvec >= i) > + xen_free_irq(irq + nvec); > +error_desc: > while (i > 0) { > i--; > __unbind_from_irq(irq + i); It seems pointless to introduce another label and another loop to fix something that can be fixed with a single label and a single loop, this just makes the code more complex for no reason. IMHO the way to solve this issue is: while (nvec--) { if (nvec >= i) xen_free_irq(irq + nvec); else __unbind_from_irq(irq + nvec); } Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] xen: events: free irqs in error condition 2018-02-27 15:55 ` [PATCH v2 2/2] xen: events: free irqs in error condition Amit Shah @ 2018-02-27 17:07 ` Roger Pau Monné 2018-02-27 17:07 ` Roger Pau Monné 1 sibling, 0 replies; 20+ messages in thread From: Roger Pau Monné @ 2018-02-27 17:07 UTC (permalink / raw) To: Amit Shah Cc: xen-devel, linux-kernel, stable, Boris Ostrovsky, Eduardo Valentin, Juergen Gross, Thomas Gleixner, K. Y. Srinivasan, Liu Shuo, Anoob Soman On Tue, Feb 27, 2018 at 03:55:58PM +0000, Amit Shah wrote: > In case of errors in irq setup for MSI, free up the allocated irqs. > > Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message groups") > Reported-by: Hooman Mirhadi <mirhadih@amazon.com> > CC: <stable@vger.kernel.org> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Boris Ostrovsky <boris.ostrovsky@oracle.com> > CC: Eduardo Valentin <eduval@amazon.com> > CC: Juergen Gross <jgross@suse.com> > CC: Thomas Gleixner <tglx@linutronix.de> > CC: "K. Y. Srinivasan" <kys@microsoft.com> > CC: Liu Shuo <shuo.a.liu@intel.com> > CC: Anoob Soman <anoob.soman@citrix.com> > Signed-off-by: Amit Shah <aams@amazon.com> > --- > drivers/xen/events/events_base.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c > index c86d10e..a299586 100644 > --- a/drivers/xen/events/events_base.c > +++ b/drivers/xen/events/events_base.c > @@ -750,11 +750,14 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc, > > ret = irq_set_msi_desc(irq, msidesc); > if (ret < 0) > - goto error_irq; > + goto error_desc; > out: > mutex_unlock(&irq_mapping_update_lock); > return irq; > error_irq: > + while (--nvec >= i) > + xen_free_irq(irq + nvec); > +error_desc: > while (i > 0) { > i--; > __unbind_from_irq(irq + i); It seems pointless to introduce another label and another loop to fix something that can be fixed with a single label and a single loop, this just makes the code more complex for no reason. IMHO the way to solve this issue is: while (nvec--) { if (nvec >= i) xen_free_irq(irq + nvec); else __unbind_from_irq(irq + nvec); } Roger. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] xen: events: free irqs in error condition @ 2018-02-27 17:07 ` Roger Pau Monné 0 siblings, 0 replies; 20+ messages in thread From: Roger Pau Monné @ 2018-02-27 17:07 UTC (permalink / raw) To: Amit Shah Cc: xen-devel, linux-kernel, stable, Boris Ostrovsky, Eduardo Valentin, Juergen Gross, Thomas Gleixner, K. Y. Srinivasan, Liu Shuo, Anoob Soman On Tue, Feb 27, 2018 at 03:55:58PM +0000, Amit Shah wrote: > In case of errors in irq setup for MSI, free up the allocated irqs. > > Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message groups") > Reported-by: Hooman Mirhadi <mirhadih@amazon.com> > CC: <stable@vger.kernel.org> > CC: Roger Pau Monn� <roger.pau@citrix.com> > CC: Boris Ostrovsky <boris.ostrovsky@oracle.com> > CC: Eduardo Valentin <eduval@amazon.com> > CC: Juergen Gross <jgross@suse.com> > CC: Thomas Gleixner <tglx@linutronix.de> > CC: "K. Y. Srinivasan" <kys@microsoft.com> > CC: Liu Shuo <shuo.a.liu@intel.com> > CC: Anoob Soman <anoob.soman@citrix.com> > Signed-off-by: Amit Shah <aams@amazon.com> > --- > drivers/xen/events/events_base.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c > index c86d10e..a299586 100644 > --- a/drivers/xen/events/events_base.c > +++ b/drivers/xen/events/events_base.c > @@ -750,11 +750,14 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc, > > ret = irq_set_msi_desc(irq, msidesc); > if (ret < 0) > - goto error_irq; > + goto error_desc; > out: > mutex_unlock(&irq_mapping_update_lock); > return irq; > error_irq: > + while (--nvec >= i) > + xen_free_irq(irq + nvec); > +error_desc: > while (i > 0) { > i--; > __unbind_from_irq(irq + i); It seems pointless to introduce another label and another loop to fix something that can be fixed with a single label and a single loop, this just makes the code more complex for no reason. IMHO the way to solve this issue is: while (nvec--) { if (nvec >= i) xen_free_irq(irq + nvec); else __unbind_from_irq(irq + nvec); } Roger. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] xen: events: free irqs in error condition 2018-02-27 17:07 ` Roger Pau Monné (?) @ 2018-02-27 17:32 ` Shah, Amit -1 siblings, 0 replies; 20+ messages in thread From: Shah, Amit @ 2018-02-27 17:32 UTC (permalink / raw) To: roger.pau Cc: jgross, xen-devel, anoob.soman, linux-kernel, stable, shuo.a.liu, Valentin, Eduardo, boris.ostrovsky, kys, tglx On Di, 2018-02-27 at 17:07 +0000, Roger Pau Monné wrote: > On Tue, Feb 27, 2018 at 03:55:58PM +0000, Amit Shah wrote: > > > > In case of errors in irq setup for MSI, free up the allocated irqs. > > > > Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message groups") > > Reported-by: Hooman Mirhadi <mirhadih@amazon.com> > > CC: <stable@vger.kernel.org> > > CC: Roger Pau Monné <roger.pau@citrix.com> > > CC: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > CC: Eduardo Valentin <eduval@amazon.com> > > CC: Juergen Gross <jgross@suse.com> > > CC: Thomas Gleixner <tglx@linutronix.de> > > CC: "K. Y. Srinivasan" <kys@microsoft.com> > > CC: Liu Shuo <shuo.a.liu@intel.com> > > CC: Anoob Soman <anoob.soman@citrix.com> > > Signed-off-by: Amit Shah <aams@amazon.com> > > --- > > drivers/xen/events/events_base.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/xen/events/events_base.c > > b/drivers/xen/events/events_base.c > > index c86d10e..a299586 100644 > > --- a/drivers/xen/events/events_base.c > > +++ b/drivers/xen/events/events_base.c > > @@ -750,11 +750,14 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev > > *dev, struct msi_desc *msidesc, > > > > ret = irq_set_msi_desc(irq, msidesc); > > if (ret < 0) > > - goto error_irq; > > + goto error_desc; > > out: > > mutex_unlock(&irq_mapping_update_lock); > > return irq; > > error_irq: > > + while (--nvec >= i) > > + xen_free_irq(irq + nvec); > > +error_desc: > > while (i > 0) { > > i--; > > __unbind_from_irq(irq + i); > It seems pointless to introduce another label and another loop to fix > something that can be fixed with a single label and a single loop, > this just makes the code more complex for no reason. I disagree, just because there are two different cleanups to be made for two different issues; it's not as if the if.. and else conditions are going to be interleaved. Anyway it's a matter of taste. Since you've already proposed the patch, would you mind baking a proper one and posting it? Thanks! > IMHO the way to solve this issue is: > > while (nvec--) { > if (nvec >= i) > xen_free_irq(irq + nvec); > else > __unbind_from_irq(irq + nvec); > } Amit Amazon Development Center Germany GmbH Berlin - Dresden - Aachen main office: Krausenstr. 38, 10117 Berlin Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger Ust-ID: DE289237879 Eingetragen am Amtsgericht Charlottenburg HRB 149173 B _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] xen: events: free irqs in error condition 2018-02-27 17:07 ` Roger Pau Monné (?) (?) @ 2018-02-27 17:32 ` Shah, Amit 2018-02-28 8:16 ` Roger Pau Monné 2018-02-28 8:16 ` Roger Pau Monné -1 siblings, 2 replies; 20+ messages in thread From: Shah, Amit @ 2018-02-27 17:32 UTC (permalink / raw) To: roger.pau Cc: boris.ostrovsky, linux-kernel, tglx, kys, Valentin, Eduardo, jgross, stable, shuo.a.liu, anoob.soman, xen-devel On Di, 2018-02-27 at 17:07 +0000, Roger Pau Monné wrote: > On Tue, Feb 27, 2018 at 03:55:58PM +0000, Amit Shah wrote: > > > > In case of errors in irq setup for MSI, free up the allocated irqs. > > > > Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message groups") > > Reported-by: Hooman Mirhadi <mirhadih@amazon.com> > > CC: <stable@vger.kernel.org> > > CC: Roger Pau Monné <roger.pau@citrix.com> > > CC: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > CC: Eduardo Valentin <eduval@amazon.com> > > CC: Juergen Gross <jgross@suse.com> > > CC: Thomas Gleixner <tglx@linutronix.de> > > CC: "K. Y. Srinivasan" <kys@microsoft.com> > > CC: Liu Shuo <shuo.a.liu@intel.com> > > CC: Anoob Soman <anoob.soman@citrix.com> > > Signed-off-by: Amit Shah <aams@amazon.com> > > --- > > drivers/xen/events/events_base.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/xen/events/events_base.c > > b/drivers/xen/events/events_base.c > > index c86d10e..a299586 100644 > > --- a/drivers/xen/events/events_base.c > > +++ b/drivers/xen/events/events_base.c > > @@ -750,11 +750,14 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev > > *dev, struct msi_desc *msidesc, > > > > ret = irq_set_msi_desc(irq, msidesc); > > if (ret < 0) > > - goto error_irq; > > + goto error_desc; > > out: > > mutex_unlock(&irq_mapping_update_lock); > > return irq; > > error_irq: > > + while (--nvec >= i) > > + xen_free_irq(irq + nvec); > > +error_desc: > > while (i > 0) { > > i--; > > __unbind_from_irq(irq + i); > It seems pointless to introduce another label and another loop to fix > something that can be fixed with a single label and a single loop, > this just makes the code more complex for no reason. I disagree, just because there are two different cleanups to be made for two different issues; it's not as if the if.. and else conditions are going to be interleaved. Anyway it's a matter of taste. Since you've already proposed the patch, would you mind baking a proper one and posting it? Thanks! > IMHO the way to solve this issue is: > > while (nvec--) { > if (nvec >= i) > xen_free_irq(irq + nvec); > else > __unbind_from_irq(irq + nvec); > } Amit Amazon Development Center Germany GmbH Berlin - Dresden - Aachen main office: Krausenstr. 38, 10117 Berlin Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger Ust-ID: DE289237879 Eingetragen am Amtsgericht Charlottenburg HRB 149173 B ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] xen: events: free irqs in error condition 2018-02-27 17:32 ` Shah, Amit @ 2018-02-28 8:16 ` Roger Pau Monné 2018-02-28 8:16 ` Roger Pau Monné 1 sibling, 0 replies; 20+ messages in thread From: Roger Pau Monné @ 2018-02-28 8:16 UTC (permalink / raw) To: Shah, Amit Cc: boris.ostrovsky, linux-kernel, tglx, kys, Valentin, Eduardo, jgross, stable, shuo.a.liu, anoob.soman, xen-devel On Tue, Feb 27, 2018 at 05:32:53PM +0000, Shah, Amit wrote: > > On Di, 2018-02-27 at 17:07 +0000, Roger Pau Monné wrote: > > On Tue, Feb 27, 2018 at 03:55:58PM +0000, Amit Shah wrote: > > > > > > In case of errors in irq setup for MSI, free up the allocated irqs. > > > > > > Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message groups") > > > Reported-by: Hooman Mirhadi <mirhadih@amazon.com> > > > CC: <stable@vger.kernel.org> > > > CC: Roger Pau Monné <roger.pau@citrix.com> > > > CC: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > > CC: Eduardo Valentin <eduval@amazon.com> > > > CC: Juergen Gross <jgross@suse.com> > > > CC: Thomas Gleixner <tglx@linutronix.de> > > > CC: "K. Y. Srinivasan" <kys@microsoft.com> > > > CC: Liu Shuo <shuo.a.liu@intel.com> > > > CC: Anoob Soman <anoob.soman@citrix.com> > > > Signed-off-by: Amit Shah <aams@amazon.com> > > > --- > > > drivers/xen/events/events_base.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/xen/events/events_base.c > > > b/drivers/xen/events/events_base.c > > > index c86d10e..a299586 100644 > > > --- a/drivers/xen/events/events_base.c > > > +++ b/drivers/xen/events/events_base.c > > > @@ -750,11 +750,14 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev > > > *dev, struct msi_desc *msidesc, > > > > > > ret = irq_set_msi_desc(irq, msidesc); > > > if (ret < 0) > > > - goto error_irq; > > > + goto error_desc; > > > out: > > > mutex_unlock(&irq_mapping_update_lock); > > > return irq; > > > error_irq: > > > + while (--nvec >= i) > > > + xen_free_irq(irq + nvec); > > > +error_desc: > > > while (i > 0) { > > > i--; > > > __unbind_from_irq(irq + i); > > It seems pointless to introduce another label and another loop to fix > > something that can be fixed with a single label and a single loop, > > this just makes the code more complex for no reason. > > I disagree, just because there are two different cleanups to be made > for two different issues; it's not as if the if.. and else conditions > are going to be interleaved. Oh, I don't mind so much whether it ends up being two patches or a single one, but IMHO the code should end up looking similar to what I proposed, I would like to avoid having two loops and two labels. Could you rework the series so that the end result uses a single loop (and label)? Thanks, Roger. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] xen: events: free irqs in error condition @ 2018-02-28 8:16 ` Roger Pau Monné 0 siblings, 0 replies; 20+ messages in thread From: Roger Pau Monné @ 2018-02-28 8:16 UTC (permalink / raw) To: Shah, Amit Cc: boris.ostrovsky, linux-kernel, tglx, kys, Valentin, Eduardo, jgross, stable, shuo.a.liu, anoob.soman, xen-devel On Tue, Feb 27, 2018 at 05:32:53PM +0000, Shah, Amit wrote: > > On Di, 2018-02-27 at 17:07 +0000, Roger Pau Monn� wrote: > > On Tue, Feb 27, 2018 at 03:55:58PM +0000, Amit Shah wrote: > > > > > > In case of errors in irq setup for MSI, free up the allocated irqs. > > > > > > Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message groups") > > > Reported-by: Hooman Mirhadi <mirhadih@amazon.com> > > > CC: <stable@vger.kernel.org> > > > CC: Roger Pau Monn� <roger.pau@citrix.com> > > > CC: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > > CC: Eduardo Valentin <eduval@amazon.com> > > > CC: Juergen Gross <jgross@suse.com> > > > CC: Thomas Gleixner <tglx@linutronix.de> > > > CC: "K. Y. Srinivasan" <kys@microsoft.com> > > > CC: Liu Shuo <shuo.a.liu@intel.com> > > > CC: Anoob Soman <anoob.soman@citrix.com> > > > Signed-off-by: Amit Shah <aams@amazon.com> > > > --- > > > �drivers/xen/events/events_base.c | 5 ++++- > > > �1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/xen/events/events_base.c > > > b/drivers/xen/events/events_base.c > > > index c86d10e..a299586 100644 > > > --- a/drivers/xen/events/events_base.c > > > +++ b/drivers/xen/events/events_base.c > > > @@ -750,11 +750,14 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev > > > *dev, struct msi_desc *msidesc, > > > � > > > � ret = irq_set_msi_desc(irq, msidesc); > > > � if (ret < 0) > > > - goto error_irq; > > > + goto error_desc; > > > �out: > > > � mutex_unlock(&irq_mapping_update_lock); > > > � return irq; > > > �error_irq: > > > + while (--nvec >= i) > > > + xen_free_irq(irq + nvec); > > > +error_desc: > > > � while (i > 0) { > > > � i--; > > > � __unbind_from_irq(irq + i); > > It seems pointless to introduce another label and another loop to fix > > something that can be fixed with a single label and a single loop, > > this just makes the code more complex for no reason. > > I disagree, just because there are two different cleanups to be made > for two different issues; it's not as if the if.. and else conditions > are going to be interleaved. Oh, I don't mind so much whether it ends up being two patches or a single one, but IMHO the code should end up looking similar to what I proposed, I would like to avoid having two loops and two labels. Could you rework the series so that the end result uses a single loop (and label)? Thanks, Roger. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] xen: events: free irqs in error condition 2018-02-28 8:16 ` Roger Pau Monné (?) @ 2018-02-28 8:25 ` Shah, Amit -1 siblings, 0 replies; 20+ messages in thread From: Shah, Amit @ 2018-02-28 8:25 UTC (permalink / raw) To: roger.pau Cc: jgross, xen-devel, anoob.soman, linux-kernel, stable, shuo.a.liu, Valentin, Eduardo, boris.ostrovsky, kys, tglx On Mi, 2018-02-28 at 08:16 +0000, Roger Pau Monné wrote: > On Tue, Feb 27, 2018 at 05:32:53PM +0000, Shah, Amit wrote: > > > > > > On Di, 2018-02-27 at 17:07 +0000, Roger Pau Monné wrote: > > > > > > On Tue, Feb 27, 2018 at 03:55:58PM +0000, Amit Shah wrote: > > > > > > > > > > > > In case of errors in irq setup for MSI, free up the allocated > > > > irqs. > > > > > > > > Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message > > > > groups") > > > > Reported-by: Hooman Mirhadi <mirhadih@amazon.com> > > > > CC: <stable@vger.kernel.org> > > > > CC: Roger Pau Monné <roger.pau@citrix.com> > > > > CC: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > > > CC: Eduardo Valentin <eduval@amazon.com> > > > > CC: Juergen Gross <jgross@suse.com> > > > > CC: Thomas Gleixner <tglx@linutronix.de> > > > > CC: "K. Y. Srinivasan" <kys@microsoft.com> > > > > CC: Liu Shuo <shuo.a.liu@intel.com> > > > > CC: Anoob Soman <anoob.soman@citrix.com> > > > > Signed-off-by: Amit Shah <aams@amazon.com> > > > > --- > > > > drivers/xen/events/events_base.c | 5 ++++- > > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/xen/events/events_base.c > > > > b/drivers/xen/events/events_base.c > > > > index c86d10e..a299586 100644 > > > > --- a/drivers/xen/events/events_base.c > > > > +++ b/drivers/xen/events/events_base.c > > > > @@ -750,11 +750,14 @@ int xen_bind_pirq_msi_to_irq(struct > > > > pci_dev > > > > *dev, struct msi_desc *msidesc, > > > > > > > > ret = irq_set_msi_desc(irq, msidesc); > > > > if (ret < 0) > > > > - goto error_irq; > > > > + goto error_desc; > > > > out: > > > > mutex_unlock(&irq_mapping_update_lock); > > > > return irq; > > > > error_irq: > > > > + while (--nvec >= i) > > > > + xen_free_irq(irq + nvec); > > > > +error_desc: > > > > while (i > 0) { > > > > i--; > > > > __unbind_from_irq(irq + i); > > > It seems pointless to introduce another label and another loop to > > > fix > > > something that can be fixed with a single label and a single > > > loop, > > > this just makes the code more complex for no reason. > > I disagree, just because there are two different cleanups to be > > made > > for two different issues; it's not as if the if.. and else > > conditions > > are going to be interleaved. > Oh, I don't mind so much whether it ends up being two patches or a > single one, but IMHO the code should end up looking similar to what I > proposed, I would like to avoid having two loops and two labels. > > Could you rework the series so that the end result uses a single loop > (and label)? That was the part I didn't like much, so it would be better if the patch came from you :) Amit Amazon Development Center Germany GmbH Berlin - Dresden - Aachen main office: Krausenstr. 38, 10117 Berlin Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger Ust-ID: DE289237879 Eingetragen am Amtsgericht Charlottenburg HRB 149173 B _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] xen: events: free irqs in error condition 2018-02-28 8:16 ` Roger Pau Monné (?) (?) @ 2018-02-28 8:25 ` Shah, Amit 2018-02-28 8:27 ` Juergen Gross 2018-02-28 8:27 ` Juergen Gross -1 siblings, 2 replies; 20+ messages in thread From: Shah, Amit @ 2018-02-28 8:25 UTC (permalink / raw) To: roger.pau Cc: boris.ostrovsky, linux-kernel, tglx, kys, jgross, stable, shuo.a.liu, anoob.soman, Valentin, Eduardo, xen-devel On Mi, 2018-02-28 at 08:16 +0000, Roger Pau Monné wrote: > On Tue, Feb 27, 2018 at 05:32:53PM +0000, Shah, Amit wrote: > > > > > > On Di, 2018-02-27 at 17:07 +0000, Roger Pau Monné wrote: > > > > > > On Tue, Feb 27, 2018 at 03:55:58PM +0000, Amit Shah wrote: > > > > > > > > > > > > In case of errors in irq setup for MSI, free up the allocated > > > > irqs. > > > > > > > > Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message > > > > groups") > > > > Reported-by: Hooman Mirhadi <mirhadih@amazon.com> > > > > CC: <stable@vger.kernel.org> > > > > CC: Roger Pau Monné <roger.pau@citrix.com> > > > > CC: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > > > CC: Eduardo Valentin <eduval@amazon.com> > > > > CC: Juergen Gross <jgross@suse.com> > > > > CC: Thomas Gleixner <tglx@linutronix.de> > > > > CC: "K. Y. Srinivasan" <kys@microsoft.com> > > > > CC: Liu Shuo <shuo.a.liu@intel.com> > > > > CC: Anoob Soman <anoob.soman@citrix.com> > > > > Signed-off-by: Amit Shah <aams@amazon.com> > > > > --- > > > > drivers/xen/events/events_base.c | 5 ++++- > > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/xen/events/events_base.c > > > > b/drivers/xen/events/events_base.c > > > > index c86d10e..a299586 100644 > > > > --- a/drivers/xen/events/events_base.c > > > > +++ b/drivers/xen/events/events_base.c > > > > @@ -750,11 +750,14 @@ int xen_bind_pirq_msi_to_irq(struct > > > > pci_dev > > > > *dev, struct msi_desc *msidesc, > > > > > > > > ret = irq_set_msi_desc(irq, msidesc); > > > > if (ret < 0) > > > > - goto error_irq; > > > > + goto error_desc; > > > > out: > > > > mutex_unlock(&irq_mapping_update_lock); > > > > return irq; > > > > error_irq: > > > > + while (--nvec >= i) > > > > + xen_free_irq(irq + nvec); > > > > +error_desc: > > > > while (i > 0) { > > > > i--; > > > > __unbind_from_irq(irq + i); > > > It seems pointless to introduce another label and another loop to > > > fix > > > something that can be fixed with a single label and a single > > > loop, > > > this just makes the code more complex for no reason. > > I disagree, just because there are two different cleanups to be > > made > > for two different issues; it's not as if the if.. and else > > conditions > > are going to be interleaved. > Oh, I don't mind so much whether it ends up being two patches or a > single one, but IMHO the code should end up looking similar to what I > proposed, I would like to avoid having two loops and two labels. > > Could you rework the series so that the end result uses a single loop > (and label)? That was the part I didn't like much, so it would be better if the patch came from you :) Amit Amazon Development Center Germany GmbH Berlin - Dresden - Aachen main office: Krausenstr. 38, 10117 Berlin Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger Ust-ID: DE289237879 Eingetragen am Amtsgericht Charlottenburg HRB 149173 B ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] xen: events: free irqs in error condition 2018-02-28 8:25 ` Shah, Amit @ 2018-02-28 8:27 ` Juergen Gross 2018-02-28 8:27 ` Juergen Gross 1 sibling, 0 replies; 20+ messages in thread From: Juergen Gross @ 2018-02-28 8:27 UTC (permalink / raw) To: Shah, Amit, roger.pau Cc: boris.ostrovsky, linux-kernel, tglx, kys, stable, shuo.a.liu, anoob.soman, Valentin, Eduardo, xen-devel On 28/02/18 09:25, Shah, Amit wrote: > > On Mi, 2018-02-28 at 08:16 +0000, Roger Pau Monné wrote: >> On Tue, Feb 27, 2018 at 05:32:53PM +0000, Shah, Amit wrote: >>> >>> >>> On Di, 2018-02-27 at 17:07 +0000, Roger Pau Monné wrote: >>>> >>>> On Tue, Feb 27, 2018 at 03:55:58PM +0000, Amit Shah wrote: >>>>> >>>>> >>>>> In case of errors in irq setup for MSI, free up the allocated >>>>> irqs. >>>>> >>>>> Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message >>>>> groups") >>>>> Reported-by: Hooman Mirhadi <mirhadih@amazon.com> >>>>> CC: <stable@vger.kernel.org> >>>>> CC: Roger Pau Monné <roger.pau@citrix.com> >>>>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com> >>>>> CC: Eduardo Valentin <eduval@amazon.com> >>>>> CC: Juergen Gross <jgross@suse.com> >>>>> CC: Thomas Gleixner <tglx@linutronix.de> >>>>> CC: "K. Y. Srinivasan" <kys@microsoft.com> >>>>> CC: Liu Shuo <shuo.a.liu@intel.com> >>>>> CC: Anoob Soman <anoob.soman@citrix.com> >>>>> Signed-off-by: Amit Shah <aams@amazon.com> >>>>> --- >>>>> drivers/xen/events/events_base.c | 5 ++++- >>>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/xen/events/events_base.c >>>>> b/drivers/xen/events/events_base.c >>>>> index c86d10e..a299586 100644 >>>>> --- a/drivers/xen/events/events_base.c >>>>> +++ b/drivers/xen/events/events_base.c >>>>> @@ -750,11 +750,14 @@ int xen_bind_pirq_msi_to_irq(struct >>>>> pci_dev >>>>> *dev, struct msi_desc *msidesc, >>>>> >>>>> ret = irq_set_msi_desc(irq, msidesc); >>>>> if (ret < 0) >>>>> - goto error_irq; >>>>> + goto error_desc; >>>>> out: >>>>> mutex_unlock(&irq_mapping_update_lock); >>>>> return irq; >>>>> error_irq: >>>>> + while (--nvec >= i) >>>>> + xen_free_irq(irq + nvec); >>>>> +error_desc: >>>>> while (i > 0) { >>>>> i--; >>>>> __unbind_from_irq(irq + i); >>>> It seems pointless to introduce another label and another loop to >>>> fix >>>> something that can be fixed with a single label and a single >>>> loop, >>>> this just makes the code more complex for no reason. >>> I disagree, just because there are two different cleanups to be >>> made >>> for two different issues; it's not as if the if.. and else >>> conditions >>> are going to be interleaved. >> Oh, I don't mind so much whether it ends up being two patches or a >> single one, but IMHO the code should end up looking similar to what I >> proposed, I would like to avoid having two loops and two labels. >> >> Could you rework the series so that the end result uses a single loop >> (and label)? > > That was the part I didn't like much, so it would be better if the > patch came from you :) I'd prefer Roger's solution, too. Roger, in case you don't want to write the patch, I can do it. Juergen ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] xen: events: free irqs in error condition 2018-02-28 8:25 ` Shah, Amit 2018-02-28 8:27 ` Juergen Gross @ 2018-02-28 8:27 ` Juergen Gross 1 sibling, 0 replies; 20+ messages in thread From: Juergen Gross @ 2018-02-28 8:27 UTC (permalink / raw) To: Shah, Amit, roger.pau Cc: xen-devel, anoob.soman, linux-kernel, stable, shuo.a.liu, Valentin, Eduardo, boris.ostrovsky, kys, tglx On 28/02/18 09:25, Shah, Amit wrote: > > On Mi, 2018-02-28 at 08:16 +0000, Roger Pau Monné wrote: >> On Tue, Feb 27, 2018 at 05:32:53PM +0000, Shah, Amit wrote: >>> >>> >>> On Di, 2018-02-27 at 17:07 +0000, Roger Pau Monné wrote: >>>> >>>> On Tue, Feb 27, 2018 at 03:55:58PM +0000, Amit Shah wrote: >>>>> >>>>> >>>>> In case of errors in irq setup for MSI, free up the allocated >>>>> irqs. >>>>> >>>>> Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message >>>>> groups") >>>>> Reported-by: Hooman Mirhadi <mirhadih@amazon.com> >>>>> CC: <stable@vger.kernel.org> >>>>> CC: Roger Pau Monné <roger.pau@citrix.com> >>>>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com> >>>>> CC: Eduardo Valentin <eduval@amazon.com> >>>>> CC: Juergen Gross <jgross@suse.com> >>>>> CC: Thomas Gleixner <tglx@linutronix.de> >>>>> CC: "K. Y. Srinivasan" <kys@microsoft.com> >>>>> CC: Liu Shuo <shuo.a.liu@intel.com> >>>>> CC: Anoob Soman <anoob.soman@citrix.com> >>>>> Signed-off-by: Amit Shah <aams@amazon.com> >>>>> --- >>>>> drivers/xen/events/events_base.c | 5 ++++- >>>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/xen/events/events_base.c >>>>> b/drivers/xen/events/events_base.c >>>>> index c86d10e..a299586 100644 >>>>> --- a/drivers/xen/events/events_base.c >>>>> +++ b/drivers/xen/events/events_base.c >>>>> @@ -750,11 +750,14 @@ int xen_bind_pirq_msi_to_irq(struct >>>>> pci_dev >>>>> *dev, struct msi_desc *msidesc, >>>>> >>>>> ret = irq_set_msi_desc(irq, msidesc); >>>>> if (ret < 0) >>>>> - goto error_irq; >>>>> + goto error_desc; >>>>> out: >>>>> mutex_unlock(&irq_mapping_update_lock); >>>>> return irq; >>>>> error_irq: >>>>> + while (--nvec >= i) >>>>> + xen_free_irq(irq + nvec); >>>>> +error_desc: >>>>> while (i > 0) { >>>>> i--; >>>>> __unbind_from_irq(irq + i); >>>> It seems pointless to introduce another label and another loop to >>>> fix >>>> something that can be fixed with a single label and a single >>>> loop, >>>> this just makes the code more complex for no reason. >>> I disagree, just because there are two different cleanups to be >>> made >>> for two different issues; it's not as if the if.. and else >>> conditions >>> are going to be interleaved. >> Oh, I don't mind so much whether it ends up being two patches or a >> single one, but IMHO the code should end up looking similar to what I >> proposed, I would like to avoid having two loops and two labels. >> >> Could you rework the series so that the end result uses a single loop >> (and label)? > > That was the part I didn't like much, so it would be better if the > patch came from you :) I'd prefer Roger's solution, too. Roger, in case you don't want to write the patch, I can do it. Juergen _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] xen: events: free irqs in error condition 2018-02-27 17:32 ` Shah, Amit 2018-02-28 8:16 ` Roger Pau Monné @ 2018-02-28 8:16 ` Roger Pau Monné 1 sibling, 0 replies; 20+ messages in thread From: Roger Pau Monné @ 2018-02-28 8:16 UTC (permalink / raw) To: Shah, Amit Cc: jgross, xen-devel, anoob.soman, linux-kernel, stable, shuo.a.liu, Valentin, Eduardo, boris.ostrovsky, kys, tglx On Tue, Feb 27, 2018 at 05:32:53PM +0000, Shah, Amit wrote: > > On Di, 2018-02-27 at 17:07 +0000, Roger Pau Monné wrote: > > On Tue, Feb 27, 2018 at 03:55:58PM +0000, Amit Shah wrote: > > > > > > In case of errors in irq setup for MSI, free up the allocated irqs. > > > > > > Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message groups") > > > Reported-by: Hooman Mirhadi <mirhadih@amazon.com> > > > CC: <stable@vger.kernel.org> > > > CC: Roger Pau Monné <roger.pau@citrix.com> > > > CC: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > > CC: Eduardo Valentin <eduval@amazon.com> > > > CC: Juergen Gross <jgross@suse.com> > > > CC: Thomas Gleixner <tglx@linutronix.de> > > > CC: "K. Y. Srinivasan" <kys@microsoft.com> > > > CC: Liu Shuo <shuo.a.liu@intel.com> > > > CC: Anoob Soman <anoob.soman@citrix.com> > > > Signed-off-by: Amit Shah <aams@amazon.com> > > > --- > > > drivers/xen/events/events_base.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/xen/events/events_base.c > > > b/drivers/xen/events/events_base.c > > > index c86d10e..a299586 100644 > > > --- a/drivers/xen/events/events_base.c > > > +++ b/drivers/xen/events/events_base.c > > > @@ -750,11 +750,14 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev > > > *dev, struct msi_desc *msidesc, > > > > > > ret = irq_set_msi_desc(irq, msidesc); > > > if (ret < 0) > > > - goto error_irq; > > > + goto error_desc; > > > out: > > > mutex_unlock(&irq_mapping_update_lock); > > > return irq; > > > error_irq: > > > + while (--nvec >= i) > > > + xen_free_irq(irq + nvec); > > > +error_desc: > > > while (i > 0) { > > > i--; > > > __unbind_from_irq(irq + i); > > It seems pointless to introduce another label and another loop to fix > > something that can be fixed with a single label and a single loop, > > this just makes the code more complex for no reason. > > I disagree, just because there are two different cleanups to be made > for two different issues; it's not as if the if.. and else conditions > are going to be interleaved. Oh, I don't mind so much whether it ends up being two patches or a single one, but IMHO the code should end up looking similar to what I proposed, I would like to avoid having two loops and two labels. Could you rework the series so that the end result uses a single loop (and label)? Thanks, Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 2/2] xen: events: free irqs in error condition 2018-02-27 15:55 [PATCH v2 0/2] xen: fix bugs in error conditions Amit Shah ` (2 preceding siblings ...) 2018-02-27 15:55 ` [PATCH v2 2/2] xen: events: free irqs in error condition Amit Shah @ 2018-02-27 15:55 ` Amit Shah 3 siblings, 0 replies; 20+ messages in thread From: Amit Shah @ 2018-02-27 15:55 UTC (permalink / raw) To: xen-devel Cc: Juergen Gross, Eduardo Valentin, linux-kernel, stable, Liu Shuo, Boris Ostrovsky, K. Y. Srinivasan, Amit Shah, Thomas Gleixner, Anoob Soman, Roger Pau Monné In case of errors in irq setup for MSI, free up the allocated irqs. Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message groups") Reported-by: Hooman Mirhadi <mirhadih@amazon.com> CC: <stable@vger.kernel.org> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com> CC: Eduardo Valentin <eduval@amazon.com> CC: Juergen Gross <jgross@suse.com> CC: Thomas Gleixner <tglx@linutronix.de> CC: "K. Y. Srinivasan" <kys@microsoft.com> CC: Liu Shuo <shuo.a.liu@intel.com> CC: Anoob Soman <anoob.soman@citrix.com> Signed-off-by: Amit Shah <aams@amazon.com> --- drivers/xen/events/events_base.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c index c86d10e..a299586 100644 --- a/drivers/xen/events/events_base.c +++ b/drivers/xen/events/events_base.c @@ -750,11 +750,14 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc, ret = irq_set_msi_desc(irq, msidesc); if (ret < 0) - goto error_irq; + goto error_desc; out: mutex_unlock(&irq_mapping_update_lock); return irq; error_irq: + while (--nvec >= i) + xen_free_irq(irq + nvec); +error_desc: while (i > 0) { i--; __unbind_from_irq(irq + i); -- 2.7.3.AMZN Amazon Development Center Germany GmbH Berlin - Dresden - Aachen main office: Krausenstr. 38, 10117 Berlin Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger Ust-ID: DE289237879 Eingetragen am Amtsgericht Charlottenburg HRB 149173 B _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 20+ messages in thread
end of thread, other threads:[~2018-02-28 8:27 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-27 15:55 [PATCH v2 0/2] xen: fix bugs in error conditions Amit Shah 2018-02-27 15:55 ` [PATCH v2 1/2] xen: fix out-of-bounds irq unbind for MSI message groups Amit Shah 2018-02-27 16:58 ` Roger Pau Monné 2018-02-27 16:58 ` Roger Pau Monné 2018-02-27 16:58 ` Roger Pau Monné 2018-02-27 15:55 ` Amit Shah 2018-02-27 15:55 ` [PATCH v2 2/2] xen: events: free irqs in error condition Amit Shah 2018-02-27 17:07 ` Roger Pau Monné 2018-02-27 17:07 ` Roger Pau Monné 2018-02-27 17:07 ` Roger Pau Monné 2018-02-27 17:32 ` Shah, Amit 2018-02-27 17:32 ` Shah, Amit 2018-02-28 8:16 ` Roger Pau Monné 2018-02-28 8:16 ` Roger Pau Monné 2018-02-28 8:25 ` Shah, Amit 2018-02-28 8:25 ` Shah, Amit 2018-02-28 8:27 ` Juergen Gross 2018-02-28 8:27 ` Juergen Gross 2018-02-28 8:16 ` Roger Pau Monné 2018-02-27 15:55 ` Amit Shah
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.