| Message ID | 20230420024037.5921-3-decui@microsoft.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | pci-hyper: Fix race condition bugs for fast device hotplug | expand |
On Wed, Apr 19, 2023 at 07:40:33PM -0700, Dexuan Cui wrote: > When the host tries to remove a PCI device, the host first sends a > PCI_EJECT message to the guest, and the guest is supposed to gracefully > remove the PCI device and send a PCI_EJECTION_COMPLETE message to the host; > the host then sends a VMBus message CHANNELMSG_RESCIND_CHANNELOFFER to > the guest (when the guest receives this message, the device is already > unassigned from the guest) and the guest can do some final cleanup work; > if the guest fails to respond to the PCI_EJECT message within one minute, > the host sends the VMBus message CHANNELMSG_RESCIND_CHANNELOFFER and > removes the PCI device forcibly. > > In the case of fast device addition/removal, it's possible that the PCI > device driver is still configuring MSI-X interrupts when the guest receives > the PCI_EJECT message; the channel callback calls hv_pci_eject_device(), > which sets hpdev->state to hv_pcichild_ejecting, and schedules a work > hv_eject_device_work(); if the PCI device driver is calling > pci_alloc_irq_vectors() -> ... -> hv_compose_msi_msg(), we can break the > while loop in hv_compose_msi_msg() due to the updated hpdev->state, and > leave data->chip_data with its default value of NULL; later, when the PCI > device driver calls request_irq() -> ... -> hv_irq_unmask(), the guest > crashes in hv_arch_irq_unmask() due to data->chip_data being NULL. > > Fix the issue by not testing hpdev->state in the while loop: when the > guest receives PCI_EJECT, the device is still assigned to the guest, and > the guest has one minute to finish the device removal gracefully. We don't > really need to (and we should not) test hpdev->state in the loop. > > Fixes: de0aa7b2f97d ("PCI: hv: Fix 2 hang issues in hv_compose_msi_msg()") > Signed-off-by: Dexuan Cui <decui@microsoft.com> > Reviewed-by: Michael Kelley <mikelley@microsoft.com> > Cc: stable@vger.kernel.org > --- > > v2: > Removed the "debug code". > No change to the patch body. > Added Cc:stable > > v3: > Added Michael's Reviewed-by. > > drivers/pci/controller/pci-hyperv.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c > index b82c7cde19e66..1b11cf7391933 100644 > --- a/drivers/pci/controller/pci-hyperv.c > +++ b/drivers/pci/controller/pci-hyperv.c > @@ -643,6 +643,11 @@ static void hv_arch_irq_unmask(struct irq_data *data) > pbus = pdev->bus; > hbus = container_of(pbus->sysdata, struct hv_pcibus_device, sysdata); > int_desc = data->chip_data; > + if (!int_desc) { > + dev_warn(&hbus->hdev->device, "%s() can not unmask irq %u\n", > + __func__, data->irq); > + return; > + } That's a check that should be there regardless ? > spin_lock_irqsave(&hbus->retarget_msi_interrupt_lock, flags); > > @@ -1911,12 +1916,6 @@ static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg) > hv_pci_onchannelcallback(hbus); > spin_unlock_irqrestore(&channel->sched_lock, flags); > > - if (hpdev->state == hv_pcichild_ejecting) { > - dev_err_once(&hbus->hdev->device, > - "the device is being ejected\n"); > - goto enable_tasklet; > - } > - > udelay(100); > } I don't understand why this code is in hv_compose_msi_msg() in the first place (and why only in that function ?) to me this looks like you are adding plasters in the code that can turn out to be problematic while ejecting a device, this does not seem robust at all - that's my opinion. Feel free to merge this code, I can't ACK it, sorry. Lorenzo
> From: Lorenzo Pieralisi <lpieralisi@kernel.org> > Sent: Thursday, May 25, 2023 3:16 AM > > --- a/drivers/pci/controller/pci-hyperv.c > > +++ b/drivers/pci/controller/pci-hyperv.c > > @@ -643,6 +643,11 @@ static void hv_arch_irq_unmask(struct irq_data > > *data) > > pbus = pdev->bus; > > hbus = container_of(pbus->sysdata, struct hv_pcibus_device, sysdata); > > int_desc = data->chip_data; > > + if (!int_desc) { > > + dev_warn(&hbus->hdev->device, "%s() can not unmask irq %u\n", > > + __func__, data->irq); > > + return; > > + } > > That's a check that should be there regardless ? Yes. Normally data->chip_data is set at the end of hv_compose_msi_msg(), and it should not be NULL. However, in rare circumstances, we might see a failure in hv_compose_msi_msg(), e.g. the hypervisor/host might return an error in comp.comp_pkt.completion_status (at least this is possible in theory). In case we see a failure in hv_compose_msi_msg(), data->chip_data stays with its default value of NULL; because the return type of hv_compose_msi_msg() is "void", we can not return an error to the upper layer; later, when the upper layer calls request_irq() -> ... -> hv_irq_unmask(), hv_arch_irq_unmask() crashes because data->chip_data is NULL -- with this check, we're able to error out gracefully, and the user can better understand what goes wrong. > > spin_lock_irqsave(&hbus->retarget_msi_interrupt_lock, flags); > > > > @@ -1911,12 +1916,6 @@ static void hv_compose_msi_msg(struct > irq_data *data, struct msi_msg *msg) > > hv_pci_onchannelcallback(hbus); > > spin_unlock_irqrestore(&channel->sched_lock, flags); > > > > - if (hpdev->state == hv_pcichild_ejecting) { > > - dev_err_once(&hbus->hdev->device, > > - "the device is being ejected\n"); > > - goto enable_tasklet; > > - } > > - > > udelay(100); > > } > > I don't understand why this code is in hv_compose_msi_msg() in the first > place (and why only in that function ?) to me this looks like you are > adding plasters in the code that can turn out to be problematic while > ejecting a device, this does not seem robust at all - that's my opinion. The code was incorrectly added by de0aa7b2f97d ("PCI: hv: Fix 2 hang issues in hv_compose_msi_msg()") de0aa7b2f97d says " 2. If the host is ejecting the VF device before we reach hv_compose_msi_msg(), in a UP VM, we can hang in hv_compose_msi_msg() forever, because at this time the host doesn't respond to the CREATE_INTERRUPT request. " de0aa7b2f97d implies that the host doesn't respond to the guest's CREATE_INTERRUPT request once the guest receives the PCI_EJECT message -- this is incorrect: after the guest receives the PCI_EJECT message, actually the host still responds to the guest's request, as long as the guest sends the request within 1 minute AND the guest doesn't send a PCI_EJECTION_COMPLETE message to the host in hv_eject_device_work(). The real issue is that currently the guest can send PCI_EJECTION_COMPLETE to the host before the guest finishes the device-probing/removing handling -- once the guest sends PCI_EJECTION_COMPLETE, the host unassigns the PCI device from the guest and ignores any request from the guest. So here the check "hpdev->state == hv_pcichild_ejecting" is incorrect. We should remove the check since it can cause a panic (see the commit messsage for the detailed explanation) The "premature PCI_EJECTION_COMPLETE" issue is resolved by: [PATCH v3 5/6] PCI: hv: Add a per-bus mutex state_lock > Feel free to merge this code, I can't ACK it, sorry. > > Lorenzo Thanks for sharing the thougths!
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c index b82c7cde19e66..1b11cf7391933 100644 --- a/drivers/pci/controller/pci-hyperv.c +++ b/drivers/pci/controller/pci-hyperv.c @@ -643,6 +643,11 @@ static void hv_arch_irq_unmask(struct irq_data *data) pbus = pdev->bus; hbus = container_of(pbus->sysdata, struct hv_pcibus_device, sysdata); int_desc = data->chip_data; + if (!int_desc) { + dev_warn(&hbus->hdev->device, "%s() can not unmask irq %u\n", + __func__, data->irq); + return; + } spin_lock_irqsave(&hbus->retarget_msi_interrupt_lock, flags); @@ -1911,12 +1916,6 @@ static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg) hv_pci_onchannelcallback(hbus); spin_unlock_irqrestore(&channel->sched_lock, flags); - if (hpdev->state == hv_pcichild_ejecting) { - dev_err_once(&hbus->hdev->device, - "the device is being ejected\n"); - goto enable_tasklet; - } - udelay(100); }
When the host tries to remove a PCI device, the host first sends a PCI_EJECT message to the guest, and the guest is supposed to gracefully remove the PCI device and send a PCI_EJECTION_COMPLETE message to the host; the host then sends a VMBus message CHANNELMSG_RESCIND_CHANNELOFFER to the guest (when the guest receives this message, the device is already unassigned from the guest) and the guest can do some final cleanup work; if the guest fails to respond to the PCI_EJECT message within one minute, the host sends the VMBus message CHANNELMSG_RESCIND_CHANNELOFFER and removes the PCI device forcibly. In the case of fast device addition/removal, it's possible that the PCI device driver is still configuring MSI-X interrupts when the guest receives the PCI_EJECT message; the channel callback calls hv_pci_eject_device(), which sets hpdev->state to hv_pcichild_ejecting, and schedules a work hv_eject_device_work(); if the PCI device driver is calling pci_alloc_irq_vectors() -> ... -> hv_compose_msi_msg(), we can break the while loop in hv_compose_msi_msg() due to the updated hpdev->state, and leave data->chip_data with its default value of NULL; later, when the PCI device driver calls request_irq() -> ... -> hv_irq_unmask(), the guest crashes in hv_arch_irq_unmask() due to data->chip_data being NULL. Fix the issue by not testing hpdev->state in the while loop: when the guest receives PCI_EJECT, the device is still assigned to the guest, and the guest has one minute to finish the device removal gracefully. We don't really need to (and we should not) test hpdev->state in the loop. Fixes: de0aa7b2f97d ("PCI: hv: Fix 2 hang issues in hv_compose_msi_msg()") Signed-off-by: Dexuan Cui <decui@microsoft.com> Reviewed-by: Michael Kelley <mikelley@microsoft.com> Cc: stable@vger.kernel.org --- v2: Removed the "debug code". No change to the patch body. Added Cc:stable v3: Added Michael's Reviewed-by. drivers/pci/controller/pci-hyperv.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)