[PATCH v2] soc: fsl: dpio: Change 'cpumask_t mask' to the driver's private data

[PATCH v2] soc: fsl: dpio: Change 'cpumask_t mask' to the driver's private data

[Yi Wang]

[-- Attachment #1.1: Type: text/plain, Size: 4556 bytes --]

From: Hao Si <si.hao@zte.com.cn>

The local variable 'cpumask_t mask' is in the stack memory, and its address
is assigned to 'desc->affinity' in 'irq_set_affinity_hint()'.
But the memory area where this variable is located is at risk of being
modified.

During LTP testing, the following error was generated:

Unable to handle kernel paging request at virtual address ffff000012e9b790
Mem abort info:
  ESR = 0x96000007
  Exception class = DABT (current EL), IL = 32 bits
  SET = 0, FnV = 0
  EA = 0, S1PTW = 0
Data abort info:
  ISV = 0, ISS = 0x00000007
  CM = 0, WnR = 0
swapper pgtable: 4k pages, 48-bit VAs, pgdp = 0000000075ac5e07
[ffff000012e9b790] pgd=00000027dbffe003, pud=00000027dbffd003,
pmd=00000027b6d61003, pte=0000000000000000
Internal error: Oops: 96000007 [#1] PREEMPT SMP
Modules linked in: xt_conntrack
Process read_all (pid: 20171, stack limit = 0x0000000044ea4095)
CPU: 14 PID: 20171 Comm: read_all Tainted: G    B   W
Hardware name: NXP Layerscape LX2160ARDB (DT)
pstate: 80000085 (Nzcv daIf -PAN -UAO)
pc : irq_affinity_hint_proc_show+0x54/0xb0
lr : irq_affinity_hint_proc_show+0x4c/0xb0
sp : ffff00001138bc10
x29: ffff00001138bc10 x28: 0000ffffd131d1e0
x27: 00000000007000c0 x26: ffff8025b9480dc0
x25: ffff8025b9480da8 x24: 00000000000003ff
x23: ffff8027334f8300 x22: ffff80272e97d000
x21: ffff80272e97d0b0 x20: ffff8025b9480d80
x19: ffff000009a49000 x18: 0000000000000000
x17: 0000000000000000 x16: 0000000000000000
x15: 0000000000000000 x14: 0000000000000000
x13: 0000000000000000 x12: 0000000000000040
x11: 0000000000000000 x10: ffff802735b79b88
x9 : 0000000000000000 x8 : 0000000000000000
x7 : ffff000009a49848 x6 : 0000000000000003
x5 : 0000000000000000 x4 : ffff000008157d6c
x3 : ffff00001138bc10 x2 : ffff000012e9b790
x1 : 0000000000000000 x0 : 0000000000000000
Call trace:
 irq_affinity_hint_proc_show+0x54/0xb0
 seq_read+0x1b0/0x440
 proc_reg_read+0x80/0xd8
 __vfs_read+0x60/0x178
 vfs_read+0x94/0x150
 ksys_read+0x74/0xf0
 __arm64_sys_read+0x24/0x30
 el0_svc_common.constprop.0+0xd8/0x1a0
 el0_svc_handler+0x34/0x88
 el0_svc+0x10/0x14
Code: f9001bbf 943e0732 f94066c2 b4000062 (f9400041)
---[ end trace b495bdcb0b3b732b ]---
Kernel panic - not syncing: Fatal exception
SMP: stopping secondary CPUs
SMP: failed to stop secondary CPUs 0,2-4,6,8,11,13-15
Kernel Offset: disabled
CPU features: 0x0,21006008
Memory Limit: none
---[ end Kernel panic - not syncing: Fatal exception ]---

Fix it by changing 'cpumask_t mask' to the driver's private data.

Signed-off-by: Hao Si <si.hao@zte.com.cn>
Signed-off-by: Lin Chen <chen.lin5@zte.com.cn>
Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
---
v2: Place 'cpumask_t mask' in the driver's private data and while at it,
rename it to cpu_mask.

 drivers/soc/fsl/dpio/dpio-driver.c | 9 +++++----
 include/linux/fsl/mc.h             | 2 ++
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/soc/fsl/dpio/dpio-driver.c b/drivers/soc/fsl/dpio/dpio-driver.c
index 7b642c3..e9d820d 100644
--- a/drivers/soc/fsl/dpio/dpio-driver.c
+++ b/drivers/soc/fsl/dpio/dpio-driver.c
@@ -95,7 +95,7 @@ static int register_dpio_irq_handlers(struct fsl_mc_device *dpio_dev, int cpu)
 {
 	int error;
 	struct fsl_mc_device_irq *irq;
-	cpumask_t mask;
+	cpumask_t *cpu_mask;
 
 	irq = dpio_dev->irqs[0];
 	error = devm_request_irq(&dpio_dev->dev,
@@ -112,9 +112,10 @@ static int register_dpio_irq_handlers(struct fsl_mc_device *dpio_dev, int cpu)
 	}
 
 	/* set the affinity hint */
-	cpumask_clear(&mask);
-	cpumask_set_cpu(cpu, &mask);
-	if (irq_set_affinity_hint(irq->msi_desc->irq, &mask))
+	cpu_mask = &dpio_dev->mask;
+	cpumask_clear(cpu_mask);
+	cpumask_set_cpu(cpu, cpu_mask);
+	if (irq_set_affinity_hint(irq->msi_desc->irq, cpu_mask))
 		dev_err(&dpio_dev->dev,
 			"irq_set_affinity failed irq %d cpu %d\n",
 			irq->msi_desc->irq, cpu);
diff --git a/include/linux/fsl/mc.h b/include/linux/fsl/mc.h
index a428c61..ebdfb54 100644
--- a/include/linux/fsl/mc.h
+++ b/include/linux/fsl/mc.h
@@ -151,6 +151,7 @@ struct fsl_mc_obj_desc {
 /**
  * struct fsl_mc_device - MC object device object
  * @dev: Linux driver model device object
+ * @mask: cpu mask for affinity_hint
  * @dma_mask: Default DMA mask
  * @flags: MC object device flags
  * @icid: Isolation context ID for the device
@@ -184,6 +185,7 @@ struct fsl_mc_obj_desc {
  */
 struct fsl_mc_device {
 	struct device dev;
+	cpumask_t mask;
 	u64 dma_mask;
 	u16 flags;
 	u16 icid;
-- 
2.15.2

RE: [PATCH v2] soc: fsl: dpio: Change 'cpumask_t mask' to the driver's private data

[Leo Li]

> -----Original Message-----
> From: Yi Wang <wang.yi59@zte.com.cn>
> Sent: Friday, October 16, 2020 1:49 AM
> To: Roy Pledge <roy.pledge@nxp.com>; Laurentiu Tudor
> <laurentiu.tudor@nxp.com>
> Cc: Leo Li <leoyang.li@nxp.com>; linux-kernel@vger.kernel.org; linuxppc-
> dev@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org;
> xue.zhihong@zte.com.cn; wang.yi59@zte.com.cn; jiang.xuexin@zte.com.cn;
> Hao Si <si.hao@zte.com.cn>; Lin Chen <chen.lin5@zte.com.cn>
> Subject: [PATCH v2] soc: fsl: dpio: Change 'cpumask_t mask' to the driver's
> private data
> 
> From: Hao Si <si.hao@zte.com.cn>
> 
> The local variable 'cpumask_t mask' is in the stack memory, and its address
> is assigned to 'desc->affinity' in 'irq_set_affinity_hint()'.
> But the memory area where this variable is located is at risk of being
> modified.
> 
> During LTP testing, the following error was generated:
> 
> Unable to handle kernel paging request at virtual address ffff000012e9b790
> Mem abort info:
>   ESR = 0x96000007
>   Exception class = DABT (current EL), IL = 32 bits
>   SET = 0, FnV = 0
>   EA = 0, S1PTW = 0
> Data abort info:
>   ISV = 0, ISS = 0x00000007
>   CM = 0, WnR = 0
> swapper pgtable: 4k pages, 48-bit VAs, pgdp = 0000000075ac5e07
> [ffff000012e9b790] pgd=00000027dbffe003, pud=00000027dbffd003,
> pmd=00000027b6d61003, pte=0000000000000000
> Internal error: Oops: 96000007 [#1] PREEMPT SMP
> Modules linked in: xt_conntrack
> Process read_all (pid: 20171, stack limit = 0x0000000044ea4095)
> CPU: 14 PID: 20171 Comm: read_all Tainted: G    B   W
> Hardware name: NXP Layerscape LX2160ARDB (DT)
> pstate: 80000085 (Nzcv daIf -PAN -UAO)
> pc : irq_affinity_hint_proc_show+0x54/0xb0
> lr : irq_affinity_hint_proc_show+0x4c/0xb0
> sp : ffff00001138bc10
> x29: ffff00001138bc10 x28: 0000ffffd131d1e0
> x27: 00000000007000c0 x26: ffff8025b9480dc0
> x25: ffff8025b9480da8 x24: 00000000000003ff
> x23: ffff8027334f8300 x22: ffff80272e97d000
> x21: ffff80272e97d0b0 x20: ffff8025b9480d80
> x19: ffff000009a49000 x18: 0000000000000000
> x17: 0000000000000000 x16: 0000000000000000
> x15: 0000000000000000 x14: 0000000000000000
> x13: 0000000000000000 x12: 0000000000000040
> x11: 0000000000000000 x10: ffff802735b79b88
> x9 : 0000000000000000 x8 : 0000000000000000
> x7 : ffff000009a49848 x6 : 0000000000000003
> x5 : 0000000000000000 x4 : ffff000008157d6c
> x3 : ffff00001138bc10 x2 : ffff000012e9b790
> x1 : 0000000000000000 x0 : 0000000000000000
> Call trace:
>  irq_affinity_hint_proc_show+0x54/0xb0
>  seq_read+0x1b0/0x440
>  proc_reg_read+0x80/0xd8
>  __vfs_read+0x60/0x178
>  vfs_read+0x94/0x150
>  ksys_read+0x74/0xf0
>  __arm64_sys_read+0x24/0x30
>  el0_svc_common.constprop.0+0xd8/0x1a0
>  el0_svc_handler+0x34/0x88
>  el0_svc+0x10/0x14
> Code: f9001bbf 943e0732 f94066c2 b4000062 (f9400041)
> ---[ end trace b495bdcb0b3b732b ]---
> Kernel panic - not syncing: Fatal exception
> SMP: stopping secondary CPUs
> SMP: failed to stop secondary CPUs 0,2-4,6,8,11,13-15
> Kernel Offset: disabled
> CPU features: 0x0,21006008
> Memory Limit: none
> ---[ end Kernel panic - not syncing: Fatal exception ]---
> 
> Fix it by changing 'cpumask_t mask' to the driver's private data.

Thanks for the patch.  Sorry to chime in late.

Since we are only setting single bit in the cpumask, it is actually not necessary to keep the cpumask in private data as we already kept the cpu number in desc.cpu.  The better and easier approach is to actually use get_cpu_mask(cpu) API to get the pre-defined cpumask in the static cpu_bit_bitmap array.  We don't even need to declare the mask/cpu_mask in the register_dpio_irq_handlers().

> 
> Signed-off-by: Hao Si <si.hao@zte.com.cn>
> Signed-off-by: Lin Chen <chen.lin5@zte.com.cn>
> Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> ---
> v2: Place 'cpumask_t mask' in the driver's private data and while at it,
> rename it to cpu_mask.
> 
>  drivers/soc/fsl/dpio/dpio-driver.c | 9 +++++----
>  include/linux/fsl/mc.h             | 2 ++
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/soc/fsl/dpio/dpio-driver.c b/drivers/soc/fsl/dpio/dpio-
> driver.c
> index 7b642c3..e9d820d 100644
> --- a/drivers/soc/fsl/dpio/dpio-driver.c
> +++ b/drivers/soc/fsl/dpio/dpio-driver.c
> @@ -95,7 +95,7 @@ static int register_dpio_irq_handlers(struct
> fsl_mc_device *dpio_dev, int cpu)
>  {
>  	int error;
>  	struct fsl_mc_device_irq *irq;
> -	cpumask_t mask;
> +	cpumask_t *cpu_mask;
> 
>  	irq = dpio_dev->irqs[0];
>  	error = devm_request_irq(&dpio_dev->dev,
> @@ -112,9 +112,10 @@ static int register_dpio_irq_handlers(struct
> fsl_mc_device *dpio_dev, int cpu)
>  	}
> 
>  	/* set the affinity hint */
> -	cpumask_clear(&mask);
> -	cpumask_set_cpu(cpu, &mask);
> -	if (irq_set_affinity_hint(irq->msi_desc->irq, &mask))
> +	cpu_mask = &dpio_dev->mask;
> +	cpumask_clear(cpu_mask);
> +	cpumask_set_cpu(cpu, cpu_mask);
> +	if (irq_set_affinity_hint(irq->msi_desc->irq, cpu_mask))
>  		dev_err(&dpio_dev->dev,
>  			"irq_set_affinity failed irq %d cpu %d\n",
>  			irq->msi_desc->irq, cpu);
> diff --git a/include/linux/fsl/mc.h b/include/linux/fsl/mc.h
> index a428c61..ebdfb54 100644
> --- a/include/linux/fsl/mc.h
> +++ b/include/linux/fsl/mc.h
> @@ -151,6 +151,7 @@ struct fsl_mc_obj_desc {
>  /**
>   * struct fsl_mc_device - MC object device object
>   * @dev: Linux driver model device object
> + * @mask: cpu mask for affinity_hint
>   * @dma_mask: Default DMA mask
>   * @flags: MC object device flags
>   * @icid: Isolation context ID for the device
> @@ -184,6 +185,7 @@ struct fsl_mc_obj_desc {
>   */
>  struct fsl_mc_device {
>  	struct device dev;
> +	cpumask_t mask;
>  	u64 dma_mask;
>  	u16 flags;
>  	u16 icid;
> --
> 2.15.2

RE: [PATCH v2] soc: fsl: dpio: Change 'cpumask_t mask' to the driver's private data

[Leo Li]

> -----Original Message-----
> From: Leo Li
> Sent: Friday, October 16, 2020 4:20 PM
> To: 'Yi Wang' <wang.yi59@zte.com.cn>; Roy Pledge <roy.pledge@nxp.com>;
> Laurentiu Tudor <laurentiu.tudor@nxp.com>
> Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-arm-
> kernel@lists.infradead.org; xue.zhihong@zte.com.cn;
> jiang.xuexin@zte.com.cn; Hao Si <si.hao@zte.com.cn>; Lin Chen
> <chen.lin5@zte.com.cn>
> Subject: RE: [PATCH v2] soc: fsl: dpio: Change 'cpumask_t mask' to the
> driver's private data
> 
> 
> 
> > -----Original Message-----
> > From: Yi Wang <wang.yi59@zte.com.cn>
> > Sent: Friday, October 16, 2020 1:49 AM
> > To: Roy Pledge <roy.pledge@nxp.com>; Laurentiu Tudor
> > <laurentiu.tudor@nxp.com>
> > Cc: Leo Li <leoyang.li@nxp.com>; linux-kernel@vger.kernel.org;
> > linuxppc- dev@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org;
> > xue.zhihong@zte.com.cn; wang.yi59@zte.com.cn;
> jiang.xuexin@zte.com.cn;
> > Hao Si <si.hao@zte.com.cn>; Lin Chen <chen.lin5@zte.com.cn>
> > Subject: [PATCH v2] soc: fsl: dpio: Change 'cpumask_t mask' to the
> > driver's private data
> >
> > From: Hao Si <si.hao@zte.com.cn>
> >
> > The local variable 'cpumask_t mask' is in the stack memory, and its
> > address is assigned to 'desc->affinity' in 'irq_set_affinity_hint()'.
> > But the memory area where this variable is located is at risk of being
> > modified.
> >
> > During LTP testing, the following error was generated:
> >
> > Unable to handle kernel paging request at virtual address
> > ffff000012e9b790 Mem abort info:
> >   ESR = 0x96000007
> >   Exception class = DABT (current EL), IL = 32 bits
> >   SET = 0, FnV = 0
> >   EA = 0, S1PTW = 0
> > Data abort info:
> >   ISV = 0, ISS = 0x00000007
> >   CM = 0, WnR = 0
> > swapper pgtable: 4k pages, 48-bit VAs, pgdp = 0000000075ac5e07
> > [ffff000012e9b790] pgd=00000027dbffe003, pud=00000027dbffd003,
> > pmd=00000027b6d61003, pte=0000000000000000 Internal error: Oops:
> > 96000007 [#1] PREEMPT SMP Modules linked in: xt_conntrack Process
> > read_all (pid: 20171, stack limit = 0x0000000044ea4095)
> > CPU: 14 PID: 20171 Comm: read_all Tainted: G    B   W
> > Hardware name: NXP Layerscape LX2160ARDB (DT)
> > pstate: 80000085 (Nzcv daIf -PAN -UAO) pc :
> > irq_affinity_hint_proc_show+0x54/0xb0
> > lr : irq_affinity_hint_proc_show+0x4c/0xb0
> > sp : ffff00001138bc10
> > x29: ffff00001138bc10 x28: 0000ffffd131d1e0
> > x27: 00000000007000c0 x26: ffff8025b9480dc0
> > x25: ffff8025b9480da8 x24: 00000000000003ff
> > x23: ffff8027334f8300 x22: ffff80272e97d000
> > x21: ffff80272e97d0b0 x20: ffff8025b9480d80
> > x19: ffff000009a49000 x18: 0000000000000000
> > x17: 0000000000000000 x16: 0000000000000000
> > x15: 0000000000000000 x14: 0000000000000000
> > x13: 0000000000000000 x12: 0000000000000040
> > x11: 0000000000000000 x10: ffff802735b79b88
> > x9 : 0000000000000000 x8 : 0000000000000000
> > x7 : ffff000009a49848 x6 : 0000000000000003
> > x5 : 0000000000000000 x4 : ffff000008157d6c
> > x3 : ffff00001138bc10 x2 : ffff000012e9b790
> > x1 : 0000000000000000 x0 : 0000000000000000 Call trace:
> >  irq_affinity_hint_proc_show+0x54/0xb0
> >  seq_read+0x1b0/0x440
> >  proc_reg_read+0x80/0xd8
> >  __vfs_read+0x60/0x178
> >  vfs_read+0x94/0x150
> >  ksys_read+0x74/0xf0
> >  __arm64_sys_read+0x24/0x30
> >  el0_svc_common.constprop.0+0xd8/0x1a0
> >  el0_svc_handler+0x34/0x88
> >  el0_svc+0x10/0x14
> > Code: f9001bbf 943e0732 f94066c2 b4000062 (f9400041) ---[ end trace
> > b495bdcb0b3b732b ]--- Kernel panic - not syncing: Fatal exception
> > SMP: stopping secondary CPUs
> > SMP: failed to stop secondary CPUs 0,2-4,6,8,11,13-15 Kernel Offset:
> > disabled CPU features: 0x0,21006008 Memory Limit: none ---[ end Kernel
> > panic - not syncing: Fatal exception ]---
> >
> > Fix it by changing 'cpumask_t mask' to the driver's private data.
> 
> Thanks for the patch.  Sorry to chime in late.
> 
> Since we are only setting single bit in the cpumask, it is actually not necessary
> to keep the cpumask in private data as we already kept the cpu number in
> desc.cpu.  The better and easier approach is to actually use
> get_cpu_mask(cpu) API to get the pre-defined cpumask in the static
> cpu_bit_bitmap array.  We don't even need to declare the mask/cpu_mask
> in the register_dpio_irq_handlers().

Btw, cpumask_of(cpu) is more commonly used than get_cpu_mask(cpu), although they are essentially the same.

> 
> >
> > Signed-off-by: Hao Si <si.hao@zte.com.cn>
> > Signed-off-by: Lin Chen <chen.lin5@zte.com.cn>
> > Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> > ---
> > v2: Place 'cpumask_t mask' in the driver's private data and while at
> > it, rename it to cpu_mask.
> >
> >  drivers/soc/fsl/dpio/dpio-driver.c | 9 +++++----
> >  include/linux/fsl/mc.h             | 2 ++
> >  2 files changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/soc/fsl/dpio/dpio-driver.c
> > b/drivers/soc/fsl/dpio/dpio- driver.c index 7b642c3..e9d820d 100644
> > --- a/drivers/soc/fsl/dpio/dpio-driver.c
> > +++ b/drivers/soc/fsl/dpio/dpio-driver.c
> > @@ -95,7 +95,7 @@ static int register_dpio_irq_handlers(struct
> > fsl_mc_device *dpio_dev, int cpu)  {
> >  	int error;
> >  	struct fsl_mc_device_irq *irq;
> > -	cpumask_t mask;
> > +	cpumask_t *cpu_mask;
> >
> >  	irq = dpio_dev->irqs[0];
> >  	error = devm_request_irq(&dpio_dev->dev, @@ -112,9 +112,10 @@
> static
> > int register_dpio_irq_handlers(struct fsl_mc_device *dpio_dev, int
> > cpu)
> >  	}
> >
> >  	/* set the affinity hint */
> > -	cpumask_clear(&mask);
> > -	cpumask_set_cpu(cpu, &mask);
> > -	if (irq_set_affinity_hint(irq->msi_desc->irq, &mask))
> > +	cpu_mask = &dpio_dev->mask;
> > +	cpumask_clear(cpu_mask);
> > +	cpumask_set_cpu(cpu, cpu_mask);
> > +	if (irq_set_affinity_hint(irq->msi_desc->irq, cpu_mask))
> >  		dev_err(&dpio_dev->dev,
> >  			"irq_set_affinity failed irq %d cpu %d\n",
> >  			irq->msi_desc->irq, cpu);
> > diff --git a/include/linux/fsl/mc.h b/include/linux/fsl/mc.h index
> > a428c61..ebdfb54 100644
> > --- a/include/linux/fsl/mc.h
> > +++ b/include/linux/fsl/mc.h
> > @@ -151,6 +151,7 @@ struct fsl_mc_obj_desc {
> >  /**
> >   * struct fsl_mc_device - MC object device object
> >   * @dev: Linux driver model device object
> > + * @mask: cpu mask for affinity_hint
> >   * @dma_mask: Default DMA mask
> >   * @flags: MC object device flags
> >   * @icid: Isolation context ID for the device @@ -184,6 +185,7 @@
> > struct fsl_mc_obj_desc {
> >   */
> >  struct fsl_mc_device {
> >  	struct device dev;
> > +	cpumask_t mask;
> >  	u64 dma_mask;
> >  	u16 flags;
> >  	u16 icid;
> > --
> > 2.15.2