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
