[PATCH] genirq/affinity: add helper of irq_affinity_calc_sets

[PATCH] genirq/affinity: add helper of irq_affinity_calc_sets

[Ming Lei]

When driver requests to allocate irq affinity managed vectors,
pci_alloc_irq_vectors_affinity() may fallback to single vector
allocation. In this situation, we don't need to call
irq_create_affinity_masks for calling into ->calc_sets() for
avoiding potential memory leak, so add the helper for this purpose.

Fixes: c66d4bd110a1 ("genirq/affinity: Add new callback for (re)calculating interrupt sets")
Reported-by: Bjorn Helgaas <helgaas@kernel.org>
Cc: linux-pci@vger.kernel.org
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/pci/msi.c         |  3 ++-
 include/linux/interrupt.h |  7 +++++++
 kernel/irq/affinity.c     | 29 ++++++++++++++++++-----------
 3 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 9232255c8515..3d6db20d1b2b 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -1224,7 +1224,8 @@ int pci_alloc_irq_vectors_affinity(struct pci_dev *dev, unsigned int min_vecs,
 			 * for the single interrupt case.
 			 */
 			if (affd)
-				irq_create_affinity_masks(1, affd);
+				WARN_ON_ONCE(irq_affinity_calc_sets(1, affd));
+
 			pci_intx(dev, 1);
 			return 1;
 		}
diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 2ed65b01c961..c7ff84d60465 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -340,6 +340,7 @@ irq_create_affinity_masks(unsigned int nvec, struct irq_affinity *affd);
 
 unsigned int irq_calc_affinity_vectors(unsigned int minvec, unsigned int maxvec,
 				       const struct irq_affinity *affd);
+int irq_affinity_calc_sets(unsigned int affvecs, struct irq_affinity *affd);
 
 #else /* CONFIG_SMP */
 
@@ -391,6 +392,12 @@ irq_calc_affinity_vectors(unsigned int minvec, unsigned int maxvec,
 	return maxvec;
 }
 
+static inline int irq_affinity_calc_sets(unsigned int affvecs,
+					 struct irq_affinity *affd)
+{
+	return 0;
+}
+
 #endif /* CONFIG_SMP */
 
 /*
diff --git a/kernel/irq/affinity.c b/kernel/irq/affinity.c
index 4d89ad4fae3b..735f697d7d15 100644
--- a/kernel/irq/affinity.c
+++ b/kernel/irq/affinity.c
@@ -405,6 +405,23 @@ static void default_calc_sets(struct irq_affinity *affd, unsigned int affvecs)
 	affd->set_size[0] = affvecs;
 }
 
+int irq_affinity_calc_sets(unsigned int affvecs, struct irq_affinity *affd)
+{
+	/*
+	 * Simple invocations do not provide a calc_sets() callback. Install
+	 * the generic one.
+	 */
+	if (!affd->calc_sets)
+		affd->calc_sets = default_calc_sets;
+
+	/* Recalculate the sets */
+	affd->calc_sets(affd, affvecs);
+
+	if (affd->nr_sets > IRQ_AFFINITY_MAX_SETS)
+		return -ERANGE;
+	return 0;
+}
+
 /**
  * irq_create_affinity_masks - Create affinity masks for multiqueue spreading
  * @nvecs:	The total number of vectors
@@ -429,17 +446,7 @@ irq_create_affinity_masks(unsigned int nvecs, struct irq_affinity *affd)
 	else
 		affvecs = 0;
 
-	/*
-	 * Simple invocations do not provide a calc_sets() callback. Install
-	 * the generic one.
-	 */
-	if (!affd->calc_sets)
-		affd->calc_sets = default_calc_sets;
-
-	/* Recalculate the sets */
-	affd->calc_sets(affd, affvecs);
-
-	if (WARN_ON_ONCE(affd->nr_sets > IRQ_AFFINITY_MAX_SETS))
+	if (WARN_ON_ONCE(irq_affinity_calc_sets(affvecs, affd)))
 		return NULL;
 
 	/* Nothing to assign? */
-- 
2.31.1

Re: [PATCH] genirq/affinity: add helper of irq_affinity_calc_sets

[Bjorn Helgaas]

On Thu, Jul 15, 2021 at 07:18:27PM +0800, Ming Lei wrote:
> When driver requests to allocate irq affinity managed vectors,
> pci_alloc_irq_vectors_affinity() may fallback to single vector
> allocation. In this situation, we don't need to call
> irq_create_affinity_masks for calling into ->calc_sets() for
> avoiding potential memory leak, so add the helper for this purpose.
> 
> Fixes: c66d4bd110a1 ("genirq/affinity: Add new callback for (re)calculating interrupt sets")
> Reported-by: Bjorn Helgaas <helgaas@kernel.org>
> Cc: linux-pci@vger.kernel.org
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  drivers/pci/msi.c         |  3 ++-
>  include/linux/interrupt.h |  7 +++++++
>  kernel/irq/affinity.c     | 29 ++++++++++++++++++-----------
>  3 files changed, 27 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 9232255c8515..3d6db20d1b2b 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1224,7 +1224,8 @@ int pci_alloc_irq_vectors_affinity(struct pci_dev *dev, unsigned int min_vecs,
>  			 * for the single interrupt case.
>  			 */
>  			if (affd)
> -				irq_create_affinity_masks(1, affd);
> +				WARN_ON_ONCE(irq_affinity_calc_sets(1, affd));

Hmmm.  Not sure I like this yet:

  - I prefer required code to be on its own, not hidden inside a
    WARN() (personal preference, I know).

  - WARN() doesn't seem like the right thing here.  I think this
    generates a backtrace but the driver that called this has no
    indication.  Isn't the problem that a .calc_sets() method set
    "affd->nr_sets > IRQ_AFFINITY_MAX_SETS"?

    It looks like those methods are supplied by drivers
    (nvme_calc_irq_sets(), csio_calc_sets()) and it seems like they
    should find out about this somehow.

>  			pci_intx(dev, 1);
>  			return 1;
>  		}
> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> index 2ed65b01c961..c7ff84d60465 100644
> --- a/include/linux/interrupt.h
> +++ b/include/linux/interrupt.h
> @@ -340,6 +340,7 @@ irq_create_affinity_masks(unsigned int nvec, struct irq_affinity *affd);
>  
>  unsigned int irq_calc_affinity_vectors(unsigned int minvec, unsigned int maxvec,
>  				       const struct irq_affinity *affd);
> +int irq_affinity_calc_sets(unsigned int affvecs, struct irq_affinity *affd);
>  
>  #else /* CONFIG_SMP */
>  
> @@ -391,6 +392,12 @@ irq_calc_affinity_vectors(unsigned int minvec, unsigned int maxvec,
>  	return maxvec;
>  }
>  
> +static inline int irq_affinity_calc_sets(unsigned int affvecs,
> +					 struct irq_affinity *affd)
> +{
> +	return 0;
> +}
> +
>  #endif /* CONFIG_SMP */
>  
>  /*
> diff --git a/kernel/irq/affinity.c b/kernel/irq/affinity.c
> index 4d89ad4fae3b..735f697d7d15 100644
> --- a/kernel/irq/affinity.c
> +++ b/kernel/irq/affinity.c
> @@ -405,6 +405,23 @@ static void default_calc_sets(struct irq_affinity *affd, unsigned int affvecs)
>  	affd->set_size[0] = affvecs;
>  }
>  
> +int irq_affinity_calc_sets(unsigned int affvecs, struct irq_affinity *affd)
> +{
> +	/*
> +	 * Simple invocations do not provide a calc_sets() callback. Install
> +	 * the generic one.
> +	 */
> +	if (!affd->calc_sets)
> +		affd->calc_sets = default_calc_sets;
> +
> +	/* Recalculate the sets */
> +	affd->calc_sets(affd, affvecs);
> +
> +	if (affd->nr_sets > IRQ_AFFINITY_MAX_SETS)
> +		return -ERANGE;
> +	return 0;
> +}
> +
>  /**
>   * irq_create_affinity_masks - Create affinity masks for multiqueue spreading
>   * @nvecs:	The total number of vectors
> @@ -429,17 +446,7 @@ irq_create_affinity_masks(unsigned int nvecs, struct irq_affinity *affd)
>  	else
>  		affvecs = 0;
>  
> -	/*
> -	 * Simple invocations do not provide a calc_sets() callback. Install
> -	 * the generic one.
> -	 */
> -	if (!affd->calc_sets)
> -		affd->calc_sets = default_calc_sets;
> -
> -	/* Recalculate the sets */
> -	affd->calc_sets(affd, affvecs);
> -
> -	if (WARN_ON_ONCE(affd->nr_sets > IRQ_AFFINITY_MAX_SETS))
> +	if (WARN_ON_ONCE(irq_affinity_calc_sets(affvecs, affd)))
>  		return NULL;
>  
>  	/* Nothing to assign? */
> -- 
> 2.31.1
> 

Re: [PATCH] genirq/affinity: add helper of irq_affinity_calc_sets

[Ming Lei]

On Thu, Jul 15, 2021 at 09:27:14AM -0500, Bjorn Helgaas wrote:
> On Thu, Jul 15, 2021 at 07:18:27PM +0800, Ming Lei wrote:
> > When driver requests to allocate irq affinity managed vectors,
> > pci_alloc_irq_vectors_affinity() may fallback to single vector
> > allocation. In this situation, we don't need to call
> > irq_create_affinity_masks for calling into ->calc_sets() for
> > avoiding potential memory leak, so add the helper for this purpose.
> > 
> > Fixes: c66d4bd110a1 ("genirq/affinity: Add new callback for (re)calculating interrupt sets")
> > Reported-by: Bjorn Helgaas <helgaas@kernel.org>
> > Cc: linux-pci@vger.kernel.org
> > Cc: Christoph Hellwig <hch@lst.de>
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  drivers/pci/msi.c         |  3 ++-
> >  include/linux/interrupt.h |  7 +++++++
> >  kernel/irq/affinity.c     | 29 ++++++++++++++++++-----------
> >  3 files changed, 27 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > index 9232255c8515..3d6db20d1b2b 100644
> > --- a/drivers/pci/msi.c
> > +++ b/drivers/pci/msi.c
> > @@ -1224,7 +1224,8 @@ int pci_alloc_irq_vectors_affinity(struct pci_dev *dev, unsigned int min_vecs,
> >  			 * for the single interrupt case.
> >  			 */
> >  			if (affd)
> > -				irq_create_affinity_masks(1, affd);
> > +				WARN_ON_ONCE(irq_affinity_calc_sets(1, affd));
> 
> Hmmm.  Not sure I like this yet:
> 
>   - I prefer required code to be on its own, not hidden inside a
>     WARN() (personal preference, I know).
> 
>   - WARN() doesn't seem like the right thing here.  I think this
>     generates a backtrace but the driver that called this has no
>     indication.  Isn't the problem that a .calc_sets() method set
>     "affd->nr_sets > IRQ_AFFINITY_MAX_SETS"?

Yes. When the warning is triggered, memory corruption may have been caused,
not sure if the indication is needed.

> 
>     It looks like those methods are supplied by drivers
>     (nvme_calc_irq_sets(), csio_calc_sets()) and it seems like they
>     should find out about this somehow.

Yeah. The WARN() here is just to report the bug earlier.


Thanks, 
Ming

Re: [PATCH] genirq/affinity: add helper of irq_affinity_calc_sets

[Christoph Hellwig]

On Thu, Jul 15, 2021 at 07:18:27PM +0800, Ming Lei wrote:
> +				WARN_ON_ONCE(irq_affinity_calc_sets(1, affd));

Hiding actual functionality inside a WARN_ON is nasty.

> +int irq_affinity_calc_sets(unsigned int affvecs, struct irq_affinity *affd)
> +{
> +	/*
> +	 * Simple invocations do not provide a calc_sets() callback. Install
> +	 * the generic one.
> +	 */
> +	if (!affd->calc_sets)
> +		affd->calc_sets = default_calc_sets;
> +
> +	/* Recalculate the sets */
> +	affd->calc_sets(affd, affvecs);

I'm not sure a function like this should have side effects.  Either
we move the assign to an init function with a single caller, or do an
if / else here.