[BUG] iio: light: opt3001: possible deadlock in opt3001_read_raw() and opt3001_irq()

[BUG] iio: light: opt3001: possible deadlock in opt3001_read_raw() and opt3001_irq()

[Jia-Ju Bai]

Hello,

My static analysis tool reports a possible deadlock in the opt3001 
driver in Linux 5.16:

opt3001_read_raw()
   mutex_lock(&opt->lock); --> Line 399 (Lock A)
   opt3001_get_lux()
     wait_event_timeout(opt->result_ready_queue, ...) --> Line 276 (Wait X)
   mutex_lock(&opt->lock); --> Line 412 (Unlock A)

opt3001_irq()
   mutex_lock(&opt->lock); --> Line 693 (Lock A)
   mutex_unlock(&opt->lock); --> Line 730 (Unlock A)
   wake_up(&opt->result_ready_queue); --> Line 733 (Wake X)

When opt3001_read_raw() is executed, "Wait X" is performed by holding 
"Lock A". If opt3001_irq() is executed at this time, "Wake X" cannot be 
performed to wake up "Wait X" in opt3001_read_raw(), because "Lock A" 
has been already hold by opt3001_read_raw(), causing a possible deadlock.
I find that "Wait X" is performed with a timeout, to relieve the 
possible deadlock; but I think this timeout can cause inefficient execution.

I am not quite sure whether this possible problem is real and how to fix 
it if it is real.
Any feedback would be appreciated, thanks :)


Best wishes,
Jia-Ju Bai

Re: [BUG] iio: light: opt3001: possible deadlock in opt3001_read_raw() and opt3001_irq()

[Jonathan Cameron]

On Mon, 7 Feb 2022 23:41:49 +0800
Jia-Ju Bai <baijiaju1990@gmail.com> wrote:

> Hello,
> 
> My static analysis tool reports a possible deadlock in the opt3001 
> driver in Linux 5.16:
> 
> opt3001_read_raw()
>    mutex_lock(&opt->lock); --> Line 399 (Lock A)
>    opt3001_get_lux()
>      wait_event_timeout(opt->result_ready_queue, ...) --> Line 276 (Wait X)
>    mutex_lock(&opt->lock); --> Line 412 (Unlock A)
> 
> opt3001_irq()
>    mutex_lock(&opt->lock); --> Line 693 (Lock A)
>    mutex_unlock(&opt->lock); --> Line 730 (Unlock A)
>    wake_up(&opt->result_ready_queue); --> Line 733 (Wake X)
> 
> When opt3001_read_raw() is executed, "Wait X" is performed by holding 
> "Lock A". If opt3001_irq() is executed at this time, "Wake X" cannot be 
> performed to wake up "Wait X" in opt3001_read_raw(), because "Lock A" 
> has been already hold by opt3001_read_raw(), causing a possible deadlock.
> I find that "Wait X" is performed with a timeout, to relieve the 
> possible deadlock; but I think this timeout can cause inefficient execution.
Hi Jia-Ju Bai,

There is a quirk in here thatyou haven't mentioned.  The "magic"
opt->ok_to_ignore_lock.

So there are two cases:
1) No irq in use.  In that case the opt3001_irq() will never run and we
   just sleep + check a status flag. That's the best we can do without
   an interrupt.
2) irq in use, the ok_to_ignore_lock = true statement occurs and
   in the opt3001_irq() the locks are never taken hence no deadlock.

It is a very odd bit of code though so I'd be surprised if a static
analyser hadn't highlighted it as a possible deadlock!

Now, I'm not immediately sure why the driver is done like this as opposed
to a more complete wait_for_completion() in read_raw(), complete() in the irq
handler and do the actual read of the data back in read_raw().
It's probably related to the other interrupt sources that we need to
differentiate from in the interrupt handler.

The lock definition is missing an documentation of exactly what it's scope
is which definitely doesn't help us understand this unusual structure.

Thanks,

Jonathan


> 
> I am not quite sure whether this possible problem is real and how to fix 
> it if it is real.
> Any feedback would be appreciated, thanks :)
> 
> 
> Best wishes,
> Jia-Ju Bai