Skip to contentSkip to conversation

PCI VPD access fixes

PATCH In review Hannes Reinecke 3 participants 7 messages Feb 15, 2016 – Feb 22, 2016 PCI

lore
Share & tools
lore
#0 lore raw

v3 cover PCI VPD access fixes

the current PCI VPD page access assumes that the entire possible VPD data is readable. However, the spec only guarantees a VPD data up to the 'end' marker, with everything beyond that being undefined. This causes a system lockup on certain devices.

With this patch we always set the VPD sysfs attribute size to '0', and calculate the available VPD size on the first access.
If no valid data can be read an I/O error is returned.

I've also included the patch from Babu to blacklists devices which are known to lockup when accessing the VPD data.

Changes to v2:
- Spelling fixes as suggested by Bjorn Helgaas
- Update to correct vpd size in pci_vpc_pci22_write
- Pass in modified VPD size to pci_vpd_pci22_size() to make quirk_brcm_570x_limit_vpd work

Babu Moger (1):
pci: Blacklist vpd access for buggy devices

Hannes Reinecke (3):
pci: Update VPD definitions
pci: allow access to VPD attributes with size '0'
pci: Determine actual VPD size on first access

 drivers/pci/access.c    | 84 +++++++++++++++++++++++++++++++++++++++++++++++--
 drivers/pci/pci-sysfs.c | 22 +++++++------
 drivers/pci/quirks.c    | 43 +++++++++++++++++++++++++
 include/linux/pci.h     | 27 ++++++++++++++--
 4 files changed, 162 insertions(+), 14 deletions(-)
Signature · 1 line
1.8.5.6
#1 lore raw

PATCH v3 1/4 pci: Update VPD definitions

The 'end' tag is actually 0x0f, it's the representation as a small resource data type tag that's 0x78 (ie shifted by 3). This patch also adds helper functions to extract the resource data type tags for both large and small resource data types.

Cc Alexander Duyck <[email protected]>
Cc Bjorn Helgaas <[email protected]>
---
 include/linux/pci.h | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)
include/linux/pci.h +25 -2
index 27df4a6..49ad85c 100644
@@ -1834,12 +1834,13 @@ bool pci_acs_path_enabled(struct pci_dev *start,
 #define PCI_VPD_LRDT_RW_DATA		PCI_VPD_LRDT_ID(PCI_VPD_LTIN_RW_DATA)
 
 /* Small Resource Data Type Tag Item Names */
-#define PCI_VPD_STIN_END		0x78	/* End */
+#define PCI_VPD_STIN_END		0x0f	/* End */
 
-#define PCI_VPD_SRDT_END		PCI_VPD_STIN_END
+#define PCI_VPD_SRDT_END		(PCI_VPD_STIN_END << 3)
 
 #define PCI_VPD_SRDT_TIN_MASK		0x78
 #define PCI_VPD_SRDT_LEN_MASK		0x07
+#define PCI_VPD_LRDT_TIN_MASK		0x7f
 
 #define PCI_VPD_LRDT_TAG_SIZE		3
 #define PCI_VPD_SRDT_TAG_SIZE		1
@@ -1863,6 +1864,17 @@ static inline u16 pci_vpd_lrdt_size(const u8 *lrdt)
 }
 
 /**
+ * pci_vpd_lrdt_tag - Extracts the Large Resource Data Type Tag Item
+ * @lrdt: Pointer to the beginning of the Large Resource Data Type tag
+ *
+ * Returns the extracted Large Resource Data Type Tag item.
+ */
+static inline u16 pci_vpd_lrdt_tag(const u8 *lrdt)
+{
+    return (u16)(lrdt[0] & PCI_VPD_LRDT_TIN_MASK);
+}
+
+/**
  * pci_vpd_srdt_size - Extracts the Small Resource Data Type length
  * @lrdt: Pointer to the beginning of the Small Resource Data Type tag
  *
@@ -1874,6 +1886,17 @@ static inline u8 pci_vpd_srdt_size(const u8 *srdt)
 }
 
 /**
+ * pci_vpd_srdt_tag - Extracts the Small Resource Data Type Tag Item
+ * @lrdt: Pointer to the beginning of the Small Resource Data Type tag
+ *
+ * Returns the extracted Small Resource Data Type Tag Item.
+ */
+static inline u8 pci_vpd_srdt_tag(const u8 *srdt)
+{
+	return ((*srdt) & PCI_VPD_SRDT_TIN_MASK) >> 3;
+}
+
+/**
  * pci_vpd_info_field_size - Extracts the information field length
  * @lrdt: Pointer to the beginning of an information field header
  *
Signature · 1 line
1.8.5.6
#2 lore raw

PATCH v3 2/4 pci: allow access to VPD attributes with size '0'

In reply to Hannes Reinecke

It is not always possible to determine the actual size of the VPD data, so allow access to them if the size is set to '0'.

Cc Alexander Duyck <[email protected]>
Cc Bjorn Helgaas <[email protected]>
---
 drivers/pci/pci-sysfs.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)
drivers/pci/pci-sysfs.c +12 -8
index 95d9e7b..a730f54 100644
@@ -769,10 +769,12 @@ static ssize_t read_vpd_attr(struct file *filp, struct kobject *kobj,
 {
 	struct pci_dev *dev = to_pci_dev(kobj_to_dev(kobj));
 
-	if (off > bin_attr->size)
-		count = 0;
-	else if (count > bin_attr->size - off)
-		count = bin_attr->size - off;
+	if (bin_attr->size > 0) {
+		if (off > bin_attr->size)
+			count = 0;
+		else if (count > bin_attr->size - off)
+			count = bin_attr->size - off;
+	}
 
 	return pci_read_vpd(dev, off, count, buf);
 }
@@ -783,10 +785,12 @@ static ssize_t write_vpd_attr(struct file *filp, struct kobject *kobj,
 {
 	struct pci_dev *dev = to_pci_dev(kobj_to_dev(kobj));
 
-	if (off > bin_attr->size)
-		count = 0;
-	else if (count > bin_attr->size - off)
-		count = bin_attr->size - off;
+	if (bin_attr->size > 0) {
+		if (off > bin_attr->size)
+			count = 0;
+		else if (count > bin_attr->size - off)
+			count = bin_attr->size - off;
+	}
 
 	return pci_write_vpd(dev, off, count, buf);
 }
Signature · 1 line
1.8.5.6
#3 lore raw

PATCH v3 3/4 pci: Determine actual VPD size on first access

In reply to Hannes Reinecke

PCI-2.2 VPD entries have a maximum size of 32k, but might actually be smaller than that. To figure out the actual size one has to read the VPD area until the 'end marker' is reached.
Trying to read VPD data beyond that marker results in 'interesting' effects, from simple read errors to crashing the card. And to make matters worse not every PCI card implements this properly, leaving us with no 'end' marker or even completely invalid data. This patch tries to determine the size of the VPD data when it's first accessed. If no valid data can be read an I/O error will be returned when reading or writing the sysfs attribute. As the amount of VPD data is unknown initially the size of the sysfs attribute will always be set to '0'.

Cc Alexander Duyck <[email protected]>
Cc Bjorn Helgaas <[email protected]>
---
 drivers/pci/access.c    | 84 +++++++++++++++++++++++++++++++++++++++++++++++--
 drivers/pci/pci-sysfs.c |  2 +-
 2 files changed, 82 insertions(+), 4 deletions(-)
drivers/pci/access.c +81 -3
index 8c05b5c..253e0c8 100644
@@ -283,10 +283,63 @@ struct pci_vpd_pci22 {
 	struct pci_vpd base;
 	struct mutex lock;
 	u16	flag;
-	bool	busy;
+	u8	busy:1;
+	u8	valid:1;
 	u8	cap;
 };
 
+/**
+ * pci_vpd_size - determine actual size of Vital Product Data
+ * @dev:	pci device struct
+ * @old_size:	current assumed size, also maximum allowed size
+ */
+static size_t pci_vpd_pci22_size(struct pci_dev *dev, size_t old_size)
+{
+	size_t off = 0;
+	unsigned char header[1+2];	/* 1 byte tag, 2 bytes length */
+
+	while (off < old_size &&
+	       pci_read_vpd(dev, off, 1, header) == 1) {
+		unsigned char tag;
+
+		if (header[0] & PCI_VPD_LRDT) {
+			/* Large Resource Data Type Tag */
+			tag = pci_vpd_lrdt_tag(header);
+			/* Only read length from known tag items */
+			if ((tag == PCI_VPD_LTIN_ID_STRING) ||
+			    (tag == PCI_VPD_LTIN_RO_DATA) ||
+			    (tag == PCI_VPD_LTIN_RW_DATA)) {
+				if (pci_read_vpd(dev, off+1, 2,
+						 &header[1]) != 2) {
+					dev_dbg(&dev->dev,
+						"invalid large VPD tag %02x size at offset %zu",
+						tag, off + 1);
+					return 0;
+				}
+				off += PCI_VPD_LRDT_TAG_SIZE +
+					pci_vpd_lrdt_size(header);
+			}
+		} else {
+			/* Short Resource Data Type Tag */
+			off += PCI_VPD_SRDT_TAG_SIZE +
+				pci_vpd_srdt_size(header);
+			tag = pci_vpd_srdt_tag(header);
+		}
+		if (tag == PCI_VPD_STIN_END)	/* End tag descriptor */
+			return off;
+		if ((tag != PCI_VPD_LTIN_ID_STRING) &&
+		    (tag != PCI_VPD_LTIN_RO_DATA) &&
+		    (tag != PCI_VPD_LTIN_RW_DATA)) {
+			dev_dbg(&dev->dev,
+				"invalid %s VPD tag %02x at offset %zu",
+				(header[0] & PCI_VPD_LRDT) ? "large" : "short",
+				tag, off);
+			return 0;
+		}
+	}
+	return 0;
+}
+
 /*
  * Wait for last operation to complete.
  * This code has to spin since there is no other notification from the PCI
@@ -337,9 +390,23 @@ static ssize_t pci_vpd_pci22_read(struct pci_dev *dev, loff_t pos, size_t count,
 	loff_t end = pos + count;
 	u8 *buf = arg;
 
-	if (pos < 0 || pos > vpd->base.len || end > vpd->base.len)
+	if (pos < 0)
 		return -EINVAL;
 
+	if (!vpd->valid) {
+		vpd->valid = true;
+		vpd->base.len = pci_vpd_pci22_size(dev, vpd->base.len);
+	}
+	if (vpd->base.len == 0)
+		return -EIO;
+
+	if (end > vpd->base.len) {
+		if (pos > vpd->base.len)
+			return 0;
+		end = vpd->base.len;
+		count = end - pos;
+	}
+
 	if (mutex_lock_killable(&vpd->lock))
 		return -EINTR;
 
@@ -389,7 +456,17 @@ static ssize_t pci_vpd_pci22_write(struct pci_dev *dev, loff_t pos, size_t count
 	loff_t end = pos + count;
 	int ret = 0;
 
-	if (pos < 0 || (pos & 3) || (count & 3) || end > vpd->base.len)
+	if (pos < 0 || (pos & 3) || (count & 3))
+		return -EINVAL;
+
+	if (!vpd->valid) {
+		vpd->valid = true;
+		vpd->base.len = pci_vpd_pci22_size(dev, vpd->base.len);
+	}
+	if (vpd->base.len == 0)
+		return -EIO;
+
+	if (end > vpd->base.len)
 		return -EINVAL;
 
 	if (mutex_lock_killable(&vpd->lock))
@@ -496,6 +573,7 @@ int pci_vpd_pci22_init(struct pci_dev *dev)
 	mutex_init(&vpd->lock);
 	vpd->cap = cap;
 	vpd->busy = false;
+	vpd->valid = false;
 	dev->vpd = &vpd->base;
 	return 0;
 }
drivers/pci/pci-sysfs.c +1 -1
index a730f54..ed39c09 100644
@@ -1323,7 +1323,7 @@ static int pci_create_capabilities_sysfs(struct pci_dev *dev)
 			return -ENOMEM;
 
 		sysfs_bin_attr_init(attr);
-		attr->size = dev->vpd->len;
+		attr->size = 0;
 		attr->attr.name = "vpd";
 		attr->attr.mode = S_IRUSR | S_IWUSR;
 		attr->read = read_vpd_attr;
Signature · 1 line
1.8.5.6
#4 lore raw

PATCH v3 4/4 pci: Blacklist vpd access for buggy devices

In reply to Hannes Reinecke

From: Babu Moger <[email protected]>

Reading or Writing of PCI VPD data causes system panic. We saw this problem by running "lspci -vvv" in the beginning. However this can be easily reproduced by running
cat /sys/bus/devices/XX../vpd

As even a simple read on any VPD data triggers a system lockup on certain cards this patch implements a PCI quirk to disabling VPD acces altogether.
This is done by setting the by setting the vpd length to '0' initially for those devices, which will then inhibit access completely..

---
 drivers/pci/access.c |  4 ++--
 drivers/pci/quirks.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 2 deletions(-)
drivers/pci/access.c +2 -2
index 253e0c8..8b6f5a2 100644
@@ -393,7 +393,7 @@ static ssize_t pci_vpd_pci22_read(struct pci_dev *dev, loff_t pos, size_t count,
 	if (pos < 0)
 		return -EINVAL;
 
-	if (!vpd->valid) {
+	if (!vpd->valid && vpd->base.len > 0) {
 		vpd->valid = true;
 		vpd->base.len = pci_vpd_pci22_size(dev, vpd->base.len);
 	}
@@ -459,7 +459,7 @@ static ssize_t pci_vpd_pci22_write(struct pci_dev *dev, loff_t pos, size_t count
 	if (pos < 0 || (pos & 3) || (count & 3))
 		return -EINVAL;
 
-	if (!vpd->valid) {
+	if (!vpd->valid && vpd->base.len > 0) {
 		vpd->valid = true;
 		vpd->base.len = pci_vpd_pci22_size(dev, vpd->base.len);
 	}
drivers/pci/quirks.c +43 -0

49 lines not on this page · raw message

Signature · 1 line
1.8.5.6
#5 lore raw

Re: [PATCHv3 4/4] pci: Blacklist vpd access for buggy devices

Quoted 7 lines · Hannes Reinecke

From: Babu Moger <[email protected]>

Reading or Writing of PCI VPD data causes system panic. We saw this problem by running "lspci -vvv" in the beginning. However this can be easily reproduced by running
cat /sys/bus/devices/XX../vpd

Quoted 6 lines

As even a simple read on any VPD data triggers a system lockup on certain cards this patch implements a PCI quirk to disabling VPD acces altogether.
This is done by setting the by setting the vpd length to '0' initially for those devices, which will then inhibit access completely..

IIRC, Babu was going to open bugzillas with details about the actual falures. I'd like references to them here.

Quoted 23 lines
---
 drivers/pci/access.c |  4 ++--
 drivers/pci/quirks.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 2 deletions(-)
drivers/pci/access.c +1 -1
index 253e0c8..8b6f5a2 100644
@@ -393,7 +393,7 @@ static ssize_t pci_vpd_pci22_read(struct pci_dev *dev, loff_t pos, size_t count,
 	if (pos < 0)
 		return -EINVAL;
 
-	if (!vpd->valid) {
+	if (!vpd->valid && vpd->base.len > 0) {
 		vpd->valid = true;
 		vpd->base.len = pci_vpd_pci22_size(dev, vpd->base.len);
 	}

drivers/pci/access.c static ssize_t pci_vpd_pci22_write(struct pci_dev *dev, loff_t pos, size_t count · line 462

Quoted 5 lines
drivers/pci/access.c +1 -1
@@ -459,7 +459,7 @@ static ssize_t pci_vpd_pci22_write(struct pci_dev *dev, loff_t pos, size_t count
 	if (pos < 0 || (pos & 3) || (count & 3))
 		return -EINVAL;
 
-	if (!vpd->valid) {
+	if (!vpd->valid && vpd->base.len > 0) {

I think these changes to pci_vpd_pci22_read() and pci_vpd_pci22_write() are unnecessary because pci_vpd_pci22_size() already returns 0 when you pass 0 to it.

Quoted 64 lines
drivers/pci/access.c +0 -0
 		vpd->valid = true;
 		vpd->base.len = pci_vpd_pci22_size(dev, vpd->base.len);
 	}
drivers/pci/quirks.c +43 -0
index 0575a1e..df1178f 100644
@@ -2135,6 +2135,49 @@ static void quirk_via_cx700_pci_parking_caching(struct pci_dev *dev)
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_VIA, 0x324e, quirk_via_cx700_pci_parking_caching);
 
 /*
+ * A read/write to sysfs entry ('/sys/bus/pci/devices/<id>/vpd')
+ * will dump 32k of data. The default length is set as 32768.
+ * Reading a full 32k will cause an access beyond the VPD end tag.
+ * The system behaviour at that point is mostly unpredictable.
+ * Apparently, some vendors have not implemented this VPD headers properly.
+ * Adding a generic function disable vpd data for these buggy adapters
+ * Add the DECLARE_PCI_FIXUP_FINAL line below with the specific with
+ * vendor and device of interest to use this quirk.
+ */
+static void quirk_blacklist_vpd(struct pci_dev *dev)
+{
+	if (dev->vpd) {
+		dev->vpd->len = 0;
+		dev_warn(&dev->dev, "PCI vpd access has been disabled due to firmware bug\n");
+	}
+}
+
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0060,
+		quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x007c,
+		quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0413,
+		quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0078,
+		quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0079,
+		quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0073,
+		quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0071,
+		quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x005b,
+		quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x002f,
+		quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x005d,
+		quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x005f,
+		quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, PCI_ANY_ID,
+		quirk_blacklist_vpd);
+
+/*
  * For Broadcom 5706, 5708, 5709 rev. A nics, any read beyond the
  * VPD end tag will hang the device.  This problem was initially
  * observed when a vpd entry was created in sysfs
Signature · 6 lines
1.8.5.6

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tested-by on the v3 series
#6 lore raw
In reply to Hannes Reinecke

For the series tested two PCIe cards that support VPD:

0d:00.0 Serial Attached SCSI controller: LSI Logic / Symbios Logic SAS2308 PCI-Express Fusion-MPT SAS-2 (rev 05) 1e:00.0 Fibre Channel: QLogic Corp. ISP2432-based 4Gb Fibre Channel to PCI Express HBA (rev 03)

The first card exposes 32KiB of NUL bytes as VPD and the second 154 bytes of non-NUL data repeating every 4KiB for 32KiB before the change. After the change the first card returns no data for VPD (since it contains only NUL bytes) and the second the expected 154 bytes of data.

Using dyn debug also tested that this message comes out as expected for the LSI card (and it did):

[  289.539523] mpt3sas 0000:0d:00.0: invalid short VPD tag 00 at offset 1

lspci works correctly with the patch in the kernel. The only change on the system was that with the LSI card lspci with -vvvv as root it now says "Not readable" instead of "Unknown small resource type 00, will not decode more." in the VPD section.
---

Quoted 49 lines

-----Original Message-----
From: [email protected] [mailto:linux-pci- [email protected]] On Behalf Of Hannes Reinecke
Sent: Monday, February 15, 2016 7:42 PM
To: Bjorn Helgaas
Cc: Alexander Duyck; [email protected]; Babu Moger; Jordan Hargrave; Hannes Reinecke
Subject: [PATCHv3 0/4] PCI VPD access fixes

the current PCI VPD page access assumes that the entire possible VPD data is readable. However, the spec only guarantees a VPD data up to the 'end' marker, with everything beyond that being undefined.
This causes a system lockup on certain devices.

With this patch we always set the VPD sysfs attribute size to '0', and calculate the available VPD size on the first access.
If no valid data can be read an I/O error is returned.

I've also included the patch from Babu to blacklists devices which are known to lockup when accessing the VPD data.

Changes to v2:
- Spelling fixes as suggested by Bjorn Helgaas
- Update to correct vpd size in pci_vpc_pci22_write
- Pass in modified VPD size to pci_vpd_pci22_size() to make quirk_brcm_570x_limit_vpd work

Babu Moger (1):
pci: Blacklist vpd access for buggy devices

Hannes Reinecke (3):
pci: Update VPD definitions
pci: allow access to VPD attributes with size '0'
pci: Determine actual VPD size on first access

 drivers/pci/access.c    | 84
+++++++++++++++++++++++++++++++++++++++++++++++--
 drivers/pci/pci-sysfs.c | 22 +++++++------
 drivers/pci/quirks.c    | 43 +++++++++++++++++++++++++
 include/linux/pci.h     | 27 ++++++++++++++--
 4 files changed, 162 insertions(+), 14 deletions(-)

--
1.8.5.6

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected] More majordomo info at
http://vger.kernel.org/majordomo-info.html