| Message ID | 20190919225418.20512-1-dpfrey@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: light: opt3001: fix mutex unlock race | expand |
David, On Thu, Sep 19, 2019 at 03:54:18PM -0700, David Frey wrote: > When an end-of-conversion interrupt is received after performing a > single-shot reading of the light sensor, the driver was waking up the > result ready queue before checking opt->ok_to_ignore_lock to determine > if it should unlock the mutex. The problem occurred in the case where > the other thread woke up and changed the value of opt->ok_to_ignore_lock > to false prior to the interrupt thread performing its read of the > variable. In this case, the mutex would be unlocked twice. > > Signed-off-by: David Frey <dpfrey@gmail.com> > --- Good find, thanks for the submission. Reviewed-by: Andreas Dannenberg <dannenberg@ti.com> -- Andreas Dannenberg Texas Instruments Inc > drivers/iio/light/opt3001.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c > index e666879007d2..92004a2563ea 100644 > --- a/drivers/iio/light/opt3001.c > +++ b/drivers/iio/light/opt3001.c > @@ -686,6 +686,7 @@ static irqreturn_t opt3001_irq(int irq, void *_iio) > struct iio_dev *iio = _iio; > struct opt3001 *opt = iio_priv(iio); > int ret; > + bool wake_result_ready_queue = false; > > if (!opt->ok_to_ignore_lock) > mutex_lock(&opt->lock); > @@ -720,13 +721,16 @@ static irqreturn_t opt3001_irq(int irq, void *_iio) > } > opt->result = ret; > opt->result_ready = true; > - wake_up(&opt->result_ready_queue); > + wake_result_ready_queue = true; > } > > out: > if (!opt->ok_to_ignore_lock) > mutex_unlock(&opt->lock); > > + if (wake_result_ready_queue) > + wake_up(&opt->result_ready_queue); > + > return IRQ_HANDLED; > } > > -- > 2.23.0 >
On Fri, 20 Sep 2019 12:40:37 -0500 Andreas Dannenberg <dannenberg@ti.com> wrote: > David, > > On Thu, Sep 19, 2019 at 03:54:18PM -0700, David Frey wrote: > > When an end-of-conversion interrupt is received after performing a > > single-shot reading of the light sensor, the driver was waking up the > > result ready queue before checking opt->ok_to_ignore_lock to determine > > if it should unlock the mutex. The problem occurred in the case where > > the other thread woke up and changed the value of opt->ok_to_ignore_lock > > to false prior to the interrupt thread performing its read of the > > variable. In this case, the mutex would be unlocked twice. > > > > Signed-off-by: David Frey <dpfrey@gmail.com> > > --- > > Good find, thanks for the submission. > > Reviewed-by: Andreas Dannenberg <dannenberg@ti.com> I think this goes all the way back to the initial driver so I've added a fixes tag for that and marked it for stable. Applied to the fixes-togreg branch of iio.git. Thanks, Jonathan > > > -- > Andreas Dannenberg > Texas Instruments Inc > > > drivers/iio/light/opt3001.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c > > index e666879007d2..92004a2563ea 100644 > > --- a/drivers/iio/light/opt3001.c > > +++ b/drivers/iio/light/opt3001.c > > @@ -686,6 +686,7 @@ static irqreturn_t opt3001_irq(int irq, void *_iio) > > struct iio_dev *iio = _iio; > > struct opt3001 *opt = iio_priv(iio); > > int ret; > > + bool wake_result_ready_queue = false; > > > > if (!opt->ok_to_ignore_lock) > > mutex_lock(&opt->lock); > > @@ -720,13 +721,16 @@ static irqreturn_t opt3001_irq(int irq, void *_iio) > > } > > opt->result = ret; > > opt->result_ready = true; > > - wake_up(&opt->result_ready_queue); > > + wake_result_ready_queue = true; > > } > > > > out: > > if (!opt->ok_to_ignore_lock) > > mutex_unlock(&opt->lock); > > > > + if (wake_result_ready_queue) > > + wake_up(&opt->result_ready_queue); > > + > > return IRQ_HANDLED; > > } > > > > -- > > 2.23.0 > >
diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c index e666879007d2..92004a2563ea 100644 --- a/drivers/iio/light/opt3001.c +++ b/drivers/iio/light/opt3001.c @@ -686,6 +686,7 @@ static irqreturn_t opt3001_irq(int irq, void *_iio) struct iio_dev *iio = _iio; struct opt3001 *opt = iio_priv(iio); int ret; + bool wake_result_ready_queue = false; if (!opt->ok_to_ignore_lock) mutex_lock(&opt->lock); @@ -720,13 +721,16 @@ static irqreturn_t opt3001_irq(int irq, void *_iio) } opt->result = ret; opt->result_ready = true; - wake_up(&opt->result_ready_queue); + wake_result_ready_queue = true; } out: if (!opt->ok_to_ignore_lock) mutex_unlock(&opt->lock); + if (wake_result_ready_queue) + wake_up(&opt->result_ready_queue); + return IRQ_HANDLED; }
When an end-of-conversion interrupt is received after performing a single-shot reading of the light sensor, the driver was waking up the result ready queue before checking opt->ok_to_ignore_lock to determine if it should unlock the mutex. The problem occurred in the case where the other thread woke up and changed the value of opt->ok_to_ignore_lock to false prior to the interrupt thread performing its read of the variable. In this case, the mutex would be unlocked twice. Signed-off-by: David Frey <dpfrey@gmail.com> --- drivers/iio/light/opt3001.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)