| Message ID | 20221110165935.106376-2-dvrabel@amazon.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: Fix racy accesses to MSI-X Control register | expand |
On 10.11.2022 17:59, David Vrabel wrote: > When setting up an MSI-X vector in msix_capability_init() the error > handling after a BAR mapping failure is different depending on whether > the first page fails or a subsequent page. There's no reason to break > working vectors so consistently use the later error handling > behaviour. "zap_on_error" can only be set when there were no working vectors yet (msix->used_entries being zero), so I don't see what case this last sentence describes. In fact it was the intention with "zap_on_error" to leave previously set up vectors functional. > The zap_on_error flag was added as part of XSA-337, beb54596cfda > (x86/MSI-X: restrict reading of table/PBA bases from BARs), but > appears to be unrelated to XSA-337 and is not useful because: > > 1. table.first and pba.first are not used unless msix->used_vectors > 0. This isn't true afaics. The condition around their setting up is involving more than just ->used_vectors: if ( !msix->used_entries && (!msi || (is_hardware_domain(current->domain) && (dev->domain == current->domain || dev->domain == dom_io))) ) Hence the associated "else if( !msix->table.first )" can also be taken if msix->used_entries is zero. And in case of a failure we need to force the error return there for DomU-s, which is achieved by clearing msix->table.first on the error handling path you alter. Furthermore I'd consider it bad practice to leave stale values on record. > 2. Force disabling MSI-X in this error path is not necessary as the > per-vector mask is still still set. I agree that we might be overly strict there, but to remove that disabling you'd need to further prove that no other inconsistencies can (later) result (this being on the safe side is where the connection to the rest of the XSA-337 changes comes from, along with the desire to not leave stale values around, as per above). Plus you'd want to justify why this error path is different from others in the function where we also disable MSI-X altogether (beyond the path you modify there's exactly one error path where we don't, and I now wonder why I had done it like that). But then I may also be misunderstanding some of your intentions here. The "consistently" in the title and the associated first sentence of the description escape me for the moment: You're talking about things in terms of pages, when the handling really is in terms of entries. Jan
diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c index d0bf63df1d..8bde6b9be1 100644 --- a/xen/arch/x86/msi.c +++ b/xen/arch/x86/msi.c @@ -776,7 +776,7 @@ static int msix_capability_init(struct pci_dev *dev, u8 bus = dev->bus; u8 slot = PCI_SLOT(dev->devfn); u8 func = PCI_FUNC(dev->devfn); - bool maskall = msix->host_maskall, zap_on_error = false; + bool maskall = msix->host_maskall; unsigned int pos = pci_find_cap_offset(seg, bus, slot, func, PCI_CAP_ID_MSIX); @@ -875,8 +875,6 @@ static int msix_capability_init(struct pci_dev *dev, BITS_TO_LONGS(msix->nr_entries) - 1); WARN_ON(rangeset_overlaps_range(mmio_ro_ranges, msix->pba.first, msix->pba.last)); - - zap_on_error = true; } else if ( !msix->table.first ) { @@ -897,14 +895,6 @@ static int msix_capability_init(struct pci_dev *dev, if ( idx < 0 ) { - if ( zap_on_error ) - { - msix->table.first = 0; - msix->pba.first = 0; - - control &= ~PCI_MSIX_FLAGS_ENABLE; - } - pci_conf_write16(dev->sbdf, msix_control_reg(pos), control); xfree(entry); return idx;
When setting up an MSI-X vector in msix_capability_init() the error handling after a BAR mapping failure is different depending on whether the first page fails or a subsequent page. There's no reason to break working vectors so consistently use the later error handling behaviour. The zap_on_error flag was added as part of XSA-337, beb54596cfda (x86/MSI-X: restrict reading of table/PBA bases from BARs), but appears to be unrelated to XSA-337 and is not useful because: 1. table.first and pba.first are not used unless msix->used_vectors > 0. 2. Force disabling MSI-X in this error path is not necessary as the per-vector mask is still still set. Signed-off-by: David Vrabel <dvrabel@amazon.co.uk> CR: https://code.amazon.com/reviews/CR-79020908 --- xen/arch/x86/msi.c | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-)