On Wed, 2018-02-28 at 09:19 +0000, Roger Pau Monne wrote:
From: Roger Pau Monne roger.pau@citrix.com
[ Upstream commit 910f8befdf5bccf25287d9f1743e3e546bcb7ce0 ]
Current cleanup in the error path of xen_bind_pirq_msi_to_irq is wrong. First of all there's an off-by-one in the cleanup loop, which can lead to unbinding wrong IRQs.
Secondly IRQs not bound won't be freed, thus leaking IRQ numbers.
Note that there's no need to differentiate between bound and unbound IRQs when freeing them, __unbind_from_irq will deal with both of them correctly.
It appears to me that it is safe to call __unbind_from_irq() after xen_irq_info_common_setup() fails, but *not* if the latter hasn't been called at all. In that case the IRQ type will still be set to IRQT_UNBOUND and this will trigger the BUG_ON() in __unbind_from_irq().
[...]
--- a/drivers/xen/events/events_base.c +++ b/drivers/xen/events/events_base.c @@ -764,8 +764,8 @@ out: mutex_unlock(&irq_mapping_update_lock); return irq; error_irq:
- for (; i >= 0; i--)
__unbind_from_irq(irq + i);
- while (nvec--)
__unbind_from_irq(irq + nvec);
If nvec > 1, and xen_irq_info_pirq_setup() fails for i != nvec - 1, then we reach here without having called xen_irq_info_common_setup() for all these IRQs.
In that case, I think we will still want to call xen_free_irq() for all IRQs. So maybe the fix would be to remove the BUG_ON() in __unbind_from_irq()?
Ben.
mutex_unlock(&irq_mapping_update_lock); return ret; }
On Wed, Jun 13, 2018 at 07:48:50PM +0100, Ben Hutchings wrote:
On Wed, 2018-02-28 at 09:19 +0000, Roger Pau Monne wrote:
From: Roger Pau Monne roger.pau@citrix.com
[ Upstream commit 910f8befdf5bccf25287d9f1743e3e546bcb7ce0 ]
Current cleanup in the error path of xen_bind_pirq_msi_to_irq is wrong. First of all there's an off-by-one in the cleanup loop, which can lead to unbinding wrong IRQs.
Secondly IRQs not bound won't be freed, thus leaking IRQ numbers.
Note that there's no need to differentiate between bound and unbound IRQs when freeing them, __unbind_from_irq will deal with both of them correctly.
It appears to me that it is safe to call __unbind_from_irq() after xen_irq_info_common_setup() fails, but *not* if the latter hasn't been called at all. In that case the IRQ type will still be set to IRQT_UNBOUND and this will trigger the BUG_ON() in __unbind_from_irq().
[...]
--- a/drivers/xen/events/events_base.c +++ b/drivers/xen/events/events_base.c @@ -764,8 +764,8 @@ out: mutex_unlock(&irq_mapping_update_lock); return irq; error_irq:
- for (; i >= 0; i--)
__unbind_from_irq(irq + i);
- while (nvec--)
__unbind_from_irq(irq + nvec);If nvec > 1, and xen_irq_info_pirq_setup() fails for i != nvec - 1, then we reach here without having called xen_irq_info_common_setup() for all these IRQs.
In that case, I think we will still want to call xen_free_irq() for all IRQs. So maybe the fix would be to remove the BUG_ON() in __unbind_from_irq()?
I think your analysis is right, and I agree that removing the BUG_ON from __unbind_from_irq seems like the right solution.
I can't see any issues from calling xen_free_irq with type == IRQT_UNBOUND, but I've already attempted to fix this once and failed, so I would like to get second opinions. Also I'm not sure of the reason behind that BUG_ON.
Roger.
On 06/14/2018 04:21 AM, Roger Pau Monné wrote:
On Wed, Jun 13, 2018 at 07:48:50PM +0100, Ben Hutchings wrote:
On Wed, 2018-02-28 at 09:19 +0000, Roger Pau Monne wrote:
From: Roger Pau Monne roger.pau@citrix.com
[ Upstream commit 910f8befdf5bccf25287d9f1743e3e546bcb7ce0 ]
Current cleanup in the error path of xen_bind_pirq_msi_to_irq is wrong. First of all there's an off-by-one in the cleanup loop, which can lead to unbinding wrong IRQs.
Secondly IRQs not bound won't be freed, thus leaking IRQ numbers.
Note that there's no need to differentiate between bound and unbound IRQs when freeing them, __unbind_from_irq will deal with both of them correctly.
It appears to me that it is safe to call __unbind_from_irq() after xen_irq_info_common_setup() fails, but *not* if the latter hasn't been called at all. In that case the IRQ type will still be set to IRQT_UNBOUND and this will trigger the BUG_ON() in __unbind_from_irq().
[...]
--- a/drivers/xen/events/events_base.c +++ b/drivers/xen/events/events_base.c @@ -764,8 +764,8 @@ out: mutex_unlock(&irq_mapping_update_lock); return irq; error_irq:
- for (; i >= 0; i--)
__unbind_from_irq(irq + i);
- while (nvec--)
__unbind_from_irq(irq + nvec);If nvec > 1, and xen_irq_info_pirq_setup() fails for i != nvec - 1, then we reach here without having called xen_irq_info_common_setup() for all these IRQs.
In that case, I think we will still want to call xen_free_irq() for all IRQs. So maybe the fix would be to remove the BUG_ON() in __unbind_from_irq()?
I think your analysis is right, and I agree that removing the BUG_ON from __unbind_from_irq seems like the right solution.
I can't see any issues from calling xen_free_irq with type == IRQT_UNBOUND, but I've already attempted to fix this once and failed, so I would like to get second opinions. Also I'm not sure of the reason behind that BUG_ON.
I don't see a reason for the BUG_ON either.
-boris
linux-stable-mirror@lists.linaro.org
-
Ben Hutchings -
Boris Ostrovsky -
Roger Pau Monné