Re: A potential bug in drivers/iio/light/opt3001.ko

From: Pavel Andrianov
Date: Mon Sep 05 2016 - 10:15:40 EST


03.09.2016 19:38, Jonathan Cameron ÐÐÑÐÑ:
On 31/08/16 11:23, Pavel Andrianov wrote:
Hi!

There is a bug in drivers/iio/light/opt3001.ko. Regard such case:

Thread 1 Thread 2
-> opt3001_read_raw
-> mutex_lock(&opt->lock)
-> opt3001_get_lux()
..
->i2c_smbus_write_word_swapped()
Now an interrupt comes
-> opt3001_irq
-> mutex_lock(&opt->lock)

This is a deadlock, as the flag ok_to_ignore_lock has not been set yet.
Good find. Will need reordering to set the ok_to_ignore_lock first.
Whether it ever actually happens will depend on just how long that EOC
interrupt takes to happen. Still it's a theoretical problem with
a fairly simple fix so let's fix it.

Regard another case:

Thread 1 Thread 2
-> opt3001_read_raw
-> mutex_lock(&opt->lock)
-> opt3001_get_lux()
..
-> i2c_smbus_write_word_swapped()
opt->ok_to_ignore_lock = true;
Now an interrupt comes
-> opt3001_irq
..
opt->result_ready = true
wake_up()
opt->result_ready = false;
wait_event_timeout()

In this case the first thread misses the result and waits until timeout expires.

Agreed - looks like some reordering is needed here as well.

Jonathan


In opt3001_get_lux has a comment, that i2c_smbus_write_word_swapped (line 246) enables interrupt mechanism. If an interrupt can not arise before the function, the assignments to both of flags should be moved before i2c_smbus_write_word_swapped and this is the best fix for both of issues.
Do you know if my assumption is correct and interrupts are disabled before i2c_smbus_write_word_swapped call?
--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andrianov@xxxxxxxxx