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é <[email protected]>
CC: Boris Ostrovsky <[email protected]>
CC: Eduardo Valentin <[email protected]>
CC: Juergen Gross <[email protected]>
CC: Thomas Gleixner <[email protected]>
CC: "K. Y. Srinivasan" <[email protected]>
CC: Liu Shuo <[email protected]>
CC: Anoob Soman <[email protected]>
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
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 <[email protected]>
CC: <[email protected]>
CC: Roger Pau Monné <[email protected]>
CC: Boris Ostrovsky <[email protected]>
CC: Eduardo Valentin <[email protected]>
CC: Juergen Gross <[email protected]>
CC: Thomas Gleixner <[email protected]>
CC: "K. Y. Srinivasan" <[email protected]>
CC: Liu Shuo <[email protected]>
CC: Anoob Soman <[email protected]>
Signed-off-by: Amit Shah <[email protected]>
---
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
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 <[email protected]>
CC: <[email protected]>
CC: Roger Pau Monné <[email protected]>
CC: Boris Ostrovsky <[email protected]>
CC: Eduardo Valentin <[email protected]>
CC: Juergen Gross <[email protected]>
CC: Thomas Gleixner <[email protected]>
CC: "K. Y. Srinivasan" <[email protected]>
CC: Liu Shuo <[email protected]>
CC: Anoob Soman <[email protected]>
Signed-off-by: Amit Shah <[email protected]>
---
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
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 <[email protected]>
> CC: <[email protected]>
> CC: Roger Pau Monn? <[email protected]>
> CC: Boris Ostrovsky <[email protected]>
> CC: Eduardo Valentin <[email protected]>
> CC: Juergen Gross <[email protected]>
> CC: Thomas Gleixner <[email protected]>
> CC: "K. Y. Srinivasan" <[email protected]>
> CC: Liu Shuo <[email protected]>
> CC: Anoob Soman <[email protected]>
> Signed-off-by: Amit Shah <[email protected]>
> ---
> 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.
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 <[email protected]>
> CC: <[email protected]>
> CC: Roger Pau Monn? <[email protected]>
> CC: Boris Ostrovsky <[email protected]>
> CC: Eduardo Valentin <[email protected]>
> CC: Juergen Gross <[email protected]>
> CC: Thomas Gleixner <[email protected]>
> CC: "K. Y. Srinivasan" <[email protected]>
> CC: Liu Shuo <[email protected]>
> CC: Anoob Soman <[email protected]>
> Signed-off-by: Amit Shah <[email protected]>
> ---
> 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.
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 <[email protected]>
> > CC: <[email protected]>
> > CC: Roger Pau Monné <[email protected]>
> > CC: Boris Ostrovsky <[email protected]>
> > CC: Eduardo Valentin <[email protected]>
> > CC: Juergen Gross <[email protected]>
> > CC: Thomas Gleixner <[email protected]>
> > CC: "K. Y. Srinivasan" <[email protected]>
> > CC: Liu Shuo <[email protected]>
> > CC: Anoob Soman <[email protected]>
> > Signed-off-by: Amit Shah <[email protected]>
> > ---
> > 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
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 <[email protected]>
> > > CC: <[email protected]>
> > > CC: Roger Pau Monn? <[email protected]>
> > > CC: Boris Ostrovsky <[email protected]>
> > > CC: Eduardo Valentin <[email protected]>
> > > CC: Juergen Gross <[email protected]>
> > > CC: Thomas Gleixner <[email protected]>
> > > CC: "K. Y. Srinivasan" <[email protected]>
> > > CC: Liu Shuo <[email protected]>
> > > CC: Anoob Soman <[email protected]>
> > > Signed-off-by: Amit Shah <[email protected]>
> > > ---
> > > ?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.
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 <[email protected]>
> > > > CC: <[email protected]>
> > > > CC: Roger Pau Monné <[email protected]>
> > > > CC: Boris Ostrovsky <[email protected]>
> > > > CC: Eduardo Valentin <[email protected]>
> > > > CC: Juergen Gross <[email protected]>
> > > > CC: Thomas Gleixner <[email protected]>
> > > > CC: "K. Y. Srinivasan" <[email protected]>
> > > > CC: Liu Shuo <[email protected]>
> > > > CC: Anoob Soman <[email protected]>
> > > > Signed-off-by: Amit Shah <[email protected]>
> > > > ---
> > > > 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
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 <[email protected]>
>>>>> CC: <[email protected]>
>>>>> CC: Roger Pau Monné <[email protected]>
>>>>> CC: Boris Ostrovsky <[email protected]>
>>>>> CC: Eduardo Valentin <[email protected]>
>>>>> CC: Juergen Gross <[email protected]>
>>>>> CC: Thomas Gleixner <[email protected]>
>>>>> CC: "K. Y. Srinivasan" <[email protected]>
>>>>> CC: Liu Shuo <[email protected]>
>>>>> CC: Anoob Soman <[email protected]>
>>>>> Signed-off-by: Amit Shah <[email protected]>
>>>>> ---
>>>>> 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