| Message ID | 20230816175939.21566-1-decui@microsoft.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] PCI: hv: Fix a crash in hv_pci_restore_msi_msg() during hibernation | expand |
> From: Dexuan Cui <decui@microsoft.com> > Sent: Wednesday, August 16, 2023 11:00 AM > [...] > When a Linux VM with an assigned PCI device runs on Hyper-V, if the PCI > device driver is not loaded yet (i.e. MSI-X/MSI is not enabled on the > device yet), doing a VM hibernation triggers a panic in > hv_pci_restore_msi_msg() -> msi_lock_descs(&pdev->dev), because > pdev->dev.msi.data is still NULL. > > Avoid the panic by checking if MSI-X/MSI is enabled. > > Fixes: dc2b453290c4 ("PCI: hv: Rework MSI handling") > Signed-off-by: Dexuan Cui <decui@microsoft.com> Sorry, I forgot to add Cc: stable@vger.kernel.org
On 8/16/2023 10:59 AM, Dexuan Cui wrote: > When a Linux VM with an assigned PCI device runs on Hyper-V, if the PCI > device driver is not loaded yet (i.e. MSI-X/MSI is not enabled on the > device yet), doing a VM hibernation triggers a panic in > hv_pci_restore_msi_msg() -> msi_lock_descs(&pdev->dev), because > pdev->dev.msi.data is still NULL. > > Avoid the panic by checking if MSI-X/MSI is enabled. > > Fixes: dc2b453290c4 ("PCI: hv: Rework MSI handling") > Signed-off-by: Dexuan Cui <decui@microsoft.com> > --- > > Changes in v2: > Replaced the test "if (!pdev->dev.msi.data)" with > "if (!pdev->msi_enabled && !pdev->msix_enabled)". > Thanks Michael! > Updated the changelog accordingly. > > drivers/pci/controller/pci-hyperv.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c > index 2d93d0c4f10d..bed3cefdaf19 100644 > --- a/drivers/pci/controller/pci-hyperv.c > +++ b/drivers/pci/controller/pci-hyperv.c > @@ -3983,6 +3983,9 @@ static int hv_pci_restore_msi_msg(struct pci_dev *pdev, void *arg) > struct msi_desc *entry; > int ret = 0; > > + if (!pdev->msi_enabled && !pdev->msix_enabled) > + return 0; Isn't this is a error condition? Don't you want to return error here? > + > msi_lock_descs(&pdev->dev); > msi_for_each_desc(entry, &pdev->dev, MSI_DESC_ASSOCIATED) { > irq_data = irq_get_irq_data(entry->irq);
> From: Kuppuswamy Sathyanarayanan > <sathyanarayanan.kuppuswamy@linux.intel.com> > Sent: Wednesday, August 16, 2023 11:12 AM > [...] > On 8/16/2023 10:59 AM, Dexuan Cui wrote: > > When a Linux VM with an assigned PCI device runs on Hyper-V, if the PCI > > device driver is not loaded yet (i.e. MSI-X/MSI is not enabled on the > > device yet), doing a VM hibernation triggers a panic in > > hv_pci_restore_msi_msg() -> msi_lock_descs(&pdev->dev), because > > pdev->dev.msi.data is still NULL. > > > > Avoid the panic by checking if MSI-X/MSI is enabled. > > > > Fixes: dc2b453290c4 ("PCI: hv: Rework MSI handling") > > Signed-off-by: Dexuan Cui <decui@microsoft.com> > > --- > > > > Changes in v2: > > Replaced the test "if (!pdev->dev.msi.data)" with > > "if (!pdev->msi_enabled && !pdev->msix_enabled)". > > Thanks Michael! > > Updated the changelog accordingly. > > > > drivers/pci/controller/pci-hyperv.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci- > hyperv.c > > index 2d93d0c4f10d..bed3cefdaf19 100644 > > --- a/drivers/pci/controller/pci-hyperv.c > > +++ b/drivers/pci/controller/pci-hyperv.c > > @@ -3983,6 +3983,9 @@ static int hv_pci_restore_msi_msg(struct pci_dev > *pdev, void *arg) > > struct msi_desc *entry; > > int ret = 0; > > > > + if (!pdev->msi_enabled && !pdev->msix_enabled) > > + return 0; > Isn't this is a error condition? Don't you want to return error here? This is not an error. If a PCI device driver is not loaded or not installed, MSI-X/MSI is not enabled on the device, so pdev->msi_enabled is 0 and pdev->msix_enabled is 0. In this case, it's still legit for a user to request the system (i.e. here it's a Linux VM running on Hyper-V) to hibernate -- in this case, we should not try to save/restore the MSI/MSI-X state, and we should not let the hibernation fail; here we should just ignore the device by returning a success ("return 0;"). > > + > > msi_lock_descs(&pdev->dev); > > msi_for_each_desc(entry, &pdev->dev, MSI_DESC_ASSOCIATED) { > > irq_data = irq_get_irq_data(entry->irq);
On 8/16/2023 12:30 PM, Dexuan Cui wrote: >> From: Kuppuswamy Sathyanarayanan >> <sathyanarayanan.kuppuswamy@linux.intel.com> >> Sent: Wednesday, August 16, 2023 11:12 AM >> [...] >> On 8/16/2023 10:59 AM, Dexuan Cui wrote: >>> When a Linux VM with an assigned PCI device runs on Hyper-V, if the PCI >>> device driver is not loaded yet (i.e. MSI-X/MSI is not enabled on the >>> device yet), doing a VM hibernation triggers a panic in >>> hv_pci_restore_msi_msg() -> msi_lock_descs(&pdev->dev), because >>> pdev->dev.msi.data is still NULL. >>> >>> Avoid the panic by checking if MSI-X/MSI is enabled. >>> >>> Fixes: dc2b453290c4 ("PCI: hv: Rework MSI handling") >>> Signed-off-by: Dexuan Cui <decui@microsoft.com> >>> --- >>> >>> Changes in v2: >>> Replaced the test "if (!pdev->dev.msi.data)" with >>> "if (!pdev->msi_enabled && !pdev->msix_enabled)". >>> Thanks Michael! >>> Updated the changelog accordingly. >>> >>> drivers/pci/controller/pci-hyperv.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci- >> hyperv.c >>> index 2d93d0c4f10d..bed3cefdaf19 100644 >>> --- a/drivers/pci/controller/pci-hyperv.c >>> +++ b/drivers/pci/controller/pci-hyperv.c >>> @@ -3983,6 +3983,9 @@ static int hv_pci_restore_msi_msg(struct pci_dev >> *pdev, void *arg) >>> struct msi_desc *entry; >>> int ret = 0; >>> >>> + if (!pdev->msi_enabled && !pdev->msix_enabled) >>> + return 0; >> Isn't this is a error condition? Don't you want to return error here? > This is not an error. If a PCI device driver is not loaded or not installed, > MSI-X/MSI is not enabled on the device, so pdev->msi_enabled is 0 > and pdev->msix_enabled is 0. In this case, it's still legit for a user to request > the system (i.e. here it's a Linux VM running on Hyper-V) to hibernate -- in > this case, we should not try to save/restore the MSI/MSI-X state, and we > should not let the hibernation fail; here we should just ignore the device > by returning a success ("return 0;"). Got it. Looks good to me. Reviewed-by: sathyanarayanan.kuppuswamy@linux.intel.com > >>> + >>> msi_lock_descs(&pdev->dev); >>> msi_for_each_desc(entry, &pdev->dev, MSI_DESC_ASSOCIATED) { >>> irq_data = irq_get_irq_data(entry->irq);
From: Dexuan Cui <decui@microsoft.com> Sent: Wednesday, August 16, 2023 11:00 AM > > When a Linux VM with an assigned PCI device runs on Hyper-V, if the PCI > device driver is not loaded yet (i.e. MSI-X/MSI is not enabled on the > device yet), doing a VM hibernation triggers a panic in > hv_pci_restore_msi_msg() -> msi_lock_descs(&pdev->dev), because > pdev->dev.msi.data is still NULL. > > Avoid the panic by checking if MSI-X/MSI is enabled. > > Fixes: dc2b453290c4 ("PCI: hv: Rework MSI handling") > Signed-off-by: Dexuan Cui <decui@microsoft.com> > --- > > Changes in v2: > Replaced the test "if (!pdev->dev.msi.data)" with > "if (!pdev->msi_enabled && !pdev->msix_enabled)". > Thanks Michael! > Updated the changelog accordingly. > > drivers/pci/controller/pci-hyperv.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c > index 2d93d0c4f10d..bed3cefdaf19 100644 > --- a/drivers/pci/controller/pci-hyperv.c > +++ b/drivers/pci/controller/pci-hyperv.c > @@ -3983,6 +3983,9 @@ static int hv_pci_restore_msi_msg(struct pci_dev *pdev, void *arg) > struct msi_desc *entry; > int ret = 0; > > + if (!pdev->msi_enabled && !pdev->msix_enabled) > + return 0; > + > msi_lock_descs(&pdev->dev); > msi_for_each_desc(entry, &pdev->dev, MSI_DESC_ASSOCIATED) { > irq_data = irq_get_irq_data(entry->irq); > -- > 2.25.1 Reviewed-by: Michael Kelley <mikelley@microsoft.com>
On Wed, Aug 16, 2023 at 08:10:25PM +0000, Michael Kelley (LINUX) wrote: > From: Dexuan Cui <decui@microsoft.com> Sent: Wednesday, August 16, 2023 11:00 AM > > > > When a Linux VM with an assigned PCI device runs on Hyper-V, if the PCI > > device driver is not loaded yet (i.e. MSI-X/MSI is not enabled on the > > device yet), doing a VM hibernation triggers a panic in > > hv_pci_restore_msi_msg() -> msi_lock_descs(&pdev->dev), because > > pdev->dev.msi.data is still NULL. > > > > Avoid the panic by checking if MSI-X/MSI is enabled. > > > > Fixes: dc2b453290c4 ("PCI: hv: Rework MSI handling") > > Signed-off-by: Dexuan Cui <decui@microsoft.com> > > --- > > > > Changes in v2: > > Replaced the test "if (!pdev->dev.msi.data)" with > > "if (!pdev->msi_enabled && !pdev->msix_enabled)". > > Thanks Michael! > > Updated the changelog accordingly. > > > > drivers/pci/controller/pci-hyperv.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c > > index 2d93d0c4f10d..bed3cefdaf19 100644 > > --- a/drivers/pci/controller/pci-hyperv.c > > +++ b/drivers/pci/controller/pci-hyperv.c > > @@ -3983,6 +3983,9 @@ static int hv_pci_restore_msi_msg(struct pci_dev *pdev, void *arg) > > struct msi_desc *entry; > > int ret = 0; > > > > + if (!pdev->msi_enabled && !pdev->msix_enabled) > > + return 0; > > + > > msi_lock_descs(&pdev->dev); > > msi_for_each_desc(entry, &pdev->dev, MSI_DESC_ASSOCIATED) { > > irq_data = irq_get_irq_data(entry->irq); > > -- > > 2.25.1 > > Reviewed-by: Michael Kelley <mikelley@microsoft.com> > > I expect this to go through the PCI tree. Just let me know if I should pick this up. Thanks, Wei.
On Wed, 16 Aug 2023 10:59:39 -0700, Dexuan Cui wrote: > When a Linux VM with an assigned PCI device runs on Hyper-V, if the PCI > device driver is not loaded yet (i.e. MSI-X/MSI is not enabled on the > device yet), doing a VM hibernation triggers a panic in > hv_pci_restore_msi_msg() -> msi_lock_descs(&pdev->dev), because > pdev->dev.msi.data is still NULL. > > Avoid the panic by checking if MSI-X/MSI is enabled. > > [...] Applied to controller/hv, thanks! [1/1] PCI: hv: Fix a crash in hv_pci_restore_msi_msg() during hibernation https://git.kernel.org/pci/pci/c/04bbe863241a Thanks, Lorenzo
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c index 2d93d0c4f10d..bed3cefdaf19 100644 --- a/drivers/pci/controller/pci-hyperv.c +++ b/drivers/pci/controller/pci-hyperv.c @@ -3983,6 +3983,9 @@ static int hv_pci_restore_msi_msg(struct pci_dev *pdev, void *arg) struct msi_desc *entry; int ret = 0; + if (!pdev->msi_enabled && !pdev->msix_enabled) + return 0; + msi_lock_descs(&pdev->dev); msi_for_each_desc(entry, &pdev->dev, MSI_DESC_ASSOCIATED) { irq_data = irq_get_irq_data(entry->irq);
When a Linux VM with an assigned PCI device runs on Hyper-V, if the PCI device driver is not loaded yet (i.e. MSI-X/MSI is not enabled on the device yet), doing a VM hibernation triggers a panic in hv_pci_restore_msi_msg() -> msi_lock_descs(&pdev->dev), because pdev->dev.msi.data is still NULL. Avoid the panic by checking if MSI-X/MSI is enabled. Fixes: dc2b453290c4 ("PCI: hv: Rework MSI handling") Signed-off-by: Dexuan Cui <decui@microsoft.com> --- Changes in v2: Replaced the test "if (!pdev->dev.msi.data)" with "if (!pdev->msi_enabled && !pdev->msix_enabled)". Thanks Michael! Updated the changelog accordingly. drivers/pci/controller/pci-hyperv.c | 3 +++ 1 file changed, 3 insertions(+)