| Message ID | 1674767085-18583-3-git-send-email-haiyangz@microsoft.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fix usage of irq affinity_hint | expand |
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>
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
> -----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
> -----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
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 {
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(-)