[PATCH] PCI/MSI: don't apply affinity if there aren't enough vectors left

[PATCH] PCI/MSI: don't apply affinity if there aren't enough vectors left

[Christoph Hellwig]

Bart reported a problem wіth an out of bounds access in the low-level
IRQ affinity code, which we root caused to the qla2xxx driver assigning
all it's MSI-X vectors to the pre and post vectors, and not having any
left for the actually spread IRQs.

This fixes this issue by not asking for affinity assignment when there
are not vectors to assign left.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Fixes: 402723ad ("PCI/MSI: Provide pci_alloc_irq_vectors_affinity()")
Reported-by: Bart Van Assche <bart.vanassche@sandisk.com>
Tested-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 drivers/pci/msi.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 50c5003..7f73bac 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -1206,6 +1206,16 @@ int pci_alloc_irq_vectors_affinity(struct pci_dev *dev, unsigned int min_vecs,
 	if (flags & PCI_IRQ_AFFINITY) {
 		if (!affd)
 			affd = &msi_default_affd;
+
+		if (affd->pre_vectors + affd->post_vectors > min_vecs)
+			return -EINVAL;
+
+		/*
+		 * If there aren't any vectors left after applying the pre/post
+		 * vectors don't bother with assigning affinity.
+		 */
+		if (affd->pre_vectors + affd->post_vectors == min_vecs)
+			affd = NULL;
 	} else {
 		if (WARN_ON(affd))
 			affd = NULL;
-- 
2.1.4

Re: [PATCH] PCI/MSI: don't apply affinity if there aren't enough vectors left

[Bjorn Helgaas]

On Mon, Jan 30, 2017 at 01:15:41PM +0100, Christoph Hellwig wrote:
> Bart reported a problem wіth an out of bounds access in the low-level
> IRQ affinity code, which we root caused to the qla2xxx driver assigning
> all it's MSI-X vectors to the pre and post vectors, and not having any
> left for the actually spread IRQs.

s/it's/its/

> This fixes this issue by not asking for affinity assignment when there
> are not vectors to assign left.

s/not/no/

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Fixes: 402723ad ("PCI/MSI: Provide pci_alloc_irq_vectors_affinity()")
> Reported-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Tested-by: Bart Van Assche <bart.vanassche@sandisk.com>
> ---
>  drivers/pci/msi.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 50c5003..7f73bac 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1206,6 +1206,16 @@ int pci_alloc_irq_vectors_affinity(struct pci_dev *dev, unsigned int min_vecs,
>  	if (flags & PCI_IRQ_AFFINITY) {
>  		if (!affd)
>  			affd = &msi_default_affd;
> +
> +		if (affd->pre_vectors + affd->post_vectors > min_vecs)
> +			return -EINVAL;
> +
> +		/*
> +		 * If there aren't any vectors left after applying the pre/post
> +		 * vectors don't bother with assigning affinity.
> +		 */
> +		if (affd->pre_vectors + affd->post_vectors == min_vecs)
> +			affd = NULL;
>  	} else {
>  		if (WARN_ON(affd))
>  			affd = NULL;

You didn't say exactly where the out of bounds access was, but I assume
it's probably in irq_create_affinity_masks() in this path:

  pci_alloc_irq_vectors_affinity(..., affd)
    __pci_enable_msix_range(..., minvec, maxvec, affd)
      # decide on nvec
      irq_calc_affinity_vectors(nvec, affd)
      __pci_enable_msix(..., nvec, affd)
        msix_capability_init(..., nvec, affd)
          msix_setup_entries(..., nvec, affd)
            irq_create_affinity_masks(nvec, affd)
              affv = nvecs - affd->pre_vectors - affd->post_vectors

              masks = kcalloc(nvecs, ...)
              for (curvec = 0; curvec < affd->pre_vectors; ...)
                cpumask_copy(masks + curvec, irq_default_affinity)

The fix in pci_alloc_irq_vectors_affinity() looks fine, but I wish it
were closer to the actual problem.

I think irq_create_affinity_masks() should be a little more careful
with the assumptions it makes about the relationships between nvecs,
affd->pre_vectors, and affd->post_vectors.

For example, it allocates with size "nvec" and iterates with a limit
of "affd->pre_vectors", which assumes "affd->pre_vectors < nvec".  And
of course, "affv" may end up being negative, which can't be good.

If we made irq_create_affinity_masks() more robust, I don't think
you'd need to set affd to NULL in pci_alloc_irq_vectors_affinity().
But maybe you would still want at least the -EINVAL change so the
caller gets an indication that it is doing something wrong?

Bjorn

Re: [PATCH] PCI/MSI: don't apply affinity if there aren't enough vectors left

[Christoph Hellwig]

On Thu, Feb 02, 2017 at 11:36:59AM -0600, Bjorn Helgaas wrote:
> You didn't say exactly where the out of bounds access was, but I assume
> it's probably in irq_create_affinity_masks() in this path:

Yes.  See the original report from Bart here:

http://www.spinics.net/lists/linux-scsi/msg104082.html

> The fix in pci_alloc_irq_vectors_affinity() looks fine, but I wish it
> were closer to the actual problem.

I plan to also fix the low-level issue, but:

 a) I have other patches pending in that are for 4.11 and I'd like
    to batch them up with those to avoid conflicts
 b) we really need a high-level check like the one added in this
    patch so that we do the right thing (drop affinity) in this
    case instead of returning an error to the caller and failing
    the probe.

Re: [PATCH] PCI/MSI: don't apply affinity if there aren't enough vectors left

[Bjorn Helgaas]

On Thu, Feb 02, 2017 at 07:32:16PM +0100, Christoph Hellwig wrote:
> On Thu, Feb 02, 2017 at 11:36:59AM -0600, Bjorn Helgaas wrote:
> > You didn't say exactly where the out of bounds access was, but I assume
> > it's probably in irq_create_affinity_masks() in this path:
> 
> Yes.  See the original report from Bart here:
> 
> http://www.spinics.net/lists/linux-scsi/msg104082.html
> 
> > The fix in pci_alloc_irq_vectors_affinity() looks fine, but I wish it
> > were closer to the actual problem.
> 
> I plan to also fix the low-level issue, but:
> 
>  a) I have other patches pending in that are for 4.11 and I'd like
>     to batch them up with those to avoid conflicts
>  b) we really need a high-level check like the one added in this
>     patch so that we do the right thing (drop affinity) in this
>     case instead of returning an error to the caller and failing
>     the probe.

OK.  I applied this to for-linus for v4.10.

I think b) refers to this piece:

  +   /*
  +    * If there aren't any vectors left after applying the pre/post
  +    * vectors don't bother with assigning affinity.
  +    */
  +   if (affd->pre_vectors + affd->post_vectors == min_vecs)
  +           affd = NULL;

I think this can be (and really should be) fixed in
irq_create_affinity_masks(), which can also ignore affinity without
returning an error to the caller of pci_alloc_irq_vectors_affinity().
But I guess it's ok to also check here.

Thanks for reminding me to pick this up for v4.10.

Bjorn