| Message ID | 20230414202932.293688-1-stewart.hildebrand@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/vpci: initialize msix->next | expand |
On 14.04.2023 22:29, Stewart Hildebrand wrote: > The list was not being initialized, which could result in a crash in > vpci_remove_device if no list items were added. Can you please point out the code path which may lead to such a crash? > --- a/xen/drivers/vpci/msix.c > +++ b/xen/drivers/vpci/msix.c > @@ -678,6 +678,8 @@ static int cf_check init_msix(struct pci_dev *pdev) > if ( !msix ) > return -ENOMEM; > > + INIT_LIST_HEAD(&msix->next); > + > rc = vpci_add_register(pdev->vpci, control_read, control_write, > msix_control_reg(msix_offset), 2, msix); > if ( rc ) The error path below here frees msix again, so can't be a problem. The only other return path from the function is after a suitable list_add(). "... if no list items were added" is misleading too - this isn't a list head, but a list element. The list head is d->arch.hvm.msix_tables. Jan
On 4/17/23 05:09, Jan Beulich wrote: > On 14.04.2023 22:29, Stewart Hildebrand wrote: >> The list was not being initialized, which could result in a crash in >> vpci_remove_device if no list items were added. > > Can you please point out the code path which may lead to such a crash? It would be xen/drivers/vpci/vpci.c:59: list_del(&pdev->vpci->msix->next); The crash was observed when msix->next had not been initialized (nor added to a list_head). It was uninitialized because ... >> --- a/xen/drivers/vpci/msix.c >> +++ b/xen/drivers/vpci/msix.c >> @@ -678,6 +678,8 @@ static int cf_check init_msix(struct pci_dev *pdev) >> if ( !msix ) >> return -ENOMEM; >> >> + INIT_LIST_HEAD(&msix->next); >> + >> rc = vpci_add_register(pdev->vpci, control_read, control_write, >> msix_control_reg(msix_offset), 2, msix); >> if ( rc ) > > The error path below here frees msix again, so can't be a problem. The > only other return path from the function is after a suitable list_add(). ... when I wrote this I had applied patch that removed the list_add() in question (on ARM). See [1]. > "... if no list items were added" is misleading too - this isn't a list > head, but a list element. The list head is d->arch.hvm.msix_tables. I can see now that the crash should more appropriately be resolved in the referenced patch where the crash was actually observed [1]. In the current vpci on ARM work-in-progress, there's no equivalent struct list_head msix_tables... Sorry for the noise. [1] https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc/-/commit/9f36d1b1dffcca1ae3fcb2dfcac4709c39d1b3bc
diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c index 25bde77586a4..1b98c3c10a64 100644 --- a/xen/drivers/vpci/msix.c +++ b/xen/drivers/vpci/msix.c @@ -678,6 +678,8 @@ static int cf_check init_msix(struct pci_dev *pdev) if ( !msix ) return -ENOMEM; + INIT_LIST_HEAD(&msix->next); + rc = vpci_add_register(pdev->vpci, control_read, control_write, msix_control_reg(msix_offset), 2, msix); if ( rc )
The list was not being initialized, which could result in a crash in vpci_remove_device if no list items were added. Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com> --- xen/drivers/vpci/msix.c | 2 ++ 1 file changed, 2 insertions(+)