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(+)

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 )
-- 
2.40.0

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