A potential data race in drivers/media/platform/s5p-mfc/

[Pavel Andrianov]

Hi,

s5p_mfc_probe[1] registers an interrupt handler s5p_mfc_irq before compete 
initialization. For example, the interrupt handler operates
with mfc_ops, which are set up in [2]. So, potentially, the interrupt handler may be 
executed in parallel with initialization. The question is if the device can produce the 
interrupts. Its registers are initialized in [3] and there are nothing like 
"enabling interrupts". So, likely, they are activated. And if interrupts can 
come, then this is a data race.

Best regards,
Pavel Andrianov
Linux Verification Center, ISPRAS
web:http://linuxtesting.org

[1]https://elixir.bootlin.com/linux/v5.4.106/source/drivers/media/platform/s5p-mfc/s5p_mfc.c#L1299
[2]https://elixir.bootlin.com/linux/v5.4.106/source/drivers/media/platform/s5p-mfc/s5p_mfc_opr.c#L19
[3]https://elixir.bootlin.com/linux/v5.4.106/source/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c#L2229

A potential data race in drivers/iio/adc/berlin2-adc.ko

[Pavel Andrianov]

Hi,

berlin2_adc_probe [1] registers two interrupt handlers: berlin2_adc_irq [2]
and berlin2_adc_tsen_irq [3]. The interrupt handlers operate with the 
same data, for example, modify

priv->data with different masks:

priv->data &= BERLIN2_SM_ADC_MASK;
and
priv->data &= BERLIN2_SM_TSEN_MASK;

If the two interrupt handlers are executed simultaneously, a potential 
data race takes place. So, the question is if the situation is possible. 
For example, in the case of the handlers are executed on different CPU 
cores.


Best regards,
Pavel

[1] 
https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/berlin2-adc.c#L283 

[2] 
https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/berlin2-adc.c#L239 

[3] 
https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/berlin2-adc.c#L259

[PATCH] net: pxa168_eth: Fix a potential data race in pxa168_eth_remove

[Pavel Andrianov]

pxa168_eth_remove() firstly calls unregister_netdev(),
then cancels a timeout work. unregister_netdev() shuts down a device
interface and removes it from the kernel tables. If the timeout occurs
in parallel, the timeout work (pxa168_eth_tx_timeout_task) performs stop
and open of the device. It may lead to an inconsistent state and memory
leaks.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Pavel Andrianov 
---
 drivers/net/ethernet/marvell/pxa168_eth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/pxa168_eth.c 
b/drivers/net/ethernet/marvell/pxa168_eth.c
index d1e4d42e497d..3712e1786091 100644
--- a/drivers/net/ethernet/marvell/pxa168_eth.c
+++ b/drivers/net/ethernet/marvell/pxa168_eth.c
@@ -1544,8 +1544,8 @@ static int pxa168_eth_remove(struct platform_device *pdev)
clk_disable_unprepare(pep->clk);
mdiobus_unregister(pep->smi_bus);
mdiobus_free(pep->smi_bus);
-   unregister_netdev(dev);
cancel_work_sync(>tx_timeout_task);
+   unregister_netdev(dev);
free_netdev(dev);
return 0;
 }
-- 
2.25.1

A potential bug in drivers/usb/gadget/udc/m66592-udc.ko

[Pavel Andrianov]

Hi!

There is a potential bug in drivers/usb/gadget/udc/m66592-udc.ko.
In m66592_probe interrupts are requested at line 1612. After that 
initialization of common resources is continued. For example, in


-> usb_add_gadget_udc (line 1678)
  -> usb_add_gadget_udc_release
-> udc_bind_to_driver
  -> usb_gadget_udc_start
-> m66592_udc_start

m66592->driver is set. In interrupt handler the data is used, thus if 
interrupt comes before udc_start is executed, null pointer dereference 
occurs.

Should the call of request_irq be after complete initialization?

--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

A potential bug in drivers/usb/gadget/udc/m66592-udc.ko

[Pavel Andrianov]

Hi!

There is a potential bug in drivers/usb/gadget/udc/m66592-udc.ko.
In m66592_probe interrupts are requested at line 1612. After that 
initialization of common resources is continued. For example, in


-> usb_add_gadget_udc (line 1678)
  -> usb_add_gadget_udc_release
-> udc_bind_to_driver
  -> usb_gadget_udc_start
-> m66592_udc_start

m66592->driver is set. In interrupt handler the data is used, thus if 
interrupt comes before udc_start is executed, null pointer dereference 
occurs.

Should the call of request_irq be after complete initialization?

--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

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

[Pavel Andrianov]

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(>lock)
  -> opt3001_get_lux()
..
->i2c_smbus_write_word_swapped()
Now an interrupt comes
 -> opt3001_irq
   -> mutex_lock(>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(>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: andria...@ispras.ru

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

[Pavel Andrianov]

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(>lock)
  -> opt3001_get_lux()
..
->i2c_smbus_write_word_swapped()
Now an interrupt comes
 -> opt3001_irq
   -> mutex_lock(>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(>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: andria...@ispras.ru

[PATCH] speakup: Add spinlock in synth_direct_store

[Pavel Andrianov]

All operations with synth buffer should be protected,
as there are global pointers, which should be modified atomically.

Found by Linux Driver Verification project (linuxtesting.org)

Signed-off-by: Pavel Andrianov <andria...@ispras.ru>
---
 drivers/staging/speakup/kobjects.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/speakup/kobjects.c 
b/drivers/staging/speakup/kobjects.c
index 528cbdc..7fedee3 100644
--- a/drivers/staging/speakup/kobjects.c
+++ b/drivers/staging/speakup/kobjects.c
@@ -411,11 +411,13 @@ static ssize_t synth_direct_store(struct kobject *kobj,
int len;
int bytes;
const char *ptr = buf;
+   unsigned long flags;
 
if (!synth)
return -EPERM;
 
len = strlen(buf);
+   spin_lock_irqsave(_info.spinlock, flags);
while (len > 0) {
bytes = min_t(size_t, len, 250);
strncpy(tmp, ptr, bytes);
@@ -425,6 +427,7 @@ static ssize_t synth_direct_store(struct kobject *kobj,
ptr += bytes;
len -= bytes;
}
+   spin_unlock_irqrestore(_info.spinlock, flags);
return count;
 }
 
-- 
2.7.4

[PATCH] speakup: Add spinlock in synth_direct_store

[Pavel Andrianov]

All operations with synth buffer should be protected,
as there are global pointers, which should be modified atomically.

Found by Linux Driver Verification project (linuxtesting.org)

Signed-off-by: Pavel Andrianov 
---
 drivers/staging/speakup/kobjects.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/speakup/kobjects.c 
b/drivers/staging/speakup/kobjects.c
index 528cbdc..7fedee3 100644
--- a/drivers/staging/speakup/kobjects.c
+++ b/drivers/staging/speakup/kobjects.c
@@ -411,11 +411,13 @@ static ssize_t synth_direct_store(struct kobject *kobj,
int len;
int bytes;
const char *ptr = buf;
+   unsigned long flags;
 
if (!synth)
return -EPERM;
 
len = strlen(buf);
+   spin_lock_irqsave(_info.spinlock, flags);
while (len > 0) {
bytes = min_t(size_t, len, 250);
strncpy(tmp, ptr, bytes);
@@ -425,6 +427,7 @@ static ssize_t synth_direct_store(struct kobject *kobj,
ptr += bytes;
len -= bytes;
}
+   spin_unlock_irqrestore(_info.spinlock, flags);
return count;
 }
 
-- 
2.7.4

Re: A potential race in drivers/staging/speakup/speakup.ko

[Pavel Andrianov]

05.09.2016 12:56, Samuel Thibault пишет:

Pavel Andrianov, on Mon 05 Sep 2016 12:54:10 +0300, wrote:

05.09.2016 12:43, Samuel Thibault пишет:

Pavel Andrianov, on Mon 05 Sep 2016 11:51:50 +0300, wrote:

There is a potential race in drivers/staging/speakup/speakup.ko.
All operations with global pointers buff_in and buff_out are performed
without any locks. Thus, a simultaneous write (via synth_buffer_clear or
synth_buffer_add) to the pointers may lead to inconsistent data.

Should a local lock be used here?


AIUI, all callers of these functions have speakup_info.spinlock held.


Regard a call stack

-> synth_direct_store
  -> synth_printf
-> synth_buffer_add

The functions have not held speakup_info.spinlock.


Apparently there is currently no caller of synth_direct_store and
synth_store. But taking the lock here would be needed indeed.

Samuel



synth_direct_store may be called via device_attributes interface. In 
which function the lock should be added?


--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

Re: A potential race in drivers/staging/speakup/speakup.ko

[Pavel Andrianov]

05.09.2016 12:56, Samuel Thibault пишет:

Pavel Andrianov, on Mon 05 Sep 2016 12:54:10 +0300, wrote:

05.09.2016 12:43, Samuel Thibault пишет:

Pavel Andrianov, on Mon 05 Sep 2016 11:51:50 +0300, wrote:

There is a potential race in drivers/staging/speakup/speakup.ko.
All operations with global pointers buff_in and buff_out are performed
without any locks. Thus, a simultaneous write (via synth_buffer_clear or
synth_buffer_add) to the pointers may lead to inconsistent data.

Should a local lock be used here?


AIUI, all callers of these functions have speakup_info.spinlock held.


Regard a call stack

-> synth_direct_store
  -> synth_printf
-> synth_buffer_add

The functions have not held speakup_info.spinlock.


Apparently there is currently no caller of synth_direct_store and
synth_store. But taking the lock here would be needed indeed.

Samuel



synth_direct_store may be called via device_attributes interface. In 
which function the lock should be added?


--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

Re: A potential race in drivers/staging/speakup/speakup.ko

[Pavel Andrianov]

05.09.2016 12:43, Samuel Thibault пишет:

Hello,

Pavel Andrianov, on Mon 05 Sep 2016 11:51:50 +0300, wrote:

There is a potential race in drivers/staging/speakup/speakup.ko.
All operations with global pointers buff_in and buff_out are performed
without any locks. Thus, a simultaneous write (via synth_buffer_clear or
synth_buffer_add) to the pointers may lead to inconsistent data.

Should a local lock be used here?


AIUI, all callers of these functions have speakup_info.spinlock held.

Samuel



Regard a call stack

-> synth_direct_store
  -> synth_printf
-> synth_buffer_add

The functions have not held speakup_info.spinlock.

--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

Re: A potential race in drivers/staging/speakup/speakup.ko

[Pavel Andrianov]

05.09.2016 12:43, Samuel Thibault пишет:

Hello,

Pavel Andrianov, on Mon 05 Sep 2016 11:51:50 +0300, wrote:

There is a potential race in drivers/staging/speakup/speakup.ko.
All operations with global pointers buff_in and buff_out are performed
without any locks. Thus, a simultaneous write (via synth_buffer_clear or
synth_buffer_add) to the pointers may lead to inconsistent data.

Should a local lock be used here?


AIUI, all callers of these functions have speakup_info.spinlock held.

Samuel



Regard a call stack

-> synth_direct_store
  -> synth_printf
-> synth_buffer_add

The functions have not held speakup_info.spinlock.

--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

A potential bug in drivers/tty/serial/jsm/jsm.ko

[Pavel Andrianov]

Hi!

There is a potential bug in drivers/tty/serial/jsm/jsm.ko.
In neo_parse_isr, line 764 a field ch->ch_flags is updated under 
spinlock protection. In neo_copy_data_from_uart_to_queue the same values 
are written to the field without any protection, as the function is 
always called without locks.

Should the same lock be used in these cases?
--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

A potential bug in drivers/tty/serial/jsm/jsm.ko

[Pavel Andrianov]

Hi!

There is a potential bug in drivers/tty/serial/jsm/jsm.ko.
In neo_parse_isr, line 764 a field ch->ch_flags is updated under 
spinlock protection. In neo_copy_data_from_uart_to_queue the same values 
are written to the field without any protection, as the function is 
always called without locks.

Should the same lock be used in these cases?
--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

A potential race in drivers/staging/speakup/speakup.ko

[Pavel Andrianov]

Hi!

There is a potential race in drivers/staging/speakup/speakup.ko.
All operations with global pointers buff_in and buff_out are performed 
without any locks. Thus, a simultaneous write (via synth_buffer_clear or 
synth_buffer_add) to the pointers may lead to inconsistent data.


Should a local lock be used here?

--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

A potential race in drivers/staging/speakup/speakup.ko

[Pavel Andrianov]

Hi!

There is a potential race in drivers/staging/speakup/speakup.ko.
All operations with global pointers buff_in and buff_out are performed 
without any locks. Thus, a simultaneous write (via synth_buffer_clear or 
synth_buffer_add) to the pointers may lead to inconsistent data.


Should a local lock be used here?

--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

A potential bug in drivers/net/ethernet/synopsys/dwc_eth_qos.ko

[Pavel Andrianov]

Hi!

There is a potential bug in 
drivers/net/ethernet/synopsys/dwc_eth_qos.ko. In dwceqos_probe there is 
a registration of net device (line 2879). After that initialization of 
common resources is continued (lines 2918, 2924, 2941), but at the 
moment handlers from net device may be already executed.


Should the registration of net device be at the end of dwceqos_probe?

--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

A potential bug in drivers/net/ethernet/synopsys/dwc_eth_qos.ko

[Pavel Andrianov]

Hi!

There is a potential bug in 
drivers/net/ethernet/synopsys/dwc_eth_qos.ko. In dwceqos_probe there is 
a registration of net device (line 2879). After that initialization of 
common resources is continued (lines 2918, 2924, 2941), but at the 
moment handlers from net device may be already executed.


Should the registration of net device be at the end of dwceqos_probe?

--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

A potential race in drivers/iio/adc/vf610_adc.ko

[Pavel Andrianov]

Hi!

There is a potential race in drivers/iio/adc/vf610_adc.ko.
Handlers vf610_set_conversion_mode and vf610_write_raw are called via 
device_attibute interface, but they are related to different attributes, 
so may be executed in parallel. vf610_set_conversion_mode acquires the 
mutex indio_dev->mlock, and vf610_write_raw does not. Thus updating the 
structure 'info' may be performed simultaneously.


Should vf610_write_raw also acquire the same mutex indio_dev->mlock?

--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

A potential race in drivers/iio/adc/vf610_adc.ko

[Pavel Andrianov]

Hi!

There is a potential race in drivers/iio/adc/vf610_adc.ko.
Handlers vf610_set_conversion_mode and vf610_write_raw are called via 
device_attibute interface, but they are related to different attributes, 
so may be executed in parallel. vf610_set_conversion_mode acquires the 
mutex indio_dev->mlock, and vf610_write_raw does not. Thus updating the 
structure 'info' may be performed simultaneously.


Should vf610_write_raw also acquire the same mutex indio_dev->mlock?

--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

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

[Pavel Andrianov]

Hi!

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

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

This is a deadlock, as the flag ok_to_ignore_lock has not been set yet.

Regard another case:

Thread 1 Thread 2
-> opt3001_read_raw
  -> mutex_lock(>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.


--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

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

[Pavel Andrianov]

Hi!

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

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

This is a deadlock, as the flag ok_to_ignore_lock has not been set yet.

Regard another case:

Thread 1 Thread 2
-> opt3001_read_raw
  -> mutex_lock(>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.


--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

[PATCH] smc91c92_cs : add a spinlock to avoid race condition

[Pavel Andrianov]

smc_reset may be executed in parallel with timer function media_check.
To avoid data race in smc_set_xcvr a spinlock was added.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Pavel Andrianov <andria...@ispras.ru>
---
 drivers/net/ethernet/smsc/smc91c92_cs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/smsc/smc91c92_cs.c 
b/drivers/net/ethernet/smsc/smc91c92_cs.c
index db3c696..69d865c 100644
--- a/drivers/net/ethernet/smsc/smc91c92_cs.c
+++ b/drivers/net/ethernet/smsc/smc91c92_cs.c
@@ -1637,6 +1637,7 @@ static void smc_reset(struct net_device *dev)
 unsigned int ioaddr = dev->base_addr;
 struct smc_private *smc = netdev_priv(dev);
 int i;
+unsigned long flags;
 
 netdev_dbg(dev, "smc91c92 reset called.\n");
 
@@ -1647,6 +1648,7 @@ static void smc_reset(struct net_device *dev)
 outw(RCR_SOFTRESET, ioaddr + RCR);
 udelay(10);
 
+spin_lock_irqsave(>lock, flags);
 /* Clear the transmit and receive configuration registers. */
 outw(RCR_CLEAR, ioaddr + RCR);
 outw(TCR_CLEAR, ioaddr + TCR);
@@ -1699,6 +1701,7 @@ static void smc_reset(struct net_device *dev)
 SMC_SELECT_BANK(2);
 outw((IM_EPH_INT | IM_RX_OVRN_INT | IM_RCV_INT) << 8,
 ioaddr + INTERRUPT);
+spin_unlock_irqrestore(>lock, flags);
 }
 
 /*==
-- 
2.7.4

[PATCH] smc91c92_cs : add a spinlock to avoid race condition

[Pavel Andrianov]

smc_reset may be executed in parallel with timer function media_check.
To avoid data race in smc_set_xcvr a spinlock was added.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Pavel Andrianov 
---
 drivers/net/ethernet/smsc/smc91c92_cs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/smsc/smc91c92_cs.c 
b/drivers/net/ethernet/smsc/smc91c92_cs.c
index db3c696..69d865c 100644
--- a/drivers/net/ethernet/smsc/smc91c92_cs.c
+++ b/drivers/net/ethernet/smsc/smc91c92_cs.c
@@ -1637,6 +1637,7 @@ static void smc_reset(struct net_device *dev)
 unsigned int ioaddr = dev->base_addr;
 struct smc_private *smc = netdev_priv(dev);
 int i;
+unsigned long flags;
 
 netdev_dbg(dev, "smc91c92 reset called.\n");
 
@@ -1647,6 +1648,7 @@ static void smc_reset(struct net_device *dev)
 outw(RCR_SOFTRESET, ioaddr + RCR);
 udelay(10);
 
+spin_lock_irqsave(>lock, flags);
 /* Clear the transmit and receive configuration registers. */
 outw(RCR_CLEAR, ioaddr + RCR);
 outw(TCR_CLEAR, ioaddr + TCR);
@@ -1699,6 +1701,7 @@ static void smc_reset(struct net_device *dev)
 SMC_SELECT_BANK(2);
 outw((IM_EPH_INT | IM_RX_OVRN_INT | IM_RCV_INT) << 8,
 ioaddr + INTERRUPT);
+spin_unlock_irqrestore(>lock, flags);
 }
 
 /*==
-- 
2.7.4

A potential data race in drivers/isdn/hardware/eicon/diva_mnt.ko

[Pavel Andrianov]

Hi!

There is a potential data race in drivers/isdn/hardware/eicon/diva_mnt.ko

In diva_maint_init there is an initialization of spinlocks (debug.c: 
lines 245, 252) and a queue (debug.c: line 237). These objects are used 
in handlers of divas_main_fops, for instance, divas_maint_read. These 
ops are registered in divas_maint_register_chrdev, which is called 
(divamnt.c: line 206) before initialization (divamnt.c: line 211). Thus, 
there may occur a situation when the handlers of divas_main_fops occur 
to uninitialized resources.


--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

A potential data race in drivers/isdn/hardware/eicon/diva_mnt.ko

[Pavel Andrianov]

Hi!

There is a potential data race in drivers/isdn/hardware/eicon/diva_mnt.ko

In diva_maint_init there is an initialization of spinlocks (debug.c: 
lines 245, 252) and a queue (debug.c: line 237). These objects are used 
in handlers of divas_main_fops, for instance, divas_maint_read. These 
ops are registered in divas_maint_register_chrdev, which is called 
(divamnt.c: line 206) before initialization (divamnt.c: line 211). Thus, 
there may occur a situation when the handlers of divas_main_fops occur 
to uninitialized resources.


--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

Potential data race in drivers/net/ethernet/sis/sis190.ko

[Pavel Andrianov]

Hi!

There is a potential data race in drivers/net/ethernet/sis/sis190.ko.

Regard such situation:

CPU 1   CPU 2

...
->sis190_open
- registers interrupts
...
->sis190_tx_timeout
- is called at some point
   ->sis190_tx_clear
  skb = tp->Tx_skbuff[i]
  [skb != null]

   an interrupt comes to CPU 2

-> sis190_irq
  -> sis190_tx_interrupt
skb = tp->Tx_skbuff[entry];
  ...
  -> dev_kfree_skb_irq(skb)
->dev_kfree_skb(skb)

In this case the skb is freed twice. Likely, in the interrupt handler 
the same spinlock should be acquired as in

sis190_tx_timeout.

--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

Potential data race in drivers/net/ethernet/sis/sis190.ko

[Pavel Andrianov]

Hi!

There is a potential data race in drivers/net/ethernet/sis/sis190.ko.

Regard such situation:

CPU 1   CPU 2

...
->sis190_open
- registers interrupts
...
->sis190_tx_timeout
- is called at some point
   ->sis190_tx_clear
  skb = tp->Tx_skbuff[i]
  [skb != null]

   an interrupt comes to CPU 2

-> sis190_irq
  -> sis190_tx_interrupt
skb = tp->Tx_skbuff[entry];
  ...
  -> dev_kfree_skb_irq(skb)
->dev_kfree_skb(skb)

In this case the skb is freed twice. Likely, in the interrupt handler 
the same spinlock should be acquired as in

sis190_tx_timeout.

--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

A potential data race in drivers/scsi/mvumi.ko

[Pavel Andrianov]

Hi!

There is a potential data race in drivers/scsi/mvumi.ko.
Regard such case:

Thread 1Thread 2
...
-> mvumi_reset_host_9500 -
   is called without any locking
  -> mvumi_wait_for_outstanding
->mvumi_start
  ->mvumi_check_handshake
-> mvumi_handshake_event
  -> mvumi_handshake
->mvumi_hs_build_page

   interrupt comes
  -> mvumi_isr_handler
-> mvumi_handshake
  -> mvumi_hs_build_page

In this case the same data mhba->handshake_page is modified from two 
threads. Likely the first thread should acquire the same spinlock 
mhba->shost->host_lock as the second thread.


--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

A potential data race in drivers/scsi/mvumi.ko

[Pavel Andrianov]

Hi!

There is a potential data race in drivers/scsi/mvumi.ko.
Regard such case:

Thread 1Thread 2
...
-> mvumi_reset_host_9500 -
   is called without any locking
  -> mvumi_wait_for_outstanding
->mvumi_start
  ->mvumi_check_handshake
-> mvumi_handshake_event
  -> mvumi_handshake
->mvumi_hs_build_page

   interrupt comes
  -> mvumi_isr_handler
-> mvumi_handshake
  -> mvumi_hs_build_page

In this case the same data mhba->handshake_page is modified from two 
threads. Likely the first thread should acquire the same spinlock 
mhba->shost->host_lock as the second thread.


--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

A potential race in drivers/scsi/megaraid.ko

[Pavel Andrianov]

Hi!

There is a potential data race in drivers/scsi/megaraid.ko
Regards such case:

Thread 1Thread 2
... ...
-> megaraid_probe_one
   -> request_irq- now an interrupt may arise
   -> mega_query_adapter
 -> issue_scb_block
interrupt comes 
-> megaraid_isr_iomapped
  -> mega_runpendq
-> __mega_runpendq
  -> issue_skb

In this case the structure 'mbox' is modified from two threads 
simultaneously. Or, more correct, the first modification is interrupted 
by the second one. Likely, the first thread should acquire the same 
spinlock adapter->lock as the second one.


--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

A potential race in drivers/scsi/megaraid.ko

[Pavel Andrianov]

Hi!

There is a potential data race in drivers/scsi/megaraid.ko
Regards such case:

Thread 1Thread 2
... ...
-> megaraid_probe_one
   -> request_irq- now an interrupt may arise
   -> mega_query_adapter
 -> issue_scb_block
interrupt comes 
-> megaraid_isr_iomapped
  -> mega_runpendq
-> __mega_runpendq
  -> issue_skb

In this case the structure 'mbox' is modified from two threads 
simultaneously. Or, more correct, the first modification is interrupted 
by the second one. Likely, the first thread should acquire the same 
spinlock adapter->lock as the second one.


--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

A potential data race in drivers/net/ethernet/smsc/smc91c92_cs.ko

[Pavel Andrianov]

Hi!

There is a potential data race in drivers/net/ethernet/smsc/smc91c92_cs.ko.
Regard two threads:

Thread 1Thread 2
...
... ->smc_open
->smc_tx_timeout   ->mod_timer
  -> smc_reset   ->media_check - timer function
-> smc_set_xcvr->smc_set_xcvr 

In this case the struct 'smc' is modified from two threads 
simultaneously. Likely, the first thread should acquire the same 
spinlock smc->lock as the second thread in media_check.


--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

A potential data race in drivers/net/ethernet/smsc/smc91c92_cs.ko

[Pavel Andrianov]

Hi!

There is a potential data race in drivers/net/ethernet/smsc/smc91c92_cs.ko.
Regard two threads:

Thread 1Thread 2
...
... ->smc_open
->smc_tx_timeout   ->mod_timer
  -> smc_reset   ->media_check - timer function
-> smc_set_xcvr->smc_set_xcvr 

In this case the struct 'smc' is modified from two threads 
simultaneously. Likely, the first thread should acquire the same 
spinlock smc->lock as the second thread in media_check.


--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

Re: Potential race condition in drivers/ata/sata_mv.ko

[Pavel Andrianov]

Hi!

I have found such example:

... ->

ata_exec_internal_sg ->

ata_qc_issue ->

mv_qc_issue ->

mv_clear_and_enable_port_irqs ->

mv_enable_port_irqs ->

mv_set_main_irq_mask


ata_exec_internal_sg acquires spin_lock(ap->lock) and call of the last 
function mv_set_main_irq_mask is with this lock. mv_interrupt acquires 
spin_lock(host->lock) before call of the same function. I am not sure is 
it correct to add one more spin_lock or move a call of request_irq in 
ata_host_activate, thus I can not easily fix the issue.


One more question is related to ata_exec_internal_sg. In comments there 
is an information the function is called without locking. However, 
ata_exec_internal_sg calls ata_eh_release before ata_eh_acquire (lines 
1650, 1655).There is a block of code under spinlock and eh context can 
not be acquired there. The comment may be wrong and eh_context is 
acquired somewhere before, but I also can not find it. Do you know where 
is the initial acquire of eh_context in this case?



10.08.2016 06:51, Tejun Heo пишет:

Hello,

On Fri, Aug 05, 2016 at 03:43:30PM +0300, Pavel Andrianov wrote:

In drivers/ata/sata_mv.ko function mv_set_main_irq_mask is called several
times. Twice with a spinlock, twice from init function and once without any
protection. The call without protection rises to several handlers from
ata_port_operations. The structure with the ata_port_operations is included
into a structure 'host' in mv_platform_probe and in mv_pci_init_one. At the
end of these functions ata_host operations are activated together with
interrupt handler. The conclusion is: interrupt handler may be executed in
parallel with handlers from ata_port_operations, or, more formally, it may
interrupt its execution.

In mv_set_main_irq_mask and in interrupt handler mv_interrupt the interrupt
mask is modified, but, as I said, handlers from ata_port_operations do not
acquire any lock. Thus, the interrupt mask may be set incorrectly if the are
two conflicting modifications.

It depends on which operations.  Most are only called from EH context
and racing there isn't likely to cause any actual issues.  Care to
submit a patch to fix the issue?

Thanks.



--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

Re: Potential race condition in drivers/ata/sata_mv.ko

[Pavel Andrianov]

Hi!

I have found such example:

... ->

ata_exec_internal_sg ->

ata_qc_issue ->

mv_qc_issue ->

mv_clear_and_enable_port_irqs ->

mv_enable_port_irqs ->

mv_set_main_irq_mask


ata_exec_internal_sg acquires spin_lock(ap->lock) and call of the last 
function mv_set_main_irq_mask is with this lock. mv_interrupt acquires 
spin_lock(host->lock) before call of the same function. I am not sure is 
it correct to add one more spin_lock or move a call of request_irq in 
ata_host_activate, thus I can not easily fix the issue.


One more question is related to ata_exec_internal_sg. In comments there 
is an information the function is called without locking. However, 
ata_exec_internal_sg calls ata_eh_release before ata_eh_acquire (lines 
1650, 1655).There is a block of code under spinlock and eh context can 
not be acquired there. The comment may be wrong and eh_context is 
acquired somewhere before, but I also can not find it. Do you know where 
is the initial acquire of eh_context in this case?



10.08.2016 06:51, Tejun Heo пишет:

Hello,

On Fri, Aug 05, 2016 at 03:43:30PM +0300, Pavel Andrianov wrote:

In drivers/ata/sata_mv.ko function mv_set_main_irq_mask is called several
times. Twice with a spinlock, twice from init function and once without any
protection. The call without protection rises to several handlers from
ata_port_operations. The structure with the ata_port_operations is included
into a structure 'host' in mv_platform_probe and in mv_pci_init_one. At the
end of these functions ata_host operations are activated together with
interrupt handler. The conclusion is: interrupt handler may be executed in
parallel with handlers from ata_port_operations, or, more formally, it may
interrupt its execution.

In mv_set_main_irq_mask and in interrupt handler mv_interrupt the interrupt
mask is modified, but, as I said, handlers from ata_port_operations do not
acquire any lock. Thus, the interrupt mask may be set incorrectly if the are
two conflicting modifications.

It depends on which operations.  Most are only called from EH context
and racing there isn't likely to cause any actual issues.  Care to
submit a patch to fix the issue?

Thanks.



--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

A potential race in drivers/atm/eni.ko

[Pavel Andrianov]

Hi!

There is a potential race in drivers/atm/eni.ko.

In the interrupt handler eni_int the field eni_dev->events is masked 
(line 1519) with a spinlock protection. In eni_start eni_dev->events is 
initialized (line 1844), but it is done after interrupts are requested 
(line 1813). Thus, the interrupt may occur while initialization is not 
finishing and the new value of events will be lost. Moreover, the 
spinlock, which is used in the interrupt handler, is also initialized 
(line 1842) after request_irq (line 1813).


--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

A potential race in drivers/atm/eni.ko

[Pavel Andrianov]

Hi!

There is a potential race in drivers/atm/eni.ko.

In the interrupt handler eni_int the field eni_dev->events is masked 
(line 1519) with a spinlock protection. In eni_start eni_dev->events is 
initialized (line 1844), but it is done after interrupts are requested 
(line 1813). Thus, the interrupt may occur while initialization is not 
finishing and the new value of events will be lost. Moreover, the 
spinlock, which is used in the interrupt handler, is also initialized 
(line 1842) after request_irq (line 1813).


--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

Potential race condition in drivers/ata/sata_mv.ko

[Pavel Andrianov]

Hi!

In drivers/ata/sata_mv.ko function mv_set_main_irq_mask is called 
several times. Twice with a spinlock, twice from init function and once 
without any protection. The call without protection rises to several 
handlers from ata_port_operations. The structure with the 
ata_port_operations is included into a structure 'host' in 
mv_platform_probe and in mv_pci_init_one. At the end of these functions 
ata_host operations are activated together with interrupt handler. The 
conclusion is: interrupt handler may be executed in parallel with 
handlers from ata_port_operations, or, more formally, it may interrupt 
its execution.


In mv_set_main_irq_mask and in interrupt handler mv_interrupt the 
interrupt mask is modified, but, as I said, handlers from 
ata_port_operations do not acquire any lock. Thus, the interrupt mask 
may be set incorrectly if the are two conflicting modifications.



--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

Potential race condition in drivers/ata/sata_mv.ko

[Pavel Andrianov]

Hi!

In drivers/ata/sata_mv.ko function mv_set_main_irq_mask is called 
several times. Twice with a spinlock, twice from init function and once 
without any protection. The call without protection rises to several 
handlers from ata_port_operations. The structure with the 
ata_port_operations is included into a structure 'host' in 
mv_platform_probe and in mv_pci_init_one. At the end of these functions 
ata_host operations are activated together with interrupt handler. The 
conclusion is: interrupt handler may be executed in parallel with 
handlers from ata_port_operations, or, more formally, it may interrupt 
its execution.


In mv_set_main_irq_mask and in interrupt handler mv_interrupt the 
interrupt mask is modified, but, as I said, handlers from 
ata_port_operations do not acquire any lock. Thus, the interrupt mask 
may be set incorrectly if the are two conflicting modifications.



--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

[PATCH] wl3501_cs: Add spinlock to wl3501_reset

[Pavel Andrianov]

Likely wl3501_reset should acquire spinlock as wl3501_{open, close}.
One of calls of wl3501_reset has been already protected.
The others were unprotected and might lead to a race condition.
The patch adds spinlock into the wl3501_reset and removes it from 
wl3501_tx_timeout.

Found by Linux Driver Verification project (linuxtesting.org)

Signed-off-by: Pavel Andrianov <andria...@ispras.ru>
---
 drivers/net/wireless/wl3501_cs.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/wl3501_cs.c b/drivers/net/wireless/wl3501_cs.c
index 13fd734..196f13c 100644
--- a/drivers/net/wireless/wl3501_cs.c
+++ b/drivers/net/wireless/wl3501_cs.c
@@ -1247,7 +1247,9 @@ static int wl3501_reset(struct net_device *dev)
 {
struct wl3501_card *this = netdev_priv(dev);
int rc = -ENODEV;
+   unsigned long flags;
 
+   spin_lock_irqsave(>lock, flags);
wl3501_block_interrupt(this);
 
if (wl3501_init_firmware(this)) {
@@ -1269,20 +1271,17 @@ static int wl3501_reset(struct net_device *dev)
pr_debug("%s: device reset", dev->name);
rc = 0;
 out:
+   spin_unlock_irqrestore(>lock, flags);
return rc;
 }
 
 static void wl3501_tx_timeout(struct net_device *dev)
 {
-   struct wl3501_card *this = netdev_priv(dev);
struct net_device_stats *stats = >stats;
-   unsigned long flags;
int rc;
 
stats->tx_errors++;
-   spin_lock_irqsave(>lock, flags);
rc = wl3501_reset(dev);
-   spin_unlock_irqrestore(>lock, flags);
if (rc)
printk(KERN_ERR "%s: Error %d resetting card on Tx timeout!\n",
   dev->name, rc);
-- 
2.7.4

[PATCH] wl3501_cs: Add spinlock to wl3501_reset

[Pavel Andrianov]

Likely wl3501_reset should acquire spinlock as wl3501_{open, close}.
One of calls of wl3501_reset has been already protected.
The others were unprotected and might lead to a race condition.
The patch adds spinlock into the wl3501_reset and removes it from 
wl3501_tx_timeout.

Found by Linux Driver Verification project (linuxtesting.org)

Signed-off-by: Pavel Andrianov 
---
 drivers/net/wireless/wl3501_cs.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/wl3501_cs.c b/drivers/net/wireless/wl3501_cs.c
index 13fd734..196f13c 100644
--- a/drivers/net/wireless/wl3501_cs.c
+++ b/drivers/net/wireless/wl3501_cs.c
@@ -1247,7 +1247,9 @@ static int wl3501_reset(struct net_device *dev)
 {
struct wl3501_card *this = netdev_priv(dev);
int rc = -ENODEV;
+   unsigned long flags;
 
+   spin_lock_irqsave(>lock, flags);
wl3501_block_interrupt(this);
 
if (wl3501_init_firmware(this)) {
@@ -1269,20 +1271,17 @@ static int wl3501_reset(struct net_device *dev)
pr_debug("%s: device reset", dev->name);
rc = 0;
 out:
+   spin_unlock_irqrestore(>lock, flags);
return rc;
 }
 
 static void wl3501_tx_timeout(struct net_device *dev)
 {
-   struct wl3501_card *this = netdev_priv(dev);
struct net_device_stats *stats = >stats;
-   unsigned long flags;
int rc;
 
stats->tx_errors++;
-   spin_lock_irqsave(>lock, flags);
rc = wl3501_reset(dev);
-   spin_unlock_irqrestore(>lock, flags);
if (rc)
printk(KERN_ERR "%s: Error %d resetting card on Tx timeout!\n",
   dev->name, rc);
-- 
2.7.4

Re: A potential race

[Pavel Andrianov]

Hi!

We have no hardware to test possible fixes. If somebody has it and 
agrees to check our patches, we will prepare them.


Best regards,
Pavel

01.07.2016 20:17, Hans Verkuil пишет:

On 07/01/2016 05:02 PM, Pavel Andrianov wrote:

01.07.2016 19:53, Hans Verkuil пишет:

On 07/01/2016 04:39 PM, Pavel Andrianov wrote:

   Hi!

There is a potential race condition between usbvision_v4l2_close and 
usbvision_disconnect. The possible scenario may be the following. 
usbvision_disconnect starts execution, assigns usbvision->remove_pending = 1, 
and is interrupted
(rescheduled) after mutex_unlock. After that usbvision_v4l2_close is executed, 
decrease usbvision->user-- , checks usbvision->remove_pending, executes 
usbvision_release and finishes. Then usbvision_disconnect continues its execution. It 
checks
usbversion->user (it is already 0) and also execute usbvision_release. Thus, 
release is executed twice. The same situation may
occur if usbvision_v4l2_close is interrupted by usbvision_disconnect. Moreover, 
the same problem is in usbvision_radio_close. In all these cases the check 
before call usbvision_release under mutex_lock protection does not solve the 
problem, because  there may occur an open() after the check and the race takes 
place again. The question is: why the usbvision_release
is called from close() (usbvision_v4l2_close and usbvision_radio_close)? 
Usually release functions are called from
disconnect.

Please don't use html mail, mailinglists will silently reject this.

The usbvision driver is old and unloved and known to be very bad code. It needs 
a huge amount of work to make all this work correctly.

I don't see anyone picking this up...

Regards,

Hans

If you know the driver, could you, please, explain me, why
usbvision_release is called from close functions (usbvision_v4l2_close
and usbvision_radio_close) and not only from disconnect? Thanks!


Because the author didn't know what he was doing. Although, to be fair, we have 
much better
solutions for this. But who is willing to put in the time to fix this properly?

The basic idea was: if someone still has a video/radio node open when 
disconnect happens, then
we leave it to the last close to call release, otherwise we can call release 
right away.

It needs to be rewritten.

If you're volunteering to clean this up, then I can give pointers.

Regards,

    Hans



--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

Re: A potential race

[Pavel Andrianov]

Hi!

We have no hardware to test possible fixes. If somebody has it and 
agrees to check our patches, we will prepare them.


Best regards,
Pavel

01.07.2016 20:17, Hans Verkuil пишет:

On 07/01/2016 05:02 PM, Pavel Andrianov wrote:

01.07.2016 19:53, Hans Verkuil пишет:

On 07/01/2016 04:39 PM, Pavel Andrianov wrote:

   Hi!

There is a potential race condition between usbvision_v4l2_close and 
usbvision_disconnect. The possible scenario may be the following. 
usbvision_disconnect starts execution, assigns usbvision->remove_pending = 1, 
and is interrupted
(rescheduled) after mutex_unlock. After that usbvision_v4l2_close is executed, 
decrease usbvision->user-- , checks usbvision->remove_pending, executes 
usbvision_release and finishes. Then usbvision_disconnect continues its execution. It 
checks
usbversion->user (it is already 0) and also execute usbvision_release. Thus, 
release is executed twice. The same situation may
occur if usbvision_v4l2_close is interrupted by usbvision_disconnect. Moreover, 
the same problem is in usbvision_radio_close. In all these cases the check 
before call usbvision_release under mutex_lock protection does not solve the 
problem, because  there may occur an open() after the check and the race takes 
place again. The question is: why the usbvision_release
is called from close() (usbvision_v4l2_close and usbvision_radio_close)? 
Usually release functions are called from
disconnect.

Please don't use html mail, mailinglists will silently reject this.

The usbvision driver is old and unloved and known to be very bad code. It needs 
a huge amount of work to make all this work correctly.

I don't see anyone picking this up...

Regards,

Hans

If you know the driver, could you, please, explain me, why
usbvision_release is called from close functions (usbvision_v4l2_close
and usbvision_radio_close) and not only from disconnect? Thanks!


Because the author didn't know what he was doing. Although, to be fair, we have 
much better
solutions for this. But who is willing to put in the time to fix this properly?

The basic idea was: if someone still has a video/radio node open when 
disconnect happens, then
we leave it to the last close to call release, otherwise we can call release 
right away.

It needs to be rewritten.

If you're volunteering to clean this up, then I can give pointers.

Regards,

    Hans



--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

Re: A potential race

[Pavel Andrianov]

01.07.2016 19:53, Hans Verkuil пишет:

On 07/01/2016 04:39 PM, Pavel Andrianov wrote:

  Hi!

There is a potential race condition between usbvision_v4l2_close and 
usbvision_disconnect. The possible scenario may be the following. 
usbvision_disconnect starts execution, assigns usbvision->remove_pending = 1, 
and is interrupted
(rescheduled) after mutex_unlock. After that usbvision_v4l2_close is executed, 
decrease usbvision->user-- , checks usbvision->remove_pending, executes 
usbvision_release and finishes. Then usbvision_disconnect continues its execution. It 
checks
usbversion->user (it is already 0) and also execute usbvision_release. Thus, 
release is executed twice. The same situation may
occur if usbvision_v4l2_close is interrupted by usbvision_disconnect. Moreover, 
the same problem is in usbvision_radio_close. In all these cases the check 
before call usbvision_release under mutex_lock protection does not solve the 
problem, because  there may occur an open() after the check and the race takes 
place again. The question is: why the usbvision_release
is called from close() (usbvision_v4l2_close and usbvision_radio_close)? 
Usually release functions are called from
disconnect.

Please don't use html mail, mailinglists will silently reject this.

The usbvision driver is old and unloved and known to be very bad code. It needs 
a huge amount of work to make all this work correctly.

I don't see anyone picking this up...

Regards,

Hans
If you know the driver, could you, please, explain me, why 
usbvision_release is called from close functions (usbvision_v4l2_close 
and usbvision_radio_close) and not only from disconnect? Thanks!


--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

Re: A potential race

[Pavel Andrianov]

01.07.2016 19:53, Hans Verkuil пишет:

On 07/01/2016 04:39 PM, Pavel Andrianov wrote:

  Hi!

There is a potential race condition between usbvision_v4l2_close and 
usbvision_disconnect. The possible scenario may be the following. 
usbvision_disconnect starts execution, assigns usbvision->remove_pending = 1, 
and is interrupted
(rescheduled) after mutex_unlock. After that usbvision_v4l2_close is executed, 
decrease usbvision->user-- , checks usbvision->remove_pending, executes 
usbvision_release and finishes. Then usbvision_disconnect continues its execution. It 
checks
usbversion->user (it is already 0) and also execute usbvision_release. Thus, 
release is executed twice. The same situation may
occur if usbvision_v4l2_close is interrupted by usbvision_disconnect. Moreover, 
the same problem is in usbvision_radio_close. In all these cases the check 
before call usbvision_release under mutex_lock protection does not solve the 
problem, because  there may occur an open() after the check and the race takes 
place again. The question is: why the usbvision_release
is called from close() (usbvision_v4l2_close and usbvision_radio_close)? 
Usually release functions are called from
disconnect.

Please don't use html mail, mailinglists will silently reject this.

The usbvision driver is old and unloved and known to be very bad code. It needs 
a huge amount of work to make all this work correctly.

I don't see anyone picking this up...

Regards,

Hans
If you know the driver, could you, please, explain me, why 
usbvision_release is called from close functions (usbvision_v4l2_close 
and usbvision_radio_close) and not only from disconnect? Thanks!


--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

[PATCH] libertas: Add spinlock to avoid race condition

[Pavel Andrianov]

lbs_mac_event_disconnected may free priv->currenttxskb
while lbs_hard_start_xmit accesses to it.
The patch adds a spinlock for mutual exclusion.

Tested on OLPC XO-1 (usb8388) and XO-1.5 (sd8686) with v4.7-rc3.

Confirmed that lbs_mac_event_disconnected is being called on the
station when hostapd on access point is given SIGHUP.

Signed-off-by: Pavel 
Tested-by: James Cameron 
---
 drivers/net/wireless/marvell/libertas/cmdresp.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/wireless/marvell/libertas/cmdresp.c 
b/drivers/net/wireless/marvell/libertas/cmdresp.c
index c95bf6d..c67ae07 100644
--- a/drivers/net/wireless/marvell/libertas/cmdresp.c
+++ b/drivers/net/wireless/marvell/libertas/cmdresp.c
@@ -27,6 +27,8 @@
 void lbs_mac_event_disconnected(struct lbs_private *priv,
bool locally_generated)
 {
+   unsigned long flags;
+
if (priv->connect_status != LBS_CONNECTED)
return;
 
@@ -46,9 +48,11 @@ void lbs_mac_event_disconnected(struct lbs_private *priv,
netif_carrier_off(priv->dev);
 
/* Free Tx and Rx packets */
+   spin_lock_irqsave(>driver_lock, flags);
kfree_skb(priv->currenttxskb);
priv->currenttxskb = NULL;
priv->tx_pending_len = 0;
+   spin_unlock_irqrestore(>driver_lock, flags);
 
priv->connect_status = LBS_DISCONNECTED;
 
-- 
1.7.11.7

[PATCH] libertas: Add spinlock to avoid race condition

[Pavel Andrianov]

lbs_mac_event_disconnected may free priv->currenttxskb
while lbs_hard_start_xmit accesses to it.
The patch adds a spinlock for mutual exclusion.

Tested on OLPC XO-1 (usb8388) and XO-1.5 (sd8686) with v4.7-rc3.

Confirmed that lbs_mac_event_disconnected is being called on the
station when hostapd on access point is given SIGHUP.

Signed-off-by: Pavel 
Tested-by: James Cameron 
---
 drivers/net/wireless/marvell/libertas/cmdresp.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/wireless/marvell/libertas/cmdresp.c 
b/drivers/net/wireless/marvell/libertas/cmdresp.c
index c95bf6d..c67ae07 100644
--- a/drivers/net/wireless/marvell/libertas/cmdresp.c
+++ b/drivers/net/wireless/marvell/libertas/cmdresp.c
@@ -27,6 +27,8 @@
 void lbs_mac_event_disconnected(struct lbs_private *priv,
bool locally_generated)
 {
+   unsigned long flags;
+
if (priv->connect_status != LBS_CONNECTED)
return;
 
@@ -46,9 +48,11 @@ void lbs_mac_event_disconnected(struct lbs_private *priv,
netif_carrier_off(priv->dev);
 
/* Free Tx and Rx packets */
+   spin_lock_irqsave(>driver_lock, flags);
kfree_skb(priv->currenttxskb);
priv->currenttxskb = NULL;
priv->tx_pending_len = 0;
+   spin_unlock_irqrestore(>driver_lock, flags);
 
priv->connect_status = LBS_DISCONNECTED;
 
-- 
1.7.11.7

[ldv-project] [net] wcn36xx: potential race condition

[Pavel Andrianov]

Hi!

There is a potential race condition in 
drivers/net/wireless/ath/wcn36xx/wcn36xx.ko. In wcn36xx_tx -> 
wcn36xx_start_tx -> wcn36xx_set_tx_data 
(http://lxr.free-electrons.com/source/drivers/net/wireless/ath/wcn36xx/txrx.c#L176) 
there is a read of sta_priv->bss_dpu_desc_index and 
sta_priv->bss_sta_index. In wcn36xx_bss_info_changed -> 
wcn36xx_smd_config_bss -> wcn36xx_smd_config_bss_rsp 
(http://lxr.free-electrons.com/source/drivers/net/wireless/ath/wcn36xx/smd.c#L1204) 
there is a write to the same fields. It  seems that the handlers may be 
called in parallel and inconsistent data may be obtained.
The same problem is with sta_priv->sta_index and 
sta_priv->sta_dpu_desc_index:
http://lxr.free-electrons.com/source/drivers/net/wireless/ath/wcn36xx/txrx.c#L181 

http://lxr.free-electrons.com/source/drivers/net/wireless/ath/wcn36xx/smd.c#L986 


Is it a real bug? Is it enough to add mutex_lock to wcn36xx_set_tx_data?

--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

[ldv-project] [net] wcn36xx: potential race condition

[Pavel Andrianov]

Hi!

There is a potential race condition in 
drivers/net/wireless/ath/wcn36xx/wcn36xx.ko. In wcn36xx_tx -> 
wcn36xx_start_tx -> wcn36xx_set_tx_data 
(http://lxr.free-electrons.com/source/drivers/net/wireless/ath/wcn36xx/txrx.c#L176) 
there is a read of sta_priv->bss_dpu_desc_index and 
sta_priv->bss_sta_index. In wcn36xx_bss_info_changed -> 
wcn36xx_smd_config_bss -> wcn36xx_smd_config_bss_rsp 
(http://lxr.free-electrons.com/source/drivers/net/wireless/ath/wcn36xx/smd.c#L1204) 
there is a write to the same fields. It  seems that the handlers may be 
called in parallel and inconsistent data may be obtained.
The same problem is with sta_priv->sta_index and 
sta_priv->sta_dpu_desc_index:
http://lxr.free-electrons.com/source/drivers/net/wireless/ath/wcn36xx/txrx.c#L181 

http://lxr.free-electrons.com/source/drivers/net/wireless/ath/wcn36xx/smd.c#L986 


Is it a real bug? Is it enough to add mutex_lock to wcn36xx_set_tx_data?

--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

Re: [ldv-project] [net] libertas: potential race condition

[Pavel Andrianov]

08.06.2016 02:51, James Cameron пишет:

On Tue, Jun 07, 2016 at 09:39:55AM -0500, Dan Williams wrote:

On Tue, 2016-06-07 at 13:30 +0400, Pavel Andrianov wrote:

Hi!

There is a potential race condition in
drivers/net/wireless/libertas/libertas.ko.
In the function lbs_hard_start_xmit(..), line 159, a socket buffer
is
written to priv->current_skb with a spin_lock protection.
In the function lbs_mac_event_disconnected(..), lines 50-51, the
field
current_skb is cleaned. There is no protection used. The
corresponding
handlers are activated at the same time in lbs_start_card(..) and
then
may be executed simultaneously. Note, there are two structures
lbs_netdev_ops and mesh_netdev_ops, which have the target handler
lbs_hard_start_xmit.
Is it a real race or I have missed something?

Yeah, it looks like it should be grabbing priv->driver_lock before
clearing priv->currenttxskb in lbs_mac_event_disconnected().  Care to
submit a patch after testing?  Do you have any of that hardware?

I've hardware, with serial console.

Can test any patch, on USB (8388) or SDIO (8686).


Hi!

I've prepare the patch for this issue. Could you test it?

Thank you.

--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

>From b1a9e157ccdf8650da93844fd2dbdb6fca509b59 Mon Sep 17 00:00:00 2001
From: Pavel <andria...@ispras.ru>
Date: Tue, 14 Jun 2016 16:59:00 +0400
Subject: [PATCH] libertas: Add spinlock to avoid race condition

lbs_mac_event_disconnected may free priv->currenttxskb
while lbs_hard_start_xmit accesses to it.
The patch adds a spinlock for mutual exclusion.

Signed-off-by: Pavel <andria...@ispras.ru>
---
 drivers/net/wireless/marvell/libertas/cmdresp.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/wireless/marvell/libertas/cmdresp.c b/drivers/net/wireless/marvell/libertas/cmdresp.c
index c95bf6d..c67ae07 100644
--- a/drivers/net/wireless/marvell/libertas/cmdresp.c
+++ b/drivers/net/wireless/marvell/libertas/cmdresp.c
@@ -27,6 +27,8 @@
 void lbs_mac_event_disconnected(struct lbs_private *priv,
 bool locally_generated)
 {
+	unsigned long flags;
+
 	if (priv->connect_status != LBS_CONNECTED)
 		return;
 
@@ -46,9 +48,11 @@ void lbs_mac_event_disconnected(struct lbs_private *priv,
 	netif_carrier_off(priv->dev);
 
 	/* Free Tx and Rx packets */
+	spin_lock_irqsave(>driver_lock, flags);
 	kfree_skb(priv->currenttxskb);
 	priv->currenttxskb = NULL;
 	priv->tx_pending_len = 0;
+	spin_unlock_irqrestore(>driver_lock, flags);
 
 	priv->connect_status = LBS_DISCONNECTED;
 
-- 
1.7.11.7

Re: [ldv-project] [net] libertas: potential race condition

[Pavel Andrianov]

08.06.2016 02:51, James Cameron пишет:

On Tue, Jun 07, 2016 at 09:39:55AM -0500, Dan Williams wrote:

On Tue, 2016-06-07 at 13:30 +0400, Pavel Andrianov wrote:

Hi!

There is a potential race condition in
drivers/net/wireless/libertas/libertas.ko.
In the function lbs_hard_start_xmit(..), line 159, a socket buffer
is
written to priv->current_skb with a spin_lock protection.
In the function lbs_mac_event_disconnected(..), lines 50-51, the
field
current_skb is cleaned. There is no protection used. The
corresponding
handlers are activated at the same time in lbs_start_card(..) and
then
may be executed simultaneously. Note, there are two structures
lbs_netdev_ops and mesh_netdev_ops, which have the target handler
lbs_hard_start_xmit.
Is it a real race or I have missed something?

Yeah, it looks like it should be grabbing priv->driver_lock before
clearing priv->currenttxskb in lbs_mac_event_disconnected().  Care to
submit a patch after testing?  Do you have any of that hardware?

I've hardware, with serial console.

Can test any patch, on USB (8388) or SDIO (8686).


Hi!

I've prepare the patch for this issue. Could you test it?

Thank you.

--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

>From b1a9e157ccdf8650da93844fd2dbdb6fca509b59 Mon Sep 17 00:00:00 2001
From: Pavel 
Date: Tue, 14 Jun 2016 16:59:00 +0400
Subject: [PATCH] libertas: Add spinlock to avoid race condition

lbs_mac_event_disconnected may free priv->currenttxskb
while lbs_hard_start_xmit accesses to it.
The patch adds a spinlock for mutual exclusion.

Signed-off-by: Pavel 
---
 drivers/net/wireless/marvell/libertas/cmdresp.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/wireless/marvell/libertas/cmdresp.c b/drivers/net/wireless/marvell/libertas/cmdresp.c
index c95bf6d..c67ae07 100644
--- a/drivers/net/wireless/marvell/libertas/cmdresp.c
+++ b/drivers/net/wireless/marvell/libertas/cmdresp.c
@@ -27,6 +27,8 @@
 void lbs_mac_event_disconnected(struct lbs_private *priv,
 bool locally_generated)
 {
+	unsigned long flags;
+
 	if (priv->connect_status != LBS_CONNECTED)
 		return;
 
@@ -46,9 +48,11 @@ void lbs_mac_event_disconnected(struct lbs_private *priv,
 	netif_carrier_off(priv->dev);
 
 	/* Free Tx and Rx packets */
+	spin_lock_irqsave(>driver_lock, flags);
 	kfree_skb(priv->currenttxskb);
 	priv->currenttxskb = NULL;
 	priv->tx_pending_len = 0;
+	spin_unlock_irqrestore(>driver_lock, flags);
 
 	priv->connect_status = LBS_DISCONNECTED;
 
-- 
1.7.11.7

[ldv-project] [net] rtl8188ee: a potential race condition

[Pavel Andrianov]

Hi!

There is a potential data race in 
drivers/net/wireless/realtek/rtlwifi/rtl8188ee/rtl8188ee.ko.


In the function rtl88ee_gpio_radio_on_off_checking the flag 
ppsc->rfchange_inprogress is set with a spinlock protection. In the 
function rtl_ps_set_rf_state the flag is read also under a spinlock. But 
the function rtl88e_dm_watchdog read it without any locks. As a result 
rtl88e_dm_watchdog may execute the succeeding code while changing (with 
the flag rfchange_inprogress == true). I do not exactly determine the 
consequences, but likely they are not good if there exists such check. 
Could anybody more confident confirm this?


The function rtl_ps_set_rf_state is always called with its parameter 
[protect_or_not == false]. Is this flag really necessary, if the value 
'true' is never used? The function is also set the flag 
ppsc->rfchange_inprogress and may affect the rtl88e_dm_watchdog as in 
the previous case.


--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

[ldv-project] [net] rtl8188ee: a potential race condition

[Pavel Andrianov]

Hi!

There is a potential data race in 
drivers/net/wireless/realtek/rtlwifi/rtl8188ee/rtl8188ee.ko.


In the function rtl88ee_gpio_radio_on_off_checking the flag 
ppsc->rfchange_inprogress is set with a spinlock protection. In the 
function rtl_ps_set_rf_state the flag is read also under a spinlock. But 
the function rtl88e_dm_watchdog read it without any locks. As a result 
rtl88e_dm_watchdog may execute the succeeding code while changing (with 
the flag rfchange_inprogress == true). I do not exactly determine the 
consequences, but likely they are not good if there exists such check. 
Could anybody more confident confirm this?


The function rtl_ps_set_rf_state is always called with its parameter 
[protect_or_not == false]. Is this flag really necessary, if the value 
'true' is never used? The function is also set the flag 
ppsc->rfchange_inprogress and may affect the rtl88e_dm_watchdog as in 
the previous case.


--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

Re: [ldv-project] [net] libertas: potential race condition

[Pavel Andrianov]

07.06.2016 18:39, Dan Williams пишет:

On Tue, 2016-06-07 at 13:30 +0400, Pavel Andrianov wrote:

Hi!

There is a potential race condition in
drivers/net/wireless/libertas/libertas.ko.
In the function lbs_hard_start_xmit(..), line 159, a socket buffer
is
written to priv->current_skb with a spin_lock protection.
In the function lbs_mac_event_disconnected(..), lines 50-51, the
field
current_skb is cleaned. There is no protection used. The
corresponding
handlers are activated at the same time in lbs_start_card(..) and
then
may be executed simultaneously. Note, there are two structures
lbs_netdev_ops and mesh_netdev_ops, which have the target handler
lbs_hard_start_xmit.
Is it a real race or I have missed something?

Yeah, it looks like it should be grabbing priv->driver_lock before
clearing priv->currenttxskb in lbs_mac_event_disconnected().  Care to
submit a patch after testing?  Do you have any of that hardware?

Dan
I have no that hardware and I have some doubts about the simple fix, 
you've suggested. For instance, in lbs_hard_start_xmit the lock is 
acquired twice and the priv->tx_pending_len can be modified also by 
lbs_mac_event_disconnected (even if spin_lock will be added to 
lbs_mac_event_disconnected). Moreover, the function lbs_send_tx_feedback 
also cleaned priv->currenttxskb, but it happens also without any 
protection. Thus, the fix has to be more complicated, and I have no 
ideas about it.


--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

Re: [ldv-project] [net] libertas: potential race condition

[Pavel Andrianov]

07.06.2016 18:39, Dan Williams пишет:

On Tue, 2016-06-07 at 13:30 +0400, Pavel Andrianov wrote:

Hi!

There is a potential race condition in
drivers/net/wireless/libertas/libertas.ko.
In the function lbs_hard_start_xmit(..), line 159, a socket buffer
is
written to priv->current_skb with a spin_lock protection.
In the function lbs_mac_event_disconnected(..), lines 50-51, the
field
current_skb is cleaned. There is no protection used. The
corresponding
handlers are activated at the same time in lbs_start_card(..) and
then
may be executed simultaneously. Note, there are two structures
lbs_netdev_ops and mesh_netdev_ops, which have the target handler
lbs_hard_start_xmit.
Is it a real race or I have missed something?

Yeah, it looks like it should be grabbing priv->driver_lock before
clearing priv->currenttxskb in lbs_mac_event_disconnected().  Care to
submit a patch after testing?  Do you have any of that hardware?

Dan
I have no that hardware and I have some doubts about the simple fix, 
you've suggested. For instance, in lbs_hard_start_xmit the lock is 
acquired twice and the priv->tx_pending_len can be modified also by 
lbs_mac_event_disconnected (even if spin_lock will be added to 
lbs_mac_event_disconnected). Moreover, the function lbs_send_tx_feedback 
also cleaned priv->currenttxskb, but it happens also without any 
protection. Thus, the fix has to be more complicated, and I have no 
ideas about it.


--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

[ldv-project] [net] libertas: potential race condition

[Pavel Andrianov]

Hi!

There is a potential race condition in 
drivers/net/wireless/libertas/libertas.ko.
In the function lbs_hard_start_xmit(..), line 159, a socket buffer is 
written to priv->current_skb with a spin_lock protection.
In the function lbs_mac_event_disconnected(..), lines 50-51, the field 
current_skb is cleaned. There is no protection used. The corresponding 
handlers are activated at the same time in lbs_start_card(..) and then 
may be executed simultaneously. Note, there are two structures 
lbs_netdev_ops and mesh_netdev_ops, which have the target handler 
lbs_hard_start_xmit.

Is it a real race or I have missed something?

--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

[ldv-project] [net] libertas: potential race condition

[Pavel Andrianov]

Hi!

There is a potential race condition in 
drivers/net/wireless/libertas/libertas.ko.
In the function lbs_hard_start_xmit(..), line 159, a socket buffer is 
written to priv->current_skb with a spin_lock protection.
In the function lbs_mac_event_disconnected(..), lines 50-51, the field 
current_skb is cleaned. There is no protection used. The corresponding 
handlers are activated at the same time in lbs_start_card(..) and then 
may be executed simultaneously. Note, there are two structures 
lbs_netdev_ops and mesh_netdev_ops, which have the target handler 
lbs_hard_start_xmit.

Is it a real race or I have missed something?

--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andria...@ispras.ru

[PATCH] scsi_megaraid: addition spin_lock in megaraid_abort()

[Pavel Andrianov]

In function megaraid_abort() there are calls to megaraid_abort_and_reset()
and mega_rundoneq() which access shared data (like pending_list) without
synchronization.In function megaraid_reset() the same calls were done with
spin_lock held. So, the patch adds spin_lock_irq and spin_unlock_irq to
megaraid_abort().

Found by Linux Driver Verification project (linuxtesting.org)

Signed-off-by: Pavel Andrianov 
---
 drivers/scsi/megaraid.c |3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c
index 4d39a9f..7572d86 100644
--- a/drivers/scsi/megaraid.c
+++ b/drivers/scsi/megaraid.c
@@ -1898,6 +1898,8 @@ megaraid_abort(Scsi_Cmnd *cmd)
 
adapter = (adapter_t *)cmd->device->host->hostdata;
 
+   spin_lock_irq(>lock);
+
rval =  megaraid_abort_and_reset(adapter, cmd, SCB_ABORT);
 
/*
@@ -1905,6 +1907,7 @@ megaraid_abort(Scsi_Cmnd *cmd)
 * to be communicated over to the mid layer.
 */
mega_rundoneq(adapter);
+   spin_unlock_irq(>lock);
 
return rval;
 }
-- 
1.7.4.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] scsi_megaraid: addition spin_lock in megaraid_abort()

[Pavel Andrianov]

In function megaraid_abort() there are calls to megaraid_abort_and_reset()
and mega_rundoneq() which access shared data (like pending_list) without
synchronization.In function megaraid_reset() the same calls were done with
spin_lock held. So, the patch adds spin_lock_irq and spin_unlock_irq to
megaraid_abort().

Found by Linux Driver Verification project (linuxtesting.org)

Signed-off-by: Pavel Andrianov andria...@ispras.ru
---
 drivers/scsi/megaraid.c |3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c
index 4d39a9f..7572d86 100644
--- a/drivers/scsi/megaraid.c
+++ b/drivers/scsi/megaraid.c
@@ -1898,6 +1898,8 @@ megaraid_abort(Scsi_Cmnd *cmd)
 
adapter = (adapter_t *)cmd-device-host-hostdata;
 
+   spin_lock_irq(adapter-lock);
+
rval =  megaraid_abort_and_reset(adapter, cmd, SCB_ABORT);
 
/*
@@ -1905,6 +1907,7 @@ megaraid_abort(Scsi_Cmnd *cmd)
 * to be communicated over to the mid layer.
 */
mega_rundoneq(adapter);
+   spin_unlock_irq(adapter-lock);
 
return rval;
 }
-- 
1.7.4.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/