[PATCH net, 0/2] Fix usage of irq affinity_hint

[PATCH net, 0/2] Fix usage of irq affinity_hint

[Haiyang Zhang]

Fix 2 bugs of irq affinity_hint usage in the mana driver

Haiyang Zhang (2):
  net: mana: Fix hint value before free irq
  net: mana: Fix accessing freed irq affinity_hint

 .../net/ethernet/microsoft/mana/gdma_main.c   | 45 +++++++++++--------
 include/net/mana/gdma.h                       |  1 +
 2 files changed, 28 insertions(+), 18 deletions(-)

-- 
2.25.1

[PATCH net, 1/2] net: mana: Fix hint value before free irq

[Haiyang Zhang]

Need to clear affinity_hint before free_irq(), otherwise there is a
one-time warning and stack trace during module unloading.

To fix this bug, set affinity_hint to NULL before free as required.

Cc: stable@vger.kernel.org
Fixes: 71fa6887eeca ("net: mana: Assign interrupts to CPUs based on NUMA nodes")
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/net/ethernet/microsoft/mana/gdma_main.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
index b144f2237748..3bae9d4c1f08 100644
--- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
+++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
@@ -1297,6 +1297,8 @@ static int mana_gd_setup_irqs(struct pci_dev *pdev)
 	for (j = i - 1; j >= 0; j--) {
 		irq = pci_irq_vector(pdev, j);
 		gic = &gc->irq_contexts[j];
+
+		irq_update_affinity_hint(irq, NULL);
 		free_irq(irq, gic);
 	}
 
@@ -1324,6 +1326,9 @@ static void mana_gd_remove_irqs(struct pci_dev *pdev)
 			continue;
 
 		gic = &gc->irq_contexts[i];
+
+		/* Need to clear the hint before free_irq */
+		irq_update_affinity_hint(irq, NULL);
 		free_irq(irq, gic);
 	}
 
-- 
2.25.1

Re: [PATCH net, 1/2] net: mana: Fix hint value before free irq

[Leon Romanovsky]

On Thu, Jan 26, 2023 at 01:04:44PM -0800, Haiyang Zhang wrote:
> Need to clear affinity_hint before free_irq(), otherwise there is a
> one-time warning and stack trace during module unloading.

Please add this warning and stack trace to the commit message.

Thanks

> 
> To fix this bug, set affinity_hint to NULL before free as required.
> 
> Cc: stable@vger.kernel.org
> Fixes: 71fa6887eeca ("net: mana: Assign interrupts to CPUs based on NUMA nodes")
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
>  drivers/net/ethernet/microsoft/mana/gdma_main.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index b144f2237748..3bae9d4c1f08 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -1297,6 +1297,8 @@ static int mana_gd_setup_irqs(struct pci_dev *pdev)
>  	for (j = i - 1; j >= 0; j--) {
>  		irq = pci_irq_vector(pdev, j);
>  		gic = &gc->irq_contexts[j];
> +
> +		irq_update_affinity_hint(irq, NULL);
>  		free_irq(irq, gic);
>  	}
>  
> @@ -1324,6 +1326,9 @@ static void mana_gd_remove_irqs(struct pci_dev *pdev)
>  			continue;
>  
>  		gic = &gc->irq_contexts[i];
> +
> +		/* Need to clear the hint before free_irq */
> +		irq_update_affinity_hint(irq, NULL);
>  		free_irq(irq, gic);
>  	}
>  
> -- 
> 2.25.1
> 

RE: [PATCH net, 1/2] net: mana: Fix hint value before free irq

[Haiyang Zhang]

> -----Original Message-----
> From: Leon Romanovsky <leon@kernel.org>
> Sent: Sunday, January 29, 2023 4:28 AM
> To: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: linux-hyperv@vger.kernel.org; netdev@vger.kernel.org; Dexuan Cui
> <decui@microsoft.com>; KY Srinivasan <kys@microsoft.com>; Paul Rosswurm
> <paulros@microsoft.com>; olaf@aepfle.de; vkuznets@redhat.com;
> davem@davemloft.net; linux-kernel@vger.kernel.org; stable@vger.kernel.org
> Subject: Re: [PATCH net, 1/2] net: mana: Fix hint value before free irq
> 
> On Thu, Jan 26, 2023 at 01:04:44PM -0800, Haiyang Zhang wrote:
> > Need to clear affinity_hint before free_irq(), otherwise there is a
> > one-time warning and stack trace during module unloading.
> 
> Please add this warning and stack trace to the commit message.

Will do.

Thanks,
- Haiyang

RE: [PATCH net, 1/2] net: mana: Fix hint value before free irq

[Michael Kelley (LINUX)]

From: LKML haiyangz <lkmlhyz@microsoft.com> On Behalf Of Haiyang Zhang Sent: Thursday, January 26, 2023 1:05 PM
> 
> Need to clear affinity_hint before free_irq(), otherwise there is a
> one-time warning and stack trace during module unloading.
> 
> To fix this bug, set affinity_hint to NULL before free as required.
> 
> Cc: stable@vger.kernel.org
> Fixes: 71fa6887eeca ("net: mana: Assign interrupts to CPUs based on NUMA nodes")
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
>  drivers/net/ethernet/microsoft/mana/gdma_main.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index b144f2237748..3bae9d4c1f08 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -1297,6 +1297,8 @@ static int mana_gd_setup_irqs(struct pci_dev *pdev)
>  	for (j = i - 1; j >= 0; j--) {
>  		irq = pci_irq_vector(pdev, j);
>  		gic = &gc->irq_contexts[j];
> +
> +		irq_update_affinity_hint(irq, NULL);
>  		free_irq(irq, gic);
>  	}
> 
> @@ -1324,6 +1326,9 @@ static void mana_gd_remove_irqs(struct pci_dev *pdev)
>  			continue;
> 
>  		gic = &gc->irq_contexts[i];
> +
> +		/* Need to clear the hint before free_irq */
> +		irq_update_affinity_hint(irq, NULL);
>  		free_irq(irq, gic);
>  	}
> 
> --
> 2.25.1

I think this patch should be folded into the second patch of this series.  While
this patch makes the warning go away, it doesn't really solve any problems by
itself.  It just papers over the warning.  My first reaction on seeing this patch
is that the warning exists because the memory for the mask likely had
been incorrectly managed, which is exactly the case.  Since this patch doesn't
really fix a problem on its own, I'd say merge it into the second patch.

That's just my $.02.  If you really want to keep it separate, that's not a
blocker for me.

Michael

RE: [PATCH net, 1/2] net: mana: Fix hint value before free irq

[Haiyang Zhang]

> -----Original Message-----
> From: Michael Kelley (LINUX) <mikelley@microsoft.com>
> Sent: Sunday, January 29, 2023 9:26 AM
> To: Haiyang Zhang <haiyangz@microsoft.com>; linux-hyperv@vger.kernel.org;
> netdev@vger.kernel.org
> Cc: Haiyang Zhang <haiyangz@microsoft.com>; Dexuan Cui
> <decui@microsoft.com>; KY Srinivasan <kys@microsoft.com>; Paul Rosswurm
> <paulros@microsoft.com>; olaf@aepfle.de; vkuznets@redhat.com;
> davem@davemloft.net; linux-kernel@vger.kernel.org; stable@vger.kernel.org
> Subject: RE: [PATCH net, 1/2] net: mana: Fix hint value before free irq
> 
> From: LKML haiyangz <lkmlhyz@microsoft.com> On Behalf Of Haiyang Zhang
> Sent: Thursday, January 26, 2023 1:05 PM
> >
> > Need to clear affinity_hint before free_irq(), otherwise there is a
> > one-time warning and stack trace during module unloading.
> >
> > To fix this bug, set affinity_hint to NULL before free as required.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: 71fa6887eeca ("net: mana: Assign interrupts to CPUs based on NUMA
> nodes")
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > ---
> >  drivers/net/ethernet/microsoft/mana/gdma_main.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> > b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> > index b144f2237748..3bae9d4c1f08 100644
> > --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> > +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> > @@ -1297,6 +1297,8 @@ static int mana_gd_setup_irqs(struct pci_dev
> *pdev)
> >  	for (j = i - 1; j >= 0; j--) {
> >  		irq = pci_irq_vector(pdev, j);
> >  		gic = &gc->irq_contexts[j];
> > +
> > +		irq_update_affinity_hint(irq, NULL);
> >  		free_irq(irq, gic);
> >  	}
> >
> > @@ -1324,6 +1326,9 @@ static void mana_gd_remove_irqs(struct pci_dev
> *pdev)
> >  			continue;
> >
> >  		gic = &gc->irq_contexts[i];
> > +
> > +		/* Need to clear the hint before free_irq */
> > +		irq_update_affinity_hint(irq, NULL);
> >  		free_irq(irq, gic);
> >  	}
> >
> > --
> > 2.25.1
> 
> I think this patch should be folded into the second patch of this series.  While
> this patch makes the warning go away, it doesn't really solve any problems by
> itself.  It just papers over the warning.  My first reaction on seeing this patch
> is that the warning exists because the memory for the mask likely had
> been incorrectly managed, which is exactly the case.  Since this patch doesn't
> really fix a problem on its own, I'd say merge it into the second patch.
> 
> That's just my $.02.  If you really want to keep it separate, that's not a
> blocker for me.

Will do.

Thanks,
- Haiyang

[PATCH net, 2/2] net: mana: Fix accessing freed irq affinity_hint

[Haiyang Zhang]

After calling irq_set_affinity_and_hint(), the cpumask pointer is
saved in desc->affinity_hint, and will be used later when reading
/proc/irq/<num>/affinity_hint. So the cpumask variable needs to be
allocated per irq, and available until freeing the irq. Otherwise,
we are accessing freed memory when reading the affinity_hint file.

To fix the bug, allocate the cpumask per irq, and free it just
before freeing the irq.

Cc: stable@vger.kernel.org
Fixes: 71fa6887eeca ("net: mana: Assign interrupts to CPUs based on NUMA nodes")
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 .../net/ethernet/microsoft/mana/gdma_main.c   | 40 ++++++++++---------
 include/net/mana/gdma.h                       |  1 +
 2 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
index 3bae9d4c1f08..37473ae3859c 100644
--- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
+++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
@@ -1219,7 +1219,6 @@ static int mana_gd_setup_irqs(struct pci_dev *pdev)
 	struct gdma_irq_context *gic;
 	unsigned int max_irqs;
 	u16 *cpus;
-	cpumask_var_t req_mask;
 	int nvec, irq;
 	int err, i = 0, j;
 
@@ -1240,25 +1239,26 @@ static int mana_gd_setup_irqs(struct pci_dev *pdev)
 		goto free_irq_vector;
 	}
 
-	if (!zalloc_cpumask_var(&req_mask, GFP_KERNEL)) {
-		err = -ENOMEM;
-		goto free_irq;
-	}
-
 	cpus = kcalloc(nvec, sizeof(*cpus), GFP_KERNEL);
 	if (!cpus) {
 		err = -ENOMEM;
-		goto free_mask;
+		goto free_gic;
 	}
 	for (i = 0; i < nvec; i++)
 		cpus[i] = cpumask_local_spread(i, gc->numa_node);
 
 	for (i = 0; i < nvec; i++) {
-		cpumask_set_cpu(cpus[i], req_mask);
 		gic = &gc->irq_contexts[i];
 		gic->handler = NULL;
 		gic->arg = NULL;
 
+		if (!zalloc_cpumask_var(&gic->cpu_hint, GFP_KERNEL)) {
+			err = -ENOMEM;
+			goto free_irq;
+		}
+
+		cpumask_set_cpu(cpus[i], gic->cpu_hint);
+
 		if (!i)
 			snprintf(gic->name, MANA_IRQ_NAME_SZ, "mana_hwc@pci:%s",
 				 pci_name(pdev));
@@ -1269,17 +1269,18 @@ static int mana_gd_setup_irqs(struct pci_dev *pdev)
 		irq = pci_irq_vector(pdev, i);
 		if (irq < 0) {
 			err = irq;
-			goto free_mask;
+			free_cpumask_var(gic->cpu_hint);
+			goto free_irq;
 		}
 
 		err = request_irq(irq, mana_gd_intr, 0, gic->name, gic);
-		if (err)
-			goto free_mask;
-		irq_set_affinity_and_hint(irq, req_mask);
-		cpumask_clear(req_mask);
+		if (err) {
+			free_cpumask_var(gic->cpu_hint);
+			goto free_irq;
+		}
+
+		irq_set_affinity_and_hint(irq, gic->cpu_hint);
 	}
-	free_cpumask_var(req_mask);
-	kfree(cpus);
 
 	err = mana_gd_alloc_res_map(nvec, &gc->msix_resource);
 	if (err)
@@ -1288,20 +1289,22 @@ static int mana_gd_setup_irqs(struct pci_dev *pdev)
 	gc->max_num_msix = nvec;
 	gc->num_msix_usable = nvec;
 
+	kfree(cpus);
 	return 0;
 
-free_mask:
-	free_cpumask_var(req_mask);
-	kfree(cpus);
 free_irq:
 	for (j = i - 1; j >= 0; j--) {
 		irq = pci_irq_vector(pdev, j);
 		gic = &gc->irq_contexts[j];
 
 		irq_update_affinity_hint(irq, NULL);
+		free_cpumask_var(gic->cpu_hint);
 		free_irq(irq, gic);
 	}
 
+	kfree(cpus);
+
+free_gic:
 	kfree(gc->irq_contexts);
 	gc->irq_contexts = NULL;
 free_irq_vector:
@@ -1329,6 +1332,7 @@ static void mana_gd_remove_irqs(struct pci_dev *pdev)
 
 		/* Need to clear the hint before free_irq */
 		irq_update_affinity_hint(irq, NULL);
+		free_cpumask_var(gic->cpu_hint);
 		free_irq(irq, gic);
 	}
 
diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
index 56189e4252da..4dcafecbd89e 100644
--- a/include/net/mana/gdma.h
+++ b/include/net/mana/gdma.h
@@ -342,6 +342,7 @@ struct gdma_irq_context {
 	void (*handler)(void *arg);
 	void *arg;
 	char name[MANA_IRQ_NAME_SZ];
+	cpumask_var_t cpu_hint;
 };
 
 struct gdma_context {
-- 
2.25.1

Re: [PATCH net, 2/2] net: mana: Fix accessing freed irq affinity_hint

[Leon Romanovsky]

On Thu, Jan 26, 2023 at 01:04:45PM -0800, Haiyang Zhang wrote:
> After calling irq_set_affinity_and_hint(), the cpumask pointer is
> saved in desc->affinity_hint, and will be used later when reading
> /proc/irq/<num>/affinity_hint. So the cpumask variable needs to be
> allocated per irq, and available until freeing the irq. Otherwise,
> we are accessing freed memory when reading the affinity_hint file.
> 
> To fix the bug, allocate the cpumask per irq, and free it just
> before freeing the irq.
> 
> Cc: stable@vger.kernel.org
> Fixes: 71fa6887eeca ("net: mana: Assign interrupts to CPUs based on NUMA nodes")
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
>  .../net/ethernet/microsoft/mana/gdma_main.c   | 40 ++++++++++---------
>  include/net/mana/gdma.h                       |  1 +
>  2 files changed, 23 insertions(+), 18 deletions(-)
> 

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

RE: [PATCH net, 2/2] net: mana: Fix accessing freed irq affinity_hint

[Michael Kelley (LINUX)]

From: LKML haiyangz <lkmlhyz@microsoft.com> On Behalf Of Haiyang Zhang Sent: Thursday, January 26, 2023 1:05 PM
> 
> After calling irq_set_affinity_and_hint(), the cpumask pointer is
> saved in desc->affinity_hint, and will be used later when reading
> /proc/irq/<num>/affinity_hint. So the cpumask variable needs to be
> allocated per irq, and available until freeing the irq. Otherwise,
> we are accessing freed memory when reading the affinity_hint file.
> 
> To fix the bug, allocate the cpumask per irq, and free it just
> before freeing the irq.

Since the cpumask being passed to irq_set_affinity_and_hint()
always contains exactly one CPU, the code can be considerably
simplified by using the pre-calculated and persistent masks
available as cpumask_of(cpu).  All allocation of cpumasks in this
code goes away, and you can set the affinity_hint to NULL in the
cleanup and remove paths without having to free any masks.

Michael

> 
> Cc: stable@vger.kernel.org
> Fixes: 71fa6887eeca ("net: mana: Assign interrupts to CPUs based on NUMA nodes")
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
>  .../net/ethernet/microsoft/mana/gdma_main.c   | 40 ++++++++++---------
>  include/net/mana/gdma.h                       |  1 +
>  2 files changed, 23 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index 3bae9d4c1f08..37473ae3859c 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -1219,7 +1219,6 @@ static int mana_gd_setup_irqs(struct pci_dev *pdev)
>  	struct gdma_irq_context *gic;
>  	unsigned int max_irqs;
>  	u16 *cpus;
> -	cpumask_var_t req_mask;
>  	int nvec, irq;
>  	int err, i = 0, j;
> 
> @@ -1240,25 +1239,26 @@ static int mana_gd_setup_irqs(struct pci_dev *pdev)
>  		goto free_irq_vector;
>  	}
> 
> -	if (!zalloc_cpumask_var(&req_mask, GFP_KERNEL)) {
> -		err = -ENOMEM;
> -		goto free_irq;
> -	}
> -
>  	cpus = kcalloc(nvec, sizeof(*cpus), GFP_KERNEL);
>  	if (!cpus) {
>  		err = -ENOMEM;
> -		goto free_mask;
> +		goto free_gic;
>  	}
>  	for (i = 0; i < nvec; i++)
>  		cpus[i] = cpumask_local_spread(i, gc->numa_node);
> 
>  	for (i = 0; i < nvec; i++) {
> -		cpumask_set_cpu(cpus[i], req_mask);
>  		gic = &gc->irq_contexts[i];
>  		gic->handler = NULL;
>  		gic->arg = NULL;
> 
> +		if (!zalloc_cpumask_var(&gic->cpu_hint, GFP_KERNEL)) {
> +			err = -ENOMEM;
> +			goto free_irq;
> +		}
> +
> +		cpumask_set_cpu(cpus[i], gic->cpu_hint);
> +
>  		if (!i)
>  			snprintf(gic->name, MANA_IRQ_NAME_SZ,
> "mana_hwc@pci:%s",
>  				 pci_name(pdev));
> @@ -1269,17 +1269,18 @@ static int mana_gd_setup_irqs(struct pci_dev *pdev)
>  		irq = pci_irq_vector(pdev, i);
>  		if (irq < 0) {
>  			err = irq;
> -			goto free_mask;
> +			free_cpumask_var(gic->cpu_hint);
> +			goto free_irq;
>  		}
> 
>  		err = request_irq(irq, mana_gd_intr, 0, gic->name, gic);
> -		if (err)
> -			goto free_mask;
> -		irq_set_affinity_and_hint(irq, req_mask);
> -		cpumask_clear(req_mask);
> +		if (err) {
> +			free_cpumask_var(gic->cpu_hint);
> +			goto free_irq;
> +		}
> +
> +		irq_set_affinity_and_hint(irq, gic->cpu_hint);
>  	}
> -	free_cpumask_var(req_mask);
> -	kfree(cpus);
> 
>  	err = mana_gd_alloc_res_map(nvec, &gc->msix_resource);
>  	if (err)
> @@ -1288,20 +1289,22 @@ static int mana_gd_setup_irqs(struct pci_dev *pdev)
>  	gc->max_num_msix = nvec;
>  	gc->num_msix_usable = nvec;
> 
> +	kfree(cpus);
>  	return 0;
> 
> -free_mask:
> -	free_cpumask_var(req_mask);
> -	kfree(cpus);
>  free_irq:
>  	for (j = i - 1; j >= 0; j--) {
>  		irq = pci_irq_vector(pdev, j);
>  		gic = &gc->irq_contexts[j];
> 
>  		irq_update_affinity_hint(irq, NULL);
> +		free_cpumask_var(gic->cpu_hint);
>  		free_irq(irq, gic);
>  	}
> 
> +	kfree(cpus);
> +
> +free_gic:
>  	kfree(gc->irq_contexts);
>  	gc->irq_contexts = NULL;
>  free_irq_vector:
> @@ -1329,6 +1332,7 @@ static void mana_gd_remove_irqs(struct pci_dev *pdev)
> 
>  		/* Need to clear the hint before free_irq */
>  		irq_update_affinity_hint(irq, NULL);
> +		free_cpumask_var(gic->cpu_hint);
>  		free_irq(irq, gic);
>  	}
> 
> diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
> index 56189e4252da..4dcafecbd89e 100644
> --- a/include/net/mana/gdma.h
> +++ b/include/net/mana/gdma.h
> @@ -342,6 +342,7 @@ struct gdma_irq_context {
>  	void (*handler)(void *arg);
>  	void *arg;
>  	char name[MANA_IRQ_NAME_SZ];
> +	cpumask_var_t cpu_hint;
>  };
> 
>  struct gdma_context {
> --
> 2.25.1

RE: [PATCH net, 2/2] net: mana: Fix accessing freed irq affinity_hint

[Haiyang Zhang]

> -----Original Message-----
> From: Michael Kelley (LINUX) <mikelley@microsoft.com>
> Sent: Sunday, January 29, 2023 9:27 AM
> To: Haiyang Zhang <haiyangz@microsoft.com>; linux-hyperv@vger.kernel.org;
> netdev@vger.kernel.org
> Cc: Haiyang Zhang <haiyangz@microsoft.com>; Dexuan Cui
> <decui@microsoft.com>; KY Srinivasan <kys@microsoft.com>; Paul Rosswurm
> <paulros@microsoft.com>; olaf@aepfle.de; vkuznets@redhat.com;
> davem@davemloft.net; linux-kernel@vger.kernel.org; stable@vger.kernel.org
> Subject: RE: [PATCH net, 2/2] net: mana: Fix accessing freed irq affinity_hint
> 
> From: LKML haiyangz <lkmlhyz@microsoft.com> On Behalf Of Haiyang Zhang
> Sent: Thursday, January 26, 2023 1:05 PM
> >
> > After calling irq_set_affinity_and_hint(), the cpumask pointer is
> > saved in desc->affinity_hint, and will be used later when reading
> > /proc/irq/<num>/affinity_hint. So the cpumask variable needs to be
> > allocated per irq, and available until freeing the irq. Otherwise,
> > we are accessing freed memory when reading the affinity_hint file.
> >
> > To fix the bug, allocate the cpumask per irq, and free it just
> > before freeing the irq.
> 
> Since the cpumask being passed to irq_set_affinity_and_hint()
> always contains exactly one CPU, the code can be considerably
> simplified by using the pre-calculated and persistent masks
> available as cpumask_of(cpu).  All allocation of cpumasks in this
> code goes away, and you can set the affinity_hint to NULL in the
> cleanup and remove paths without having to free any masks.
> 
Great idea!
Will update the patch accordingly.

- Haiyang

RE: [PATCH net, 2/2] net: mana: Fix accessing freed irq affinity_hint

[Haiyang Zhang]

> -----Original Message-----
> From: Haiyang Zhang <haiyangz@microsoft.com>
> Sent: Sunday, January 29, 2023 1:51 PM
> To: Michael Kelley (LINUX) <mikelley@microsoft.com>; linux-
> hyperv@vger.kernel.org; netdev@vger.kernel.org
> Cc: Dexuan Cui <decui@microsoft.com>; KY Srinivasan <kys@microsoft.com>;
> Paul Rosswurm <paulros@microsoft.com>; olaf@aepfle.de;
> vkuznets@redhat.com; davem@davemloft.net; linux-kernel@vger.kernel.org;
> stable@vger.kernel.org
> Subject: RE: [PATCH net, 2/2] net: mana: Fix accessing freed irq affinity_hint
> 
> 
> 
> > -----Original Message-----
> > From: Michael Kelley (LINUX) <mikelley@microsoft.com>
> > Sent: Sunday, January 29, 2023 9:27 AM
> > To: Haiyang Zhang <haiyangz@microsoft.com>; linux-
> hyperv@vger.kernel.org;
> > netdev@vger.kernel.org
> > Cc: Haiyang Zhang <haiyangz@microsoft.com>; Dexuan Cui
> > <decui@microsoft.com>; KY Srinivasan <kys@microsoft.com>; Paul
> Rosswurm
> > <paulros@microsoft.com>; olaf@aepfle.de; vkuznets@redhat.com;
> > davem@davemloft.net; linux-kernel@vger.kernel.org;
> stable@vger.kernel.org
> > Subject: RE: [PATCH net, 2/2] net: mana: Fix accessing freed irq affinity_hint
> >
> > From: LKML haiyangz <lkmlhyz@microsoft.com> On Behalf Of Haiyang
> Zhang
> > Sent: Thursday, January 26, 2023 1:05 PM
> > >
> > > After calling irq_set_affinity_and_hint(), the cpumask pointer is
> > > saved in desc->affinity_hint, and will be used later when reading
> > > /proc/irq/<num>/affinity_hint. So the cpumask variable needs to be
> > > allocated per irq, and available until freeing the irq. Otherwise,
> > > we are accessing freed memory when reading the affinity_hint file.
> > >
> > > To fix the bug, allocate the cpumask per irq, and free it just
> > > before freeing the irq.
> >
> > Since the cpumask being passed to irq_set_affinity_and_hint()
> > always contains exactly one CPU, the code can be considerably
> > simplified by using the pre-calculated and persistent masks
> > available as cpumask_of(cpu).  All allocation of cpumasks in this
> > code goes away, and you can set the affinity_hint to NULL in the
> > cleanup and remove paths without having to free any masks.
> >
> Great idea!
> Will update the patch accordingly.

Also, I saw this alloc isn't necessary either:
	cpus = kcalloc(nvec, sizeof(*cpus), GFP_KERNEL);

We can simply use the return from cpumask_local_spread()
without saving all cpu numbers in a tmp array.

I will clean this up too :)

Thanks,
- Haiyang