[Qemu-devel] [PATCH] spapr_pci: Fix interrupt leak in rtas_ibm_change_msi() error path

Greg Kurz posted 1 patch 28 weeks ago
Test docker-mingw@fedora passed
Test asan passed
Test docker-clang@ubuntu passed
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/154956051753.218871.1505640778712413716.stgit@bahia.lab.toulouse-stg.fr.ibm.com
Maintainers: David Gibson <david@gibson.dropbear.id.au>
hw/ppc/spapr_pci.c |    6 ++++++
1 file changed, 6 insertions(+)

[Qemu-devel] [PATCH] spapr_pci: Fix interrupt leak in rtas_ibm_change_msi() error path

Posted by Greg Kurz 28 weeks ago
Now that IRQ allocation has been split in two (first allocate IRQ numbers,
then claim them), if the claiming fails, we must release the IRQs.

Fixes: 4fe75a8ccd80 "spapr: split the IRQ allocation sequence"
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_pci.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 41d81f4a8500..6fe3c10c8d4c 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -393,6 +393,12 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     for (i = 0; i < req_num; i++) {
         spapr_irq_claim(spapr, irq + i, false, &err);
         if (err) {
+            if (i) {
+                spapr_irq_free(spapr, irq, i + 1);
+            }
+            if (!smc->legacy_irq_allocation) {
+                spapr_irq_msi_free(spapr, irq, req_num);
+            }
             error_reportf_err(err, "Can't allocate MSIs for device %x: ",
                               config_addr);
             rtas_st(rets, 0, RTAS_OUT_HW_ERROR);


Re: [Qemu-devel] [PATCH] spapr_pci: Fix interrupt leak in rtas_ibm_change_msi() error path

Posted by David Gibson 28 weeks ago
On Thu, Feb 07, 2019 at 06:28:37PM +0100, Greg Kurz wrote:
> Now that IRQ allocation has been split in two (first allocate IRQ numbers,
> then claim them), if the claiming fails, we must release the IRQs.
> 
> Fixes: 4fe75a8ccd80 "spapr: split the IRQ allocation sequence"
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-4.0, thanks.

> ---
>  hw/ppc/spapr_pci.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 41d81f4a8500..6fe3c10c8d4c 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -393,6 +393,12 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      for (i = 0; i < req_num; i++) {
>          spapr_irq_claim(spapr, irq + i, false, &err);
>          if (err) {
> +            if (i) {
> +                spapr_irq_free(spapr, irq, i + 1);
> +            }
> +            if (!smc->legacy_irq_allocation) {
> +                spapr_irq_msi_free(spapr, irq, req_num);
> +            }
>              error_reportf_err(err, "Can't allocate MSIs for device %x: ",
>                                config_addr);
>              rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [Qemu-ppc] [PATCH] spapr_pci: Fix interrupt leak in rtas_ibm_change_msi() error path

Posted by Greg Kurz 28 weeks ago
On Fri, 8 Feb 2019 10:06:47 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Feb 07, 2019 at 06:28:37PM +0100, Greg Kurz wrote:
> > Now that IRQ allocation has been split in two (first allocate IRQ numbers,
> > then claim them), if the claiming fails, we must release the IRQs.
> > 
> > Fixes: 4fe75a8ccd80 "spapr: split the IRQ allocation sequence"
> > Signed-off-by: Greg Kurz <groug@kaod.org>  
> 
> Applied to ppc-for-4.0, thanks.
> 

Oops I've just realized there's an off-by-one error...

> > ---
> >  hw/ppc/spapr_pci.c |    6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 41d81f4a8500..6fe3c10c8d4c 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -393,6 +393,12 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >      for (i = 0; i < req_num; i++) {
> >          spapr_irq_claim(spapr, irq + i, false, &err);
> >          if (err) {
> > +            if (i) {
> > +                spapr_irq_free(spapr, irq, i + 1);

... here. It should actually be:

+                spapr_irq_free(spapr, irq, i);

Can you fix this in your tree or should I post a v2 ?

> > +            }
> > +            if (!smc->legacy_irq_allocation) {
> > +                spapr_irq_msi_free(spapr, irq, req_num);
> > +            }
> >              error_reportf_err(err, "Can't allocate MSIs for device %x: ",
> >                                config_addr);
> >              rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> >   
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH] spapr_pci: Fix interrupt leak in rtas_ibm_change_msi() error path

Posted by David Gibson 28 weeks ago
On Fri, Feb 08, 2019 at 12:15:03PM +0100, Greg Kurz wrote:
> On Fri, 8 Feb 2019 10:06:47 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Feb 07, 2019 at 06:28:37PM +0100, Greg Kurz wrote:
> > > Now that IRQ allocation has been split in two (first allocate IRQ numbers,
> > > then claim them), if the claiming fails, we must release the IRQs.
> > > 
> > > Fixes: 4fe75a8ccd80 "spapr: split the IRQ allocation sequence"
> > > Signed-off-by: Greg Kurz <groug@kaod.org>  
> > 
> > Applied to ppc-for-4.0, thanks.
> > 
> 
> Oops I've just realized there's an off-by-one error...
> 
> > > ---
> > >  hw/ppc/spapr_pci.c |    6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > > index 41d81f4a8500..6fe3c10c8d4c 100644
> > > --- a/hw/ppc/spapr_pci.c
> > > +++ b/hw/ppc/spapr_pci.c
> > > @@ -393,6 +393,12 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > >      for (i = 0; i < req_num; i++) {
> > >          spapr_irq_claim(spapr, irq + i, false, &err);
> > >          if (err) {
> > > +            if (i) {
> > > +                spapr_irq_free(spapr, irq, i + 1);
> 
> ... here. It should actually be:
> 
> +                spapr_irq_free(spapr, irq, i);
> 
> Can you fix this in your tree or should I post a v2 ?

Fixed up inline.

> 
> > > +            }
> > > +            if (!smc->legacy_irq_allocation) {
> > > +                spapr_irq_msi_free(spapr, irq, req_num);
> > > +            }
> > >              error_reportf_err(err, "Can't allocate MSIs for device %x: ",
> > >                                config_addr);
> > >              rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> > >   
> > 
> 



-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [Qemu-ppc] [PATCH] spapr_pci: Fix interrupt leak in rtas_ibm_change_msi() error path

Posted by Greg Kurz 28 weeks ago
On Fri, 8 Feb 2019 22:33:36 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Feb 08, 2019 at 12:15:03PM +0100, Greg Kurz wrote:
> > On Fri, 8 Feb 2019 10:06:47 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Thu, Feb 07, 2019 at 06:28:37PM +0100, Greg Kurz wrote:  
> > > > Now that IRQ allocation has been split in two (first allocate IRQ numbers,
> > > > then claim them), if the claiming fails, we must release the IRQs.
> > > > 
> > > > Fixes: 4fe75a8ccd80 "spapr: split the IRQ allocation sequence"
> > > > Signed-off-by: Greg Kurz <groug@kaod.org>    
> > > 
> > > Applied to ppc-for-4.0, thanks.
> > >   
> > 
> > Oops I've just realized there's an off-by-one error...
> >   
> > > > ---
> > > >  hw/ppc/spapr_pci.c |    6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > > > index 41d81f4a8500..6fe3c10c8d4c 100644
> > > > --- a/hw/ppc/spapr_pci.c
> > > > +++ b/hw/ppc/spapr_pci.c
> > > > @@ -393,6 +393,12 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > > >      for (i = 0; i < req_num; i++) {
> > > >          spapr_irq_claim(spapr, irq + i, false, &err);
> > > >          if (err) {
> > > > +            if (i) {
> > > > +                spapr_irq_free(spapr, irq, i + 1);  
> > 
> > ... here. It should actually be:
> > 
> > +                spapr_irq_free(spapr, irq, i);
> > 
> > Can you fix this in your tree or should I post a v2 ?  
> 
> Fixed up inline.
> 

Thanks !

> >   
> > > > +            }
> > > > +            if (!smc->legacy_irq_allocation) {
> > > > +                spapr_irq_msi_free(spapr, irq, req_num);
> > > > +            }
> > > >              error_reportf_err(err, "Can't allocate MSIs for device %x: ",
> > > >                                config_addr);
> > > >              rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> > > >     
> > >   
> >   
> 
> 
>