On 17.11.2021 23:19, Bjorn Helgaas wrote: > On Wed, Nov 17, 2021 at 10:31:51PM +0100, Heiner Kallweit wrote: >> On 13.10.2021 20:37, Heiner Kallweit wrote: >>> We have a problem with a device where each VPD read returns 0x33 [0]. >>> This results in a valid VPD structure (except the tag id) and >>> therefore pci_vpd_size() scans the full VPD address range. >>> On an affected system this took ca. 80s. >>> >>> That's not acceptable, on the other hand we may not want to re-add >>> the old tag checks. In addition these tag check still wouldn't be able >>> to avoid the described scenario 100%. >>> Instead let's add a simple sanity check on the number of found tags. >>> A VPD image conforming to the PCI spec [1] can have max. 4 tags: >>> id string, ro section, rw section, end tag. >>> >>> [0] https://lore.kernel.org/lkml/20210915223218.GA1542966@bjorn-Precision-5520/ >>> [1] PCI 3.0 I.3.1. VPD Large and Small Resource Data Tags >>> >>> Reviewed-by: Krzysztof Wilczyński <kw@xxxxxxxxx> >>> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx> >>> --- >>> drivers/pci/vpd.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c >>> index a4fc4d069..921470611 100644 >>> --- a/drivers/pci/vpd.c >>> +++ b/drivers/pci/vpd.c >>> @@ -56,6 +56,7 @@ static size_t pci_vpd_size(struct pci_dev *dev) >>> { >>> size_t off = 0, size; >>> unsigned char tag, header[1+2]; /* 1 byte tag, 2 bytes length */ >>> + int num_tags = 0; >>> >>> while (pci_read_vpd_any(dev, off, 1, header) == 1) { >>> size = 0; >>> @@ -63,6 +64,10 @@ static size_t pci_vpd_size(struct pci_dev *dev) >>> if (off == 0 && (header[0] == 0x00 || header[0] == 0xff)) >>> goto error; >>> >>> + /* We can have max 4 tags: STRING_ID, RO, RW, END */ >>> + if (++num_tags > 4) >>> + goto error; >>> + >>> if (header[0] & PCI_VPD_LRDT) { >>> /* Large Resource Data Type Tag */ >>> if (pci_read_vpd_any(dev, off + 1, 2, &header[1]) != 2) { >>> >> >> Can this one be picked up for next? > > I'm hesitating because we (or maybe just "I" :)) worked so hard to > avoid interpreting the VPD data, and now we're back to that. > > There's nothing of value in this particular device's VPD. Is there > any reason we shouldn't just use quirk_blacklist_vpd() for it? > The bogus device we talk about has vendor/device id of an Intel card, we could blacklist just based on subvendor/device id. Seems the quirk mechanism doesn't support subvendor id level. In general: Once we blacklist this device, tomorrow another similarly broken one may come. Therefore I'd prefer the more general approach. But I see your point. If (theoretically) the next PCI spec would introduce a new VPD tag, then we most likely would get to know about this only once somebody complains about reading VPD from his shiny new card fails. So it's a tradeoff .. > Bjorn > Heiner
