Similar NULL deref was originally fixed by graceful teardown sequence -
https://lore.kernel.org/linux-i2c/[email protected]
After this, a tasklet was added to take care of FIFO full condition for large i2c
transaction.
https://lore.kernel.org/linux-arm-kernel/[email protected]/
This introduced regression, a new race condition between tasklet enabling
interrupts and client unreg teardown sequence.
Kill tasklet before unreg_slave() masks bits in IE_OFFSET.
Updated teardown sequence -
(1) disable_irq()
(2) Kill tasklet
(3) Mask event enable bits in control reg
(4) Erase slave address (avoid further writes to rx fifo)
(5) Flush tx and rx FIFOs
(6) Clear pending event (interrupt) bits in status reg
(7) Set client pointer to NULL
(8) enable_irq()
--
Unable to handle kernel read from unreadable memory at virtual address 0000000000000320
Mem abort info:
ESR = 0x96000004
EC = 0x25: DABT (current EL), IL = 32 bits
SET = 0, FnV = 0
EA = 0, S1PTW = 0
Data abort info:
ISV = 0, ISS = 0x00000004
CM = 0, WnR = 0
user pgtable: 4k pages, 48-bit VAs, pgdp=000000009212a000
[0000000000000320] pgd=0000000000000000, p4d=0000000000000000
Internal error: Oops: 96000004 [#1] SMP
CPU: 0 PID: 0 Comm: swapper/0 Tainted: G O
Hardware name: Overlake (DT)
pstate: 40400085 (nZcv daIf +PAN -UAO -TCO BTYPE=--)
pc : bcm_iproc_i2c_slave_isr+0x2b8/0x8e4
lr : bcm_iproc_i2c_slave_isr+0x1c8/0x8e4
sp : ffff800010003e70
x29: ffff800010003e80 x28: ffffda017acdc000
x27: ffffda017b0ae000 x26: ffff800010004000
x25: ffff800010000000 x24: ffffda017af4a168
x23: 0000000000000073 x22: 0000000000000000
x21: 0000000001400000 x20: 0000000001000000
x19: ffff06f09583f880 x18: 00000000fa83b2da
x17: 000000000000b67e x16: 0000000002edb2f3
x15: 00000000000002c7 x14: 00000000000002c7
x13: 0000000000000006 x12: 0000000000000033
x11: 0000000000000000 x10: 0000000001000000
x9 : 0000000003289312 x8 : 0000000003289311
x7 : 02d0cd03a303adbc x6 : 02d18e7f0a4dfc6c
x5 : 02edb2f33f76ea68 x4 : 00000000fa83b2da
x3 : ffffda017af43cd0 x2 : ffff800010003e74
x1 : 0000000001400000 x0 : 0000000000000000
Call trace:
bcm_iproc_i2c_slave_isr+0x2b8/0x8e4
bcm_iproc_i2c_isr+0x178/0x290
__handle_irq_event_percpu+0xd0/0x200
handle_irq_event+0x60/0x1a0
handle_fasteoi_irq+0x130/0x220
__handle_domain_irq+0x8c/0xcc
gic_handle_irq+0xc0/0x120
el1_irq+0xcc/0x180
finish_task_switch+0x100/0x1d8
__schedule+0x61c/0x7a0
schedule_idle+0x28/0x44
do_idle+0x254/0x28c
cpu_startup_entry+0x28/0x2c
rest_init+0xc4/0xd0
arch_call_rest_init+0x14/0x1c
start_kernel+0x33c/0x3b8
Code: f9423260 910013e2 11000509 b9047a69 (f9419009)
---[ end trace 4781455b2a7bec15 ]---
Fixes: 4d658451c9d6 ("i2c: iproc: handle rx fifo full interrupt")
Signed-off-by: Dhananjay Phadke <[email protected]>
---
drivers/i2c/busses/i2c-bcm-iproc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index cceaf69279a9..6304d1dd2dd6 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -1224,14 +1224,14 @@ static int bcm_iproc_i2c_unreg_slave(struct i2c_client *slave)
disable_irq(iproc_i2c->irq);
+ tasklet_kill(&iproc_i2c->slave_rx_tasklet);
+
/* disable all slave interrupts */
tmp = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
tmp &= ~(IE_S_ALL_INTERRUPT_MASK <<
IE_S_ALL_INTERRUPT_SHIFT);
iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, tmp);
- tasklet_kill(&iproc_i2c->slave_rx_tasklet);
-
/* Erase the slave address programmed */
tmp = iproc_i2c_rd_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET);
tmp &= ~BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT);
--
2.17.1
On 8/5/2021 2:49 PM, Dhananjay Phadke wrote:
> Similar NULL deref was originally fixed by graceful teardown sequence -
>
> https://lore.kernel.org/linux-i2c/[email protected]
>
> After this, a tasklet was added to take care of FIFO full condition for large i2c
> transaction.
>
> https://lore.kernel.org/linux-arm-kernel/[email protected]/
>
> This introduced regression, a new race condition between tasklet enabling
> interrupts and client unreg teardown sequence.
>
> Kill tasklet before unreg_slave() masks bits in IE_OFFSET.
> Updated teardown sequence -
> (1) disable_irq()
> (2) Kill tasklet
> (3) Mask event enable bits in control reg
> (4) Erase slave address (avoid further writes to rx fifo)
> (5) Flush tx and rx FIFOs
> (6) Clear pending event (interrupt) bits in status reg
> (7) Set client pointer to NULL
> (8) enable_irq()
>
> --
>
> Unable to handle kernel read from unreadable memory at virtual address 0000000000000320
> Mem abort info:
> ESR = 0x96000004
> EC = 0x25: DABT (current EL), IL = 32 bits
> SET = 0, FnV = 0
> EA = 0, S1PTW = 0
> Data abort info:
> ISV = 0, ISS = 0x00000004
> CM = 0, WnR = 0
> user pgtable: 4k pages, 48-bit VAs, pgdp=000000009212a000
> [0000000000000320] pgd=0000000000000000, p4d=0000000000000000
> Internal error: Oops: 96000004 [#1] SMP
> CPU: 0 PID: 0 Comm: swapper/0 Tainted: G O
> Hardware name: Overlake (DT)
> pstate: 40400085 (nZcv daIf +PAN -UAO -TCO BTYPE=--)
> pc : bcm_iproc_i2c_slave_isr+0x2b8/0x8e4
> lr : bcm_iproc_i2c_slave_isr+0x1c8/0x8e4
> sp : ffff800010003e70
> x29: ffff800010003e80 x28: ffffda017acdc000
> x27: ffffda017b0ae000 x26: ffff800010004000
> x25: ffff800010000000 x24: ffffda017af4a168
> x23: 0000000000000073 x22: 0000000000000000
> x21: 0000000001400000 x20: 0000000001000000
> x19: ffff06f09583f880 x18: 00000000fa83b2da
> x17: 000000000000b67e x16: 0000000002edb2f3
> x15: 00000000000002c7 x14: 00000000000002c7
> x13: 0000000000000006 x12: 0000000000000033
> x11: 0000000000000000 x10: 0000000001000000
> x9 : 0000000003289312 x8 : 0000000003289311
> x7 : 02d0cd03a303adbc x6 : 02d18e7f0a4dfc6c
> x5 : 02edb2f33f76ea68 x4 : 00000000fa83b2da
> x3 : ffffda017af43cd0 x2 : ffff800010003e74
> x1 : 0000000001400000 x0 : 0000000000000000
> Call trace:
> bcm_iproc_i2c_slave_isr+0x2b8/0x8e4
> bcm_iproc_i2c_isr+0x178/0x290
> __handle_irq_event_percpu+0xd0/0x200
> handle_irq_event+0x60/0x1a0
> handle_fasteoi_irq+0x130/0x220
> __handle_domain_irq+0x8c/0xcc
> gic_handle_irq+0xc0/0x120
> el1_irq+0xcc/0x180
> finish_task_switch+0x100/0x1d8
> __schedule+0x61c/0x7a0
> schedule_idle+0x28/0x44
> do_idle+0x254/0x28c
> cpu_startup_entry+0x28/0x2c
> rest_init+0xc4/0xd0
> arch_call_rest_init+0x14/0x1c
> start_kernel+0x33c/0x3b8
> Code: f9423260 910013e2 11000509 b9047a69 (f9419009)
> ---[ end trace 4781455b2a7bec15 ]---
>
> Fixes: 4d658451c9d6 ("i2c: iproc: handle rx fifo full interrupt")
>
> Signed-off-by: Dhananjay Phadke <[email protected]>
> ---
> drivers/i2c/busses/i2c-bcm-iproc.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
> index cceaf69279a9..6304d1dd2dd6 100644
> --- a/drivers/i2c/busses/i2c-bcm-iproc.c
> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
> @@ -1224,14 +1224,14 @@ static int bcm_iproc_i2c_unreg_slave(struct i2c_client *slave)
>
> disable_irq(iproc_i2c->irq);
>
> + tasklet_kill(&iproc_i2c->slave_rx_tasklet);
> +
> /* disable all slave interrupts */
> tmp = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
> tmp &= ~(IE_S_ALL_INTERRUPT_MASK <<
> IE_S_ALL_INTERRUPT_SHIFT);
> iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, tmp);
>
> - tasklet_kill(&iproc_i2c->slave_rx_tasklet);
> -
> /* Erase the slave address programmed */
> tmp = iproc_i2c_rd_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET);
> tmp &= ~BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT);
>
Yeah, needs to ensure no pending tasklet running to enable (unmask)
slave interrupts before disable (mask) them.
Thanks for the fix!
Acked-by: Ray Jui <[email protected]>
On Fri, Aug 6, 2021 at 10:19 PM Ray Jui <[email protected]> wrote:
>
>
>
> On 8/5/2021 2:49 PM, Dhananjay Phadke wrote:
> > Similar NULL deref was originally fixed by graceful teardown sequence -
> >
> > https://lore.kernel.org/linux-i2c/[email protected]
> >
> > After this, a tasklet was added to take care of FIFO full condition for large i2c
> > transaction.
> >
> > https://lore.kernel.org/linux-arm-kernel/[email protected]/
> >
> > This introduced regression, a new race condition between tasklet enabling
> > interrupts and client unreg teardown sequence.
> >
> > Kill tasklet before unreg_slave() masks bits in IE_OFFSET.
> > Updated teardown sequence -
> > (1) disable_irq()
> > (2) Kill tasklet
> > (3) Mask event enable bits in control reg
> > (4) Erase slave address (avoid further writes to rx fifo)
> > (5) Flush tx and rx FIFOs
> > (6) Clear pending event (interrupt) bits in status reg
> > (7) Set client pointer to NULL
> > (8) enable_irq()
> >
> > --
> >
> > Unable to handle kernel read from unreadable memory at virtual address 0000000000000320
> > Mem abort info:
> > ESR = 0x96000004
> > EC = 0x25: DABT (current EL), IL = 32 bits
> > SET = 0, FnV = 0
> > EA = 0, S1PTW = 0
> > Data abort info:
> > ISV = 0, ISS = 0x00000004
> > CM = 0, WnR = 0
> > user pgtable: 4k pages, 48-bit VAs, pgdp=000000009212a000
> > [0000000000000320] pgd=0000000000000000, p4d=0000000000000000
> > Internal error: Oops: 96000004 [#1] SMP
> > CPU: 0 PID: 0 Comm: swapper/0 Tainted: G O
> > Hardware name: Overlake (DT)
> > pstate: 40400085 (nZcv daIf +PAN -UAO -TCO BTYPE=--)
> > pc : bcm_iproc_i2c_slave_isr+0x2b8/0x8e4
> > lr : bcm_iproc_i2c_slave_isr+0x1c8/0x8e4
> > sp : ffff800010003e70
> > x29: ffff800010003e80 x28: ffffda017acdc000
> > x27: ffffda017b0ae000 x26: ffff800010004000
> > x25: ffff800010000000 x24: ffffda017af4a168
> > x23: 0000000000000073 x22: 0000000000000000
> > x21: 0000000001400000 x20: 0000000001000000
> > x19: ffff06f09583f880 x18: 00000000fa83b2da
> > x17: 000000000000b67e x16: 0000000002edb2f3
> > x15: 00000000000002c7 x14: 00000000000002c7
> > x13: 0000000000000006 x12: 0000000000000033
> > x11: 0000000000000000 x10: 0000000001000000
> > x9 : 0000000003289312 x8 : 0000000003289311
> > x7 : 02d0cd03a303adbc x6 : 02d18e7f0a4dfc6c
> > x5 : 02edb2f33f76ea68 x4 : 00000000fa83b2da
> > x3 : ffffda017af43cd0 x2 : ffff800010003e74
> > x1 : 0000000001400000 x0 : 0000000000000000
> > Call trace:
> > bcm_iproc_i2c_slave_isr+0x2b8/0x8e4
> > bcm_iproc_i2c_isr+0x178/0x290
> > __handle_irq_event_percpu+0xd0/0x200
> > handle_irq_event+0x60/0x1a0
> > handle_fasteoi_irq+0x130/0x220
> > __handle_domain_irq+0x8c/0xcc
> > gic_handle_irq+0xc0/0x120
> > el1_irq+0xcc/0x180
> > finish_task_switch+0x100/0x1d8
> > __schedule+0x61c/0x7a0
> > schedule_idle+0x28/0x44
> > do_idle+0x254/0x28c
> > cpu_startup_entry+0x28/0x2c
> > rest_init+0xc4/0xd0
> > arch_call_rest_init+0x14/0x1c
> > start_kernel+0x33c/0x3b8
> > Code: f9423260 910013e2 11000509 b9047a69 (f9419009)
> > ---[ end trace 4781455b2a7bec15 ]---
> >
> > Fixes: 4d658451c9d6 ("i2c: iproc: handle rx fifo full interrupt")
> >
> > Signed-off-by: Dhananjay Phadke <[email protected]>
> > ---
> > drivers/i2c/busses/i2c-bcm-iproc.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
> > index cceaf69279a9..6304d1dd2dd6 100644
> > --- a/drivers/i2c/busses/i2c-bcm-iproc.c
> > +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
> > @@ -1224,14 +1224,14 @@ static int bcm_iproc_i2c_unreg_slave(struct i2c_client *slave)
> >
> > disable_irq(iproc_i2c->irq);
> >
> > + tasklet_kill(&iproc_i2c->slave_rx_tasklet);
> > +
> > /* disable all slave interrupts */
> > tmp = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
> > tmp &= ~(IE_S_ALL_INTERRUPT_MASK <<
> > IE_S_ALL_INTERRUPT_SHIFT);
> > iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, tmp);
> >
> > - tasklet_kill(&iproc_i2c->slave_rx_tasklet);
> > -
> > /* Erase the slave address programmed */
> > tmp = iproc_i2c_rd_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET);
> > tmp &= ~BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT);
> >
>
> Yeah, needs to ensure no pending tasklet running to enable (unmask)
> slave interrupts before disable (mask) them.
>
> Thanks for the fix!
>
> Acked-by: Ray Jui <[email protected]>
>
Acked-by: Rayagonda Kokatanur <[email protected]>
On Thu, Aug 05, 2021 at 02:49:05PM -0700, Dhananjay Phadke wrote:
> Similar NULL deref was originally fixed by graceful teardown sequence -
>
> https://lore.kernel.org/linux-i2c/[email protected]
>
> After this, a tasklet was added to take care of FIFO full condition for large i2c
> transaction.
>
> https://lore.kernel.org/linux-arm-kernel/[email protected]/
>
> This introduced regression, a new race condition between tasklet enabling
> interrupts and client unreg teardown sequence.
>
> Kill tasklet before unreg_slave() masks bits in IE_OFFSET.
> Updated teardown sequence -
> (1) disable_irq()
> (2) Kill tasklet
> (3) Mask event enable bits in control reg
> (4) Erase slave address (avoid further writes to rx fifo)
> (5) Flush tx and rx FIFOs
> (6) Clear pending event (interrupt) bits in status reg
> (7) Set client pointer to NULL
> (8) enable_irq()
>
> --
>
> Unable to handle kernel read from unreadable memory at virtual address 0000000000000320
> Mem abort info:
> ESR = 0x96000004
> EC = 0x25: DABT (current EL), IL = 32 bits
> SET = 0, FnV = 0
> EA = 0, S1PTW = 0
> Data abort info:
> ISV = 0, ISS = 0x00000004
> CM = 0, WnR = 0
> user pgtable: 4k pages, 48-bit VAs, pgdp=000000009212a000
> [0000000000000320] pgd=0000000000000000, p4d=0000000000000000
> Internal error: Oops: 96000004 [#1] SMP
> CPU: 0 PID: 0 Comm: swapper/0 Tainted: G O
> Hardware name: Overlake (DT)
> pstate: 40400085 (nZcv daIf +PAN -UAO -TCO BTYPE=--)
> pc : bcm_iproc_i2c_slave_isr+0x2b8/0x8e4
> lr : bcm_iproc_i2c_slave_isr+0x1c8/0x8e4
> sp : ffff800010003e70
> x29: ffff800010003e80 x28: ffffda017acdc000
> x27: ffffda017b0ae000 x26: ffff800010004000
> x25: ffff800010000000 x24: ffffda017af4a168
> x23: 0000000000000073 x22: 0000000000000000
> x21: 0000000001400000 x20: 0000000001000000
> x19: ffff06f09583f880 x18: 00000000fa83b2da
> x17: 000000000000b67e x16: 0000000002edb2f3
> x15: 00000000000002c7 x14: 00000000000002c7
> x13: 0000000000000006 x12: 0000000000000033
> x11: 0000000000000000 x10: 0000000001000000
> x9 : 0000000003289312 x8 : 0000000003289311
> x7 : 02d0cd03a303adbc x6 : 02d18e7f0a4dfc6c
> x5 : 02edb2f33f76ea68 x4 : 00000000fa83b2da
> x3 : ffffda017af43cd0 x2 : ffff800010003e74
> x1 : 0000000001400000 x0 : 0000000000000000
> Call trace:
> bcm_iproc_i2c_slave_isr+0x2b8/0x8e4
> bcm_iproc_i2c_isr+0x178/0x290
> __handle_irq_event_percpu+0xd0/0x200
> handle_irq_event+0x60/0x1a0
> handle_fasteoi_irq+0x130/0x220
> __handle_domain_irq+0x8c/0xcc
> gic_handle_irq+0xc0/0x120
> el1_irq+0xcc/0x180
> finish_task_switch+0x100/0x1d8
> __schedule+0x61c/0x7a0
> schedule_idle+0x28/0x44
> do_idle+0x254/0x28c
> cpu_startup_entry+0x28/0x2c
> rest_init+0xc4/0xd0
> arch_call_rest_init+0x14/0x1c
> start_kernel+0x33c/0x3b8
> Code: f9423260 910013e2 11000509 b9047a69 (f9419009)
Applied to for-current, thanks!