revert bab65e48cb064 PCI/MSI Sanitize MSI-X checks

The change in bab65e48cb064 breaks pci_enable_msix_range(). The intent is to optimise the sanity checks, but it is somewhat overenthusiastic.

The interface allows you to ask for a lot of vectors and returns the number that were allocated. However, after the change, you can't request a vector that is higher than the largest the hardware supports. Which makes that rather pointless.

So code like:

	for (i = 0; i < 16; i++)
		msix_tbl[i].entry = i;
	nvec = pci_enable_msix_range(dev, msix_tbl, 1, 16);

Now returns -22 if the hardware only supports 8 interrupts.

Previously it returned 8.

I can fix my driver, but I suspect that any code that relies on a smaller number of vectors being returned is now broken.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)

[+cc linux-pci, regressions]

Thanks for the report! bab65e48cb06 ("PCI/MSI: Sanitize MSI-X checks") appeared in v6.2-rc1, so this is a recent regression and it would be good to fix it for v6.3.

bab65e48cb06 only touches drivers/pci/msi/msi.c, but since it didn't go through the PCI tree, I'll let Thomas handle any revert (or better, an improvement to pci_msix_validate_entries()) since he wrote and applied the original.

Bjorn

From: Bjorn Helgaas

I do try to test every release at around rc3.

Looking it:

static bool pci_msix_validate_entries(struct pci_dev *dev, struct msix_entry *entries,
int nvec, int hwsize)

{
	bool nogap;
	int i, j;

	if (!entries)
		return true;

	nogap = pci_msi_domain_supports(dev, MSI_FLAG_MSIX_CONTIGUOUS, DENY_LEGACY);

	for (i = 0; i < nvec; i++) {
		/* Entry within hardware limit? */
		if (entries[i].entry >= hwsize)
			return false;

		/* Check for duplicate entries */
		for (j = i + 1; j < nvec; j++) {
			if (entries[i].entry == entries[j].entry)
				return false;
		}
		/* Check for unsupported gaps */
		if (nogap && entries[i].entry != i)
			return false;
	}
	return true;
}

It probably needs to return an updated 'nvec'. The gap/duplicate check is also a bit horrid, why not:

		if (nogap) {
			if (entries[i].entry != i)
				return false;
			continue;
		}

		if (!i || entries[i].entry > entries[i - 1].entry)
			continue;

		horrid, expensive loop...

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)

Right. The fix is trivial as the hardware size check in this validation function is pointless.

The point is that for a range allocation with and entries array, _all_ entries up to max_vec must be correct independent of the actual hardware size.

So the fix is simply removing the hardware size check from the validation function.

The hardware size checking happens afterwards anyway.

Thanks,

tglx

---
 drivers/pci/msi/msi.c |    9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

@@ -750,8 +750,7 @@ static int msix_capability_init(struct p
 	return ret;
 }
 
-static bool pci_msix_validate_entries(struct pci_dev *dev, struct msix_entry *entries,
-				      int nvec, int hwsize)
+static bool pci_msix_validate_entries(struct pci_dev *dev, struct msix_entry *entries, int nvev)
 {
 	bool nogap;
 	int i, j;

@@ -762,10 +761,6 @@ static bool pci_msix_validate_entries(st
 	nogap = pci_msi_domain_supports(dev, MSI_FLAG_MSIX_CONTIGUOUS, DENY_LEGACY);
 
 	for (i = 0; i < nvec; i++) {
-		/* Entry within hardware limit? */
-		if (entries[i].entry >= hwsize)
-			return false;
-
 		/* Check for duplicate entries */
 		for (j = i + 1; j < nvec; j++) {
 			if (entries[i].entry == entries[j].entry)

@@ -805,7 +800,7 @@ int __pci_enable_msix_range(struct pci_d
 	if (hwsize < 0)
 		return hwsize;
 
-	if (!pci_msix_validate_entries(dev, entries, nvec, hwsize))
+	if (!pci_msix_validate_entries(dev, entries, nvec))
 		return -EINVAL;
 
 	if (hwsize < nvec) {

Does just moving the pci_msix_validate_entries() down to below the hwsize update code fix it?

IOW, something like this attached patch?

ENTIRELY UNTESTED! This may be seriously broken for some reason, but it does seem like the current code makes no sense (that "Keep the IRQ virtual hackery working" comment seems to not possibly be true since the MSIX nvec has effectively been checked against hwsize by the pci_msix_validate_entries() code before).

I don't know this code. Thomas?

Linus

 drivers/pci/msi/msi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index 1f716624ca56..8edc7beebf6f 100644
@@ -805,9 +805,6 @@ int __pci_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries, int
 	if (hwsize < 0)
 		return hwsize;
 
-	if (!pci_msix_validate_entries(dev, entries, nvec, hwsize))
-		return -EINVAL;
-
 	if (hwsize < nvec) {
 		/* Keep the IRQ virtual hackery working */
 		if (flags & PCI_IRQ_VIRTUAL)

@@ -819,6 +816,9 @@ int __pci_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries, int
 	if (nvec < minvec)
 		return -ENOSPC;
 
+	if (!pci_msix_validate_entries(dev, entries, nvec, hwsize))
+		return -EINVAL;
+
 	rc = pci_setup_msi_context(dev);
 	if (rc)
 		return rc;

Yes, that works too. But I rather remove the hwsize check from the validation function as I explained in my earlier reply to Bjorn in that thread.

Thanks,

tglx

From: Linus Torvalds

I think it depends on why the driver is asking for more interrupts than the hardware supports. Potentially the driver could do:

	for (i = 0; i < 8; i++)
		msix_tbl[i].entry = 2 * i;

if the hardware supports 8 interrupts perhaps it
should return 4?

In my case both the MSI-X logic on the fpga and the driver support 16 interrupts, but PCIe config space reports 8. The high numbered interrupts aren't used very often and get multiplexed onto the highest 'real' interrupt. (The fpga build makes it pretty much impossible to tie together the config space value and the size of the MSI-X table.)

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)

Hmm.

Something like this might get that case right too. Again - entirely untested, but looks superficially sane to me.

It just returns how many of the msix_entry[] entries can be used (or negative for error). So then the (only) caller can just say "is that still enough for what we required". Seems reasonable, and would get that non-contiguous case right, I think.

Thomas?

I'm going to drop this and assume I'll get a pull from you with what you consider the proper fix, but since I was looking at this anyway, I decided I might as well send out this RFC patch.

Linus

 drivers/pci/msi/msi.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

index 1f716624ca56..d151bde8b8e0 100644
@@ -750,32 +750,33 @@ static int msix_capability_init(struct pci_dev *dev, struct msix_entry *entries,
 	return ret;
 }
 
-static bool pci_msix_validate_entries(struct pci_dev *dev, struct msix_entry *entries,
-				      int nvec, int hwsize)
+static int pci_msix_validate_entries(struct pci_dev *dev, struct msix_entry *entries,
+				     int nvec, int hwsize)
 {
 	bool nogap;
-	int i, j;
+	int maxvec;
 
 	if (!entries)
-		return true;
+		return nvec;
 
 	nogap = pci_msi_domain_supports(dev, MSI_FLAG_MSIX_CONTIGUOUS, DENY_LEGACY);
 
-	for (i = 0; i < nvec; i++) {
+	maxvec = -EINVAL;
+	for (int i = 0; i < nvec; i++) {
 		/* Entry within hardware limit? */
-		if (entries[i].entry >= hwsize)
-			return false;
+		if (entries[i].entry < hwsize)
+			maxvec = i+1;
 
 		/* Check for duplicate entries */
-		for (j = i + 1; j < nvec; j++) {
+		for (int j = i + 1; j < nvec; j++) {
 			if (entries[i].entry == entries[j].entry)
-				return false;
+				return -EINVAL;
 		}
 		/* Check for unsupported gaps */
 		if (nogap && entries[i].entry != i)
-			return false;
+			return -EINVAL;
 	}
-	return true;
+	return maxvec;
 }
 
 int __pci_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries, int minvec,

@@ -805,8 +806,11 @@ int __pci_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries, int
 	if (hwsize < 0)
 		return hwsize;
 
-	if (!pci_msix_validate_entries(dev, entries, nvec, hwsize))
-		return -EINVAL;
+	nvec = pci_msix_validate_entries(dev, entries, nvec, hwsize);
+	if (nvec < 0)
+		return nvec;
+	if (nvec < minvec)
+		return -ENOSPC;
 
 	if (hwsize < nvec) {
 		/* Keep the IRQ virtual hackery working */

Linus!

Probably, but the point is that a driver should not hand in invalid data in the first place. Even if the hardware does not provide enough space for the requested maximum range then the handed in entries array should not contain any invalid data, right?

Ergo the hwsize check in that function is bogus. No idea what I was thinking there.

So the simple and IMO correct solution is to drop that hwsize check from the validation function and validate up to the

The point is that for a range allocation with and entries array, _all_ entries up to max_vec must be correct independent of the actual hardware size.

So the fix is simply removing the hardware size check from the validation function. The hardware size checking happens afterwards anyway.

Let me write up a proper changelog for that.

Thanks,

tglx

---
 drivers/pci/msi/msi.c |    9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

@@ -750,8 +750,7 @@ static int msix_capability_init(struct p

	return ret;

}

-static bool pci_msix_validate_entries(struct pci_dev *dev, struct msix_entry *entries,
- int nvec, int hwsize)
+static bool pci_msix_validate_entries(struct pci_dev *dev, struct msix_entry *entries, int nvev)

 {
	bool nogap;
	int i, j;
@@ -762,10 +761,6 @@ static bool pci_msix_validate_entries(st
	nogap = pci_msi_domain_supports(dev, MSI_FLAG_MSIX_CONTIGUOUS, DENY_LEGACY);

	for (i = 0; i < nvec; i++) {

- /* Entry within hardware limit? */
- if (entries[i].entry >= hwsize)
- return false;
-

		/* Check for duplicate entries */
		for (j = i + 1; j < nvec; j++) {
			if (entries[i].entry == entries[j].entry)
@@ -805,7 +800,7 @@ int __pci_enable_msix_range(struct pci_d
	if (hwsize < 0)
		return hwsize;

- if (!pci_msix_validate_entries(dev, entries, nvec, hwsize))
+ if (!pci_msix_validate_entries(dev, entries, nvec))

		return -EINVAL;

	if (hwsize < nvec) {

pci_msix_validate_entries() validates the entries array which is handed in by the caller for a MSI-X interrupt allocation. Aside of consistency failures it also detects a failure when the size of the MSI-X hardware table in the device is smaller than the size of the entries array.

That's wrong for the case of range allocations where the caller provides the minimum and the maximum number of vectors to allocate, when the hardware size is greater or equal to the mininum, but smaller than the maximum.

Remove the hardware size check completely from that function and just ensure that the entires array up to the maximum size is consistent.

The limitation and range checking versus the hardware size happens independently of that afterwards anyway because the entries array is optional.

---

David, can you please confirm that this fixes your issue?

---
 drivers/pci/msi/msi.c |    9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

@@ -750,8 +750,7 @@ static int msix_capability_init(struct p
 	return ret;
 }
 
-static bool pci_msix_validate_entries(struct pci_dev *dev, struct msix_entry *entries,
-				      int nvec, int hwsize)
+static bool pci_msix_validate_entries(struct pci_dev *dev, struct msix_entry *entries, int nvev)
 {
 	bool nogap;
 	int i, j;

@@ -762,10 +761,6 @@ static bool pci_msix_validate_entries(st
 	nogap = pci_msi_domain_supports(dev, MSI_FLAG_MSIX_CONTIGUOUS, DENY_LEGACY);
 
 	for (i = 0; i < nvec; i++) {
-		/* Entry within hardware limit? */
-		if (entries[i].entry >= hwsize)
-			return false;
-
 		/* Check for duplicate entries */
 		for (j = i + 1; j < nvec; j++) {
 			if (entries[i].entry == entries[j].entry)

@@ -805,7 +800,7 @@ int __pci_enable_msix_range(struct pci_d
 	if (hwsize < 0)
 		return hwsize;
 
-	if (!pci_msix_validate_entries(dev, entries, nvec, hwsize))
+	if (!pci_msix_validate_entries(dev, entries, nvec))
 		return -EINVAL;
 
 	if (hwsize < nvec) {