Re: [PATCH] xen/vpci: initialize msix->next

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

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