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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> @@ -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.)

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

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