Re: [PATCH] x86/msi: fix locking for SRIOV devices

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 8/6/24 02:25, Jan Beulich wrote:
> On 05.08.2024 23:09, Stewart Hildebrand wrote:
>> In commit 4f78438b45e2 ("vpci: use per-domain PCI lock to protect vpci
>> structure") a lock moved from allocate_and_map_msi_pirq() to the caller
>> and changed from pcidevs_lock() to read_lock(&d->pci_lock). However, one
>> call path wasn't updated to reflect the change, leading to a failed
>> assertion observed on debug builds of Xen when an SRIOV device is
>> present with one or more VFs enabled:
>>
>> (XEN) Assertion 'd || pcidevs_locked()' failed at 
>> drivers/passthrough/pci.c:535
>> (XEN) ----[ Xen-4.20-unstable  x86_64  debug=y  Not tainted ]----
> 
> There must be more to this than just "SR-IOV with VFs enabled"? The
> locking change has been there for quite a while, yet the issue was
> noticed only know.

Yes, the conditions as far as I can tell are:
* PV dom0
* Debug build (debug=y) only
* There is a SR-IOV device in the system with one or more VFs enabled/configured
* Dom0 has loaded the driver for the VF and enabled MSI-X

The assert triggers when dom0 loads the VF driver and enables MSI-X
before PHYSDEVOP_prepare_msix is invoked.

>> @@ -829,7 +829,8 @@ static int msix_capability_init(struct pci_dev *dev,
>>              vf = dev->sbdf.bdf;
>>          }
>>  
>> -        table_paddr = read_pci_mem_bar(seg, pbus, pslot, pfunc, bir, vf);
>> +        table_paddr = read_pci_mem_bar(dev->domain, seg, pbus, pslot, pfunc,
>> +                                       bir, vf);
> 
> While dev->domain is the owner of dev (seg:bus:slot.func), it may not be
> the owner of seg:pbus:pslot.pfunc. Or if it (reliably) is, I'd expect the
> guarantees towards that to be spelled out in the description. (Dependency
> on ordering of toolstack operations likely wouldn't count as a guarantee
> to me. Yet if I'm not mistaken there's a problem in all cases if the VF
> was already moved to DomIO, before the DomU is started.)

On the possibility that they're not the same (now or in the future),
disregard this patch.

> Further to this, until realizing that the code path leading here is
> accessible only for Dom0, I suspected a security angle to this, and hence
> didn't respond publicly on Matrix. I noticed, however, that apparently
> there's another instance of the same issue (via the other call site of
> allocate_and_map_msi_pirq(), i.e. from vpci_msix_arch_enable_entry()
> through vpci_msi_enable()). Imo that wants dealing with at the same time
> (potentially requiring a different overall approach).

There's a simpler way to solve this: the VF doesn't strictly need to
obtain the pdev of its associated PF, so the call to pci_get_pdev() can
be removed from read_pci_mem_bar(). Patch to follow...

> The code path leading here being accessible only for Dom0 likely is also
> a mistake (read: overly strict). In principle it ought to be possible to
> move a PF to a driver domain, for the VFs to then be assigned to
> individual DomU-s. In such a case I expect it shouldn't be Dom0 to invoke
> PHYSDEVOP_prepare_msix.
> 
> Jan