Hi Stewart,
Stewart Hildebrand <[email protected]> writes: > On 6/13/23 06:32, Volodymyr Babchuk wrote: > > ... > >> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c >> index 652807a4a4..1270174e78 100644 >> --- a/xen/drivers/vpci/vpci.c >> +++ b/xen/drivers/vpci/vpci.c >> @@ -38,20 +38,32 @@ extern vpci_register_init_t *const __end_vpci_array[]; >> >> void vpci_remove_device(struct pci_dev *pdev) >> { >> - if ( !has_vpci(pdev->domain) || !pdev->vpci ) >> + struct vpci *vpci; >> + >> + if ( !has_vpci(pdev->domain) ) >> return; >> >> - spin_lock(&pdev->vpci->lock); >> + write_lock(&pdev->domain->vpci_rwlock); >> + if ( !pdev->vpci ) >> + { >> + write_unlock(&pdev->domain->vpci_rwlock); >> + return; >> + } >> + >> + vpci = pdev->vpci; >> + pdev->vpci = NULL; > > Here we set pdev->vpci to NULL, yet there are still a few uses > remaining after this in vpci_remove_device. I suspect they were missed > during a s/pdev->vpci/vpci/ search and replace. > Yes, you are right. Thank you, I'll fix this in the next version. >> + write_unlock(&pdev->domain->vpci_rwlock); >> + >> while ( !list_empty(&pdev->vpci->handlers) ) > > pdev->vpci dereferenced here > >> { >> - struct vpci_register *r = list_first_entry(&pdev->vpci->handlers, >> + struct vpci_register *r = list_first_entry(&vpci->handlers, >> struct vpci_register, >> node); >> >> list_del(&r->node); >> xfree(r); >> } >> - spin_unlock(&pdev->vpci->lock); >> + >> if ( pdev->vpci->msix ) > > pdev->vpci dereferenced here > >> { >> unsigned int i; >> @@ -61,29 +73,33 @@ void vpci_remove_device(struct pci_dev *pdev) >> if ( pdev->vpci->msix->table[i] ) > > pdev->vpci dereferenced here, and two more above not shown in the diff context > >> iounmap(pdev->vpci->msix->table[i]); > > pdev->vpci dereferenced here > >> } >> - xfree(pdev->vpci->msix); >> - xfree(pdev->vpci->msi); >> - xfree(pdev->vpci); >> - pdev->vpci = NULL; >> + xfree(vpci->msix); >> + xfree(vpci->msi); >> + xfree(vpci); >> } -- WBR, Volodymyr
