[PATCH] iio: adc: ina2xx: avoid kthread_stop() with stale task_struct

[PATCH] iio: adc: ina2xx: avoid kthread_stop() with stale task_struct

[Akinobu Mita]

When the buffer is enabled for ina2xx driver, a dedicated kthread is
invoked to capture mesurement data.  When the buffer is disabled, the
kthread is stopped.

However if the kthread gets register access errors, it immediately exits
and when the malfunctional buffer is disabled, the stale task_struct
pointer is accessed as there is no kthread to be stopped.

A similar issue in the usbip driver is prevented by kthread_get_run and
kthread_stop_put helpers by increasing usage count of the task_struct.
This change applies the same solution.

Cc: Stefan Brüns <stefan.bruens@rwth-aachen.de>
Cc: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/iio/adc/ina2xx-adc.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 0635a79..d123962 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -30,6 +30,7 @@
 #include <linux/module.h>
 #include <linux/of_device.h>
 #include <linux/regmap.h>
+#include <linux/sched/task.h>
 #include <linux/util_macros.h>
 
 #include <linux/platform_data/ina2xx.h>
@@ -826,6 +827,7 @@ static int ina2xx_buffer_enable(struct iio_dev *indio_dev)
 {
 	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
 	unsigned int sampling_us = SAMPLING_PERIOD(chip);
+	struct task_struct *task;
 
 	dev_dbg(&indio_dev->dev, "Enabling buffer w/ scan_mask %02x, freq = %d, avg =%u\n",
 		(unsigned int)(*indio_dev->active_scan_mask),
@@ -835,11 +837,17 @@ static int ina2xx_buffer_enable(struct iio_dev *indio_dev)
 	dev_dbg(&indio_dev->dev, "Async readout mode: %d\n",
 		chip->allow_async_readout);
 
-	chip->task = kthread_run(ina2xx_capture_thread, (void *)indio_dev,
-				 "%s:%d-%uus", indio_dev->name, indio_dev->id,
-				 sampling_us);
+	task = kthread_create(ina2xx_capture_thread, (void *)indio_dev,
+			      "%s:%d-%uus", indio_dev->name, indio_dev->id,
+			      sampling_us);
+	if (IS_ERR(task))
+		return PTR_ERR(task);
+
+	get_task_struct(task);
+	wake_up_process(task);
+	chip->task = task;
 
-	return PTR_ERR_OR_ZERO(chip->task);
+	return 0;
 }
 
 static int ina2xx_buffer_disable(struct iio_dev *indio_dev)
@@ -848,6 +856,7 @@ static int ina2xx_buffer_disable(struct iio_dev *indio_dev)
 
 	if (chip->task) {
 		kthread_stop(chip->task);
+		put_task_struct(chip->task);
 		chip->task = NULL;
 	}
 
-- 
2.7.4

Re: [PATCH] iio: adc: ina2xx: avoid kthread_stop() with stale task_struct

[Jonathan Cameron]

On Mon, 25 Jun 2018 00:05:21 +0900
Akinobu Mita <akinobu.mita@gmail.com> wrote:

> When the buffer is enabled for ina2xx driver, a dedicated kthread is
> invoked to capture mesurement data.  When the buffer is disabled, the
> kthread is stopped.
>=20
> However if the kthread gets register access errors, it immediately exits
> and when the malfunctional buffer is disabled, the stale task_struct
> pointer is accessed as there is no kthread to be stopped.
>=20
> A similar issue in the usbip driver is prevented by kthread_get_run and
> kthread_stop_put helpers by increasing usage count of the task_struct.
> This change applies the same solution.
>=20
> Cc: Stefan Br=C3=BCns <stefan.bruens@rwth-aachen.de>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Seems fine, but this is a fix so should have an appropriate fixes
tag.  Feel free to send one in reply to this thread rather than a v2.

Without a fixes tag it can be very hard to know exactly where
a patch 'should' apply.  I also have little visibility on
how important backporting htis issue is.  What would actually trigger
the issue and is it likely to be seen in the wild?

Thanks,

Jonathan
> ---
>  drivers/iio/adc/ina2xx-adc.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>=20
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index 0635a79..d123962 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -30,6 +30,7 @@
>  #include <linux/module.h>
>  #include <linux/of_device.h>
>  #include <linux/regmap.h>
> +#include <linux/sched/task.h>
>  #include <linux/util_macros.h>
> =20
>  #include <linux/platform_data/ina2xx.h>
> @@ -826,6 +827,7 @@ static int ina2xx_buffer_enable(struct iio_dev *indio=
_dev)
>  {
>  	struct ina2xx_chip_info *chip =3D iio_priv(indio_dev);
>  	unsigned int sampling_us =3D SAMPLING_PERIOD(chip);
> +	struct task_struct *task;
> =20
>  	dev_dbg(&indio_dev->dev, "Enabling buffer w/ scan_mask %02x, freq =3D %=
d, avg =3D%u\n",
>  		(unsigned int)(*indio_dev->active_scan_mask),
> @@ -835,11 +837,17 @@ static int ina2xx_buffer_enable(struct iio_dev *ind=
io_dev)
>  	dev_dbg(&indio_dev->dev, "Async readout mode: %d\n",
>  		chip->allow_async_readout);
> =20
> -	chip->task =3D kthread_run(ina2xx_capture_thread, (void *)indio_dev,
> -				 "%s:%d-%uus", indio_dev->name, indio_dev->id,
> -				 sampling_us);
> +	task =3D kthread_create(ina2xx_capture_thread, (void *)indio_dev,
> +			      "%s:%d-%uus", indio_dev->name, indio_dev->id,
> +			      sampling_us);
> +	if (IS_ERR(task))
> +		return PTR_ERR(task);
> +
> +	get_task_struct(task);
> +	wake_up_process(task);
> +	chip->task =3D task;
> =20
> -	return PTR_ERR_OR_ZERO(chip->task);
> +	return 0;
>  }
> =20
>  static int ina2xx_buffer_disable(struct iio_dev *indio_dev)
> @@ -848,6 +856,7 @@ static int ina2xx_buffer_disable(struct iio_dev *indi=
o_dev)
> =20
>  	if (chip->task) {
>  		kthread_stop(chip->task);
> +		put_task_struct(chip->task);
>  		chip->task =3D NULL;
>  	}
> =20

Re: [PATCH] iio: adc: ina2xx: avoid kthread_stop() with stale task_struct

[Akinobu Mita]

2018=E5=B9=B47=E6=9C=881=E6=97=A5(=E6=97=A5) 2:40 Jonathan Cameron <jic23@k=
ernel.org>:
>
> On Mon, 25 Jun 2018 00:05:21 +0900
> Akinobu Mita <akinobu.mita@gmail.com> wrote:
>
> > When the buffer is enabled for ina2xx driver, a dedicated kthread is
> > invoked to capture mesurement data.  When the buffer is disabled, the
> > kthread is stopped.
> >
> > However if the kthread gets register access errors, it immediately exit=
s
> > and when the malfunctional buffer is disabled, the stale task_struct
> > pointer is accessed as there is no kthread to be stopped.
> >
> > A similar issue in the usbip driver is prevented by kthread_get_run and
> > kthread_stop_put helpers by increasing usage count of the task_struct.
> > This change applies the same solution.
> >
> > Cc: Stefan Br=C3=BCns <stefan.bruens@rwth-aachen.de>
> > Cc: Jonathan Cameron <jic23@kernel.org>
> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> Seems fine, but this is a fix so should have an appropriate fixes
> tag.  Feel free to send one in reply to this thread rather than a v2.

It seems like this problem has been there ever since the ina2xx driver
was added.

Fixes: c43a102e67db ("iio: ina2xx: add support for TI INA2xx Power Monitors=
")

> Without a fixes tag it can be very hard to know exactly where
> a patch 'should' apply.  I also have little visibility on
> how important backporting htis issue is.  What would actually trigger
> the issue and is it likely to be seen in the wild?

This issue was actually triggered in the system with an I2C controller
that occasionally malfunctions due to a software bug in the controller
driver.

BTW, there is no way to notify this error to the process that is calling
poll() or read() to the buffer.  Maybe we should implement the mechanism
for returning POLLHUP or POLLERR events.

Re: [PATCH] iio: adc: ina2xx: avoid kthread_stop() with stale task_struct

[Jonathan Cameron]

On Mon, 2 Jul 2018 17:44:42 +0900
Akinobu Mita <akinobu.mita@gmail.com> wrote:

> 2018=E5=B9=B47=E6=9C=881=E6=97=A5(=E6=97=A5) 2:40 Jonathan Cameron <jic23=
@kernel.org>:
> >
> > On Mon, 25 Jun 2018 00:05:21 +0900
> > Akinobu Mita <akinobu.mita@gmail.com> wrote:
> > =20
> > > When the buffer is enabled for ina2xx driver, a dedicated kthread is
> > > invoked to capture mesurement data.  When the buffer is disabled, the
> > > kthread is stopped.
> > >
> > > However if the kthread gets register access errors, it immediately ex=
its
> > > and when the malfunctional buffer is disabled, the stale task_struct
> > > pointer is accessed as there is no kthread to be stopped.
> > >
> > > A similar issue in the usbip driver is prevented by kthread_get_run a=
nd
> > > kthread_stop_put helpers by increasing usage count of the task_struct.
> > > This change applies the same solution.
> > >
> > > Cc: Stefan Br=C3=BCns <stefan.bruens@rwth-aachen.de>
> > > Cc: Jonathan Cameron <jic23@kernel.org>
> > > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> =20
> > Seems fine, but this is a fix so should have an appropriate fixes
> > tag.  Feel free to send one in reply to this thread rather than a v2. =
=20
>=20
> It seems like this problem has been there ever since the ina2xx driver
> was added.
>=20
> Fixes: c43a102e67db ("iio: ina2xx: add support for TI INA2xx Power Monito=
rs")
>=20
Thanks, added.
> > Without a fixes tag it can be very hard to know exactly where
> > a patch 'should' apply.  I also have little visibility on
> > how important backporting htis issue is.  What would actually trigger
> > the issue and is it likely to be seen in the wild? =20
>=20
> This issue was actually triggered in the system with an I2C controller
> that occasionally malfunctions due to a software bug in the controller
> driver.

In that case I'm not going to rush it, but rather will take it via the
next merge window.

>=20
> BTW, there is no way to notify this error to the process that is calling
> poll() or read() to the buffer.  Maybe we should implement the mechanism
> for returning POLLHUP or POLLERR events.

I'm certainly not against adding something like that to notify on error
cases.  In general, minor hardware failures are not well handled in the
kernel - there is no 'standard' way of doing it.  Every now and then
it is discussed but no one ever takes it any further.

Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.

Thanks,

Jonathan

> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html