* [PATCH] iio: adis_lib: Initialize trigger before requesting interrupt
@ 2018-02-14 14:43 Lars-Peter Clausen
2018-02-17 14:18 ` Jonathan Cameron
0 siblings, 1 reply; 2+ messages in thread
From: Lars-Peter Clausen @ 2018-02-14 14:43 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Hartmut Knaack, Peter Meerwald-Stadler, linux-iio, Lars-Peter Clausen
The adis_probe_trigger() creates a new IIO trigger and requests an
interrupt associated with the trigger. The interrupt uses the generic
iio_trigger_generic_data_rdy_poll() function as its interrupt handler.
Currently the driver initializes some fields of the trigger structure after
the interrupt has been requested. But an interrupt can fire as soon as it
has been requested. This opens up a race condition.
iio_trigger_generic_data_rdy_poll() will access the trigger data structure
and dereference the ops field. If the ops field is not yet initialized this
will result in a NULL pointer deref.
It is not expected that the device generates an interrupt at this point, so
typically this issue did not surface unless e.g. due to a hardware
misconfiguration (wrong interrupt number, wrong polarity, etc.).
But some newer devices from the ADIS family start to generate periodic
interrupts in their power-on reset configuration and unfortunately the
interrupt can not be masked in the device. This makes the race condition
much more visible and the following crash has been observed occasionally
when booting a system using the ADIS16460.
Unable to handle kernel NULL pointer dereference at virtual address 00000008
pgd = c0004000
[00000008] *pgd=00000000
Internal error: Oops: 5 [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.9.0-04126-gf9739f0-dirty #257
Hardware name: Xilinx Zynq Platform
task: ef04f640 task.stack: ef050000
PC is at iio_trigger_notify_done+0x30/0x68
LR is at iio_trigger_generic_data_rdy_poll+0x18/0x20
pc : [<c042d868>] lr : [<c042d924>] psr: 60000193
sp : ef051bb8 ip : 00000000 fp : ef106400
r10: c081d80a r9 : ef3bfa00 r8 : 00000087
r7 : ef051bec r6 : 00000000 r5 : ef3bfa00 r4 : ee92ab00
r3 : 00000000 r2 : 00000000 r1 : 00000000 r0 : ee97e400
Flags: nZCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment none
Control: 18c5387d Table: 0000404a DAC: 00000051
Process swapper/0 (pid: 1, stack limit = 0xef050210)
[<c042d868>] (iio_trigger_notify_done) from [<c0065b10>] (__handle_irq_event_percpu+0x88/0x118)
[<c0065b10>] (__handle_irq_event_percpu) from [<c0065bbc>] (handle_irq_event_percpu+0x1c/0x58)
[<c0065bbc>] (handle_irq_event_percpu) from [<c0065c30>] (handle_irq_event+0x38/0x5c)
[<c0065c30>] (handle_irq_event) from [<c0068e28>] (handle_level_irq+0xa4/0x130)
[<c0068e28>] (handle_level_irq) from [<c0064e74>] (generic_handle_irq+0x24/0x34)
[<c0064e74>] (generic_handle_irq) from [<c021ab7c>] (zynq_gpio_irqhandler+0xb8/0x13c)
[<c021ab7c>] (zynq_gpio_irqhandler) from [<c0064e74>] (generic_handle_irq+0x24/0x34)
[<c0064e74>] (generic_handle_irq) from [<c0065370>] (__handle_domain_irq+0x5c/0xb4)
[<c0065370>] (__handle_domain_irq) from [<c000940c>] (gic_handle_irq+0x48/0x8c)
[<c000940c>] (gic_handle_irq) from [<c0013e8c>] (__irq_svc+0x6c/0xa8)
To fix this make sure that the trigger is fully initialized before
requesting the interrupt.
Fixes: ccd2b52f4ac6 ("staging:iio: Add common ADIS library")
Reported-by: Robin Getz <Robin.Getz@analog.com>
Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
drivers/iio/imu/adis_trigger.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/imu/adis_trigger.c b/drivers/iio/imu/adis_trigger.c
index 0dd5a381be64..457372f36791 100644
--- a/drivers/iio/imu/adis_trigger.c
+++ b/drivers/iio/imu/adis_trigger.c
@@ -46,6 +46,10 @@ int adis_probe_trigger(struct adis *adis, struct iio_dev *indio_dev)
if (adis->trig == NULL)
return -ENOMEM;
+ adis->trig->dev.parent = &adis->spi->dev;
+ adis->trig->ops = &adis_trigger_ops;
+ iio_trigger_set_drvdata(adis->trig, adis);
+
ret = request_irq(adis->spi->irq,
&iio_trigger_generic_data_rdy_poll,
IRQF_TRIGGER_RISING,
@@ -54,9 +58,6 @@ int adis_probe_trigger(struct adis *adis, struct iio_dev *indio_dev)
if (ret)
goto error_free_trig;
- adis->trig->dev.parent = &adis->spi->dev;
- adis->trig->ops = &adis_trigger_ops;
- iio_trigger_set_drvdata(adis->trig, adis);
ret = iio_trigger_register(adis->trig);
indio_dev->trig = iio_trigger_get(adis->trig);
--
2.11.0
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] iio: adis_lib: Initialize trigger before requesting interrupt
2018-02-14 14:43 [PATCH] iio: adis_lib: Initialize trigger before requesting interrupt Lars-Peter Clausen
@ 2018-02-17 14:18 ` Jonathan Cameron
0 siblings, 0 replies; 2+ messages in thread
From: Jonathan Cameron @ 2018-02-17 14:18 UTC (permalink / raw)
To: Lars-Peter Clausen; +Cc: Hartmut Knaack, Peter Meerwald-Stadler, linux-iio
On Wed, 14 Feb 2018 15:43:00 +0100
Lars-Peter Clausen <lars@metafoo.de> wrote:
> The adis_probe_trigger() creates a new IIO trigger and requests an
> interrupt associated with the trigger. The interrupt uses the generic
> iio_trigger_generic_data_rdy_poll() function as its interrupt handler.
>
> Currently the driver initializes some fields of the trigger structure after
> the interrupt has been requested. But an interrupt can fire as soon as it
> has been requested. This opens up a race condition.
>
> iio_trigger_generic_data_rdy_poll() will access the trigger data structure
> and dereference the ops field. If the ops field is not yet initialized this
> will result in a NULL pointer deref.
>
> It is not expected that the device generates an interrupt at this point, so
> typically this issue did not surface unless e.g. due to a hardware
> misconfiguration (wrong interrupt number, wrong polarity, etc.).
>
> But some newer devices from the ADIS family start to generate periodic
> interrupts in their power-on reset configuration and unfortunately the
> interrupt can not be masked in the device. This makes the race condition
> much more visible and the following crash has been observed occasionally
> when booting a system using the ADIS16460.
>
> Unable to handle kernel NULL pointer dereference at virtual address 00000008
> pgd = c0004000
> [00000008] *pgd=00000000
> Internal error: Oops: 5 [#1] PREEMPT SMP ARM
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.9.0-04126-gf9739f0-dirty #257
> Hardware name: Xilinx Zynq Platform
> task: ef04f640 task.stack: ef050000
> PC is at iio_trigger_notify_done+0x30/0x68
> LR is at iio_trigger_generic_data_rdy_poll+0x18/0x20
> pc : [<c042d868>] lr : [<c042d924>] psr: 60000193
> sp : ef051bb8 ip : 00000000 fp : ef106400
> r10: c081d80a r9 : ef3bfa00 r8 : 00000087
> r7 : ef051bec r6 : 00000000 r5 : ef3bfa00 r4 : ee92ab00
> r3 : 00000000 r2 : 00000000 r1 : 00000000 r0 : ee97e400
> Flags: nZCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment none
> Control: 18c5387d Table: 0000404a DAC: 00000051
> Process swapper/0 (pid: 1, stack limit = 0xef050210)
> [<c042d868>] (iio_trigger_notify_done) from [<c0065b10>] (__handle_irq_event_percpu+0x88/0x118)
> [<c0065b10>] (__handle_irq_event_percpu) from [<c0065bbc>] (handle_irq_event_percpu+0x1c/0x58)
> [<c0065bbc>] (handle_irq_event_percpu) from [<c0065c30>] (handle_irq_event+0x38/0x5c)
> [<c0065c30>] (handle_irq_event) from [<c0068e28>] (handle_level_irq+0xa4/0x130)
> [<c0068e28>] (handle_level_irq) from [<c0064e74>] (generic_handle_irq+0x24/0x34)
> [<c0064e74>] (generic_handle_irq) from [<c021ab7c>] (zynq_gpio_irqhandler+0xb8/0x13c)
> [<c021ab7c>] (zynq_gpio_irqhandler) from [<c0064e74>] (generic_handle_irq+0x24/0x34)
> [<c0064e74>] (generic_handle_irq) from [<c0065370>] (__handle_domain_irq+0x5c/0xb4)
> [<c0065370>] (__handle_domain_irq) from [<c000940c>] (gic_handle_irq+0x48/0x8c)
> [<c000940c>] (gic_handle_irq) from [<c0013e8c>] (__irq_svc+0x6c/0xa8)
>
> To fix this make sure that the trigger is fully initialized before
> requesting the interrupt.
>
> Fixes: ccd2b52f4ac6 ("staging:iio: Add common ADIS library")
> Reported-by: Robin Getz <Robin.Getz@analog.com>
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Applied to the fixes-togreg-post-rc1 branch of iio.git and marked
for stable.
Thanks,
Jonathan
> ---
> drivers/iio/imu/adis_trigger.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/imu/adis_trigger.c b/drivers/iio/imu/adis_trigger.c
> index 0dd5a381be64..457372f36791 100644
> --- a/drivers/iio/imu/adis_trigger.c
> +++ b/drivers/iio/imu/adis_trigger.c
> @@ -46,6 +46,10 @@ int adis_probe_trigger(struct adis *adis, struct iio_dev *indio_dev)
> if (adis->trig == NULL)
> return -ENOMEM;
>
> + adis->trig->dev.parent = &adis->spi->dev;
> + adis->trig->ops = &adis_trigger_ops;
> + iio_trigger_set_drvdata(adis->trig, adis);
> +
> ret = request_irq(adis->spi->irq,
> &iio_trigger_generic_data_rdy_poll,
> IRQF_TRIGGER_RISING,
> @@ -54,9 +58,6 @@ int adis_probe_trigger(struct adis *adis, struct iio_dev *indio_dev)
> if (ret)
> goto error_free_trig;
>
> - adis->trig->dev.parent = &adis->spi->dev;
> - adis->trig->ops = &adis_trigger_ops;
> - iio_trigger_set_drvdata(adis->trig, adis);
> ret = iio_trigger_register(adis->trig);
>
> indio_dev->trig = iio_trigger_get(adis->trig);
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2018-02-17 14:18 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-14 14:43 [PATCH] iio: adis_lib: Initialize trigger before requesting interrupt Lars-Peter Clausen
2018-02-17 14:18 ` Jonathan Cameron
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.