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

[Vaishali Thakkar]

On Saturday 03 September 2016 08:53 PM, Jonathan Cameron wrote:
> On 02/09/16 09:05, Pavel Andrianov wrote:
>>
> 
>> Hi!
> Hi Pavel,
>>
>> 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?
>>
> 
> As Alison observed, mlock is not a general purpose lock for use by
> drivers. It's there to prevent state changes between direct reads
> (polled) and buffered/triggered reads (pushed).
> 
> The write raw simply sets the sampling frequency. That's not a problem
> whilst buffered capture is running or otherwise.  Interesting question
> of whether changing mode causes any trouble as well.  It's possible 
> something is undefined in the hardware during a mode change...
> 
> Anyhow, that covers mlock.  Next question: Is there a race condition in
> general?
> 
> Yes there definitely is as we have read modify write cycles
> on VF610_REG_ADC_CFG in both paths.  So what is needed is a local lock
> to protect these accesses.  Whilst in theory mlock could be used
> it should not be as it has a clearly stated purpose and using it
> for other purposes makes for much fiddlier and harder to read code!

Makes sense. What would be the best solution in this case? Should we
just introduce local lock for this module and use it for both or there
is anything we need to take care of while we have mlock for one?

> (as an aside IIRC there is no locking in sysfs attributes to prevent
> a single attribute being read twice at the same time.)
> 
> Jonathan
> 

-- 
Vaishali

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

[Vaishali Thakkar]

On Saturday 03 September 2016 08:53 PM, Jonathan Cameron wrote:
> On 02/09/16 09:05, Pavel Andrianov wrote:
>>
> 
>> Hi!
> Hi Pavel,
>>
>> 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?
>>
> 
> As Alison observed, mlock is not a general purpose lock for use by
> drivers. It's there to prevent state changes between direct reads
> (polled) and buffered/triggered reads (pushed).
> 
> The write raw simply sets the sampling frequency. That's not a problem
> whilst buffered capture is running or otherwise.  Interesting question
> of whether changing mode causes any trouble as well.  It's possible 
> something is undefined in the hardware during a mode change...
> 
> Anyhow, that covers mlock.  Next question: Is there a race condition in
> general?
> 
> Yes there definitely is as we have read modify write cycles
> on VF610_REG_ADC_CFG in both paths.  So what is needed is a local lock
> to protect these accesses.  Whilst in theory mlock could be used
> it should not be as it has a clearly stated purpose and using it
> for other purposes makes for much fiddlier and harder to read code!

Makes sense. What would be the best solution in this case? Should we
just introduce local lock for this module and use it for both or there
is anything we need to take care of while we have mlock for one?

> (as an aside IIRC there is no locking in sysfs attributes to prevent
> a single attribute being read twice at the same time.)
> 
> Jonathan
> 

-- 
Vaishali

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

[Jonathan Cameron]

On 05/09/16 07:49, Vaishali Thakkar wrote:
> 
> 
> On Saturday 03 September 2016 08:53 PM, Jonathan Cameron wrote:
>> On 02/09/16 09:05, Pavel Andrianov wrote:
>>>
>>
>>> Hi!
>> Hi Pavel,
>>>
>>> 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?
>>>
>>
>> As Alison observed, mlock is not a general purpose lock for use by
>> drivers. It's there to prevent state changes between direct reads
>> (polled) and buffered/triggered reads (pushed).
>>
>> The write raw simply sets the sampling frequency. That's not a problem
>> whilst buffered capture is running or otherwise.  Interesting question
>> of whether changing mode causes any trouble as well.  It's possible 
>> something is undefined in the hardware during a mode change...
>>
>> Anyhow, that covers mlock.  Next question: Is there a race condition in
>> general?
>>
>> Yes there definitely is as we have read modify write cycles
>> on VF610_REG_ADC_CFG in both paths.  So what is needed is a local lock
>> to protect these accesses.  Whilst in theory mlock could be used
>> it should not be as it has a clearly stated purpose and using it
>> for other purposes makes for much fiddlier and harder to read code!
> 
> Makes sense. What would be the best solution in this case? Should we
> just introduce local lock for this module and use it for both or there
> is anything we need to take care of while we have mlock for one?
I'd leave the mlock as it is (or use the direct_claim wrappers as relevant).
It would require a deep dive into the data sheet and hardware
testing (for undocumented 'features') to be sure we could relax the
current locking.  Tightening it doesn't make sense unless we have
reason to believe the frequency change causes trouble.

A local lock as part of the structure retrieved from iio_priv would
make sense to protect against multiple read, modify, write cycles
occurring at the same time would cover the race conditions nicely.

Jonathan
> 
>> (as an aside IIRC there is no locking in sysfs attributes to prevent
>> a single attribute being read twice at the same time.)
>>
>> Jonathan
>>
> 

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

[Jonathan Cameron]

On 05/09/16 07:49, Vaishali Thakkar wrote:
> 
> 
> On Saturday 03 September 2016 08:53 PM, Jonathan Cameron wrote:
>> On 02/09/16 09:05, Pavel Andrianov wrote:
>>>
>>
>>> Hi!
>> Hi Pavel,
>>>
>>> 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?
>>>
>>
>> As Alison observed, mlock is not a general purpose lock for use by
>> drivers. It's there to prevent state changes between direct reads
>> (polled) and buffered/triggered reads (pushed).
>>
>> The write raw simply sets the sampling frequency. That's not a problem
>> whilst buffered capture is running or otherwise.  Interesting question
>> of whether changing mode causes any trouble as well.  It's possible 
>> something is undefined in the hardware during a mode change...
>>
>> Anyhow, that covers mlock.  Next question: Is there a race condition in
>> general?
>>
>> Yes there definitely is as we have read modify write cycles
>> on VF610_REG_ADC_CFG in both paths.  So what is needed is a local lock
>> to protect these accesses.  Whilst in theory mlock could be used
>> it should not be as it has a clearly stated purpose and using it
>> for other purposes makes for much fiddlier and harder to read code!
> 
> Makes sense. What would be the best solution in this case? Should we
> just introduce local lock for this module and use it for both or there
> is anything we need to take care of while we have mlock for one?
I'd leave the mlock as it is (or use the direct_claim wrappers as relevant).
It would require a deep dive into the data sheet and hardware
testing (for undocumented 'features') to be sure we could relax the
current locking.  Tightening it doesn't make sense unless we have
reason to believe the frequency change causes trouble.

A local lock as part of the structure retrieved from iio_priv would
make sense to protect against multiple read, modify, write cycles
occurring at the same time would cover the race conditions nicely.

Jonathan
> 
>> (as an aside IIRC there is no locking in sysfs attributes to prevent
>> a single attribute being read twice at the same time.)
>>
>> Jonathan
>>
> 

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

[Samuel Thibault]

Pavel Andrianov, on Mon 05 Sep 2016 13:33:33 +0300, wrote:
> synth_direct_store may be called via device_attributes interface.

Ah, right.

> In which function the lock should be added?

That'd be synth_direct_store then, around the while loop.

Samuel

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

[Samuel Thibault]

Pavel Andrianov, on Mon 05 Sep 2016 13:33:33 +0300, wrote:
> synth_direct_store may be called via device_attributes interface.

Ah, right.

> In which function the lock should be added?

That'd be synth_direct_store then, around the while loop.

Samuel

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

[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

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

[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

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

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

[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

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

[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

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

[Vaishali Thakkar]

On Saturday 03 September 2016 08:53 PM, Jonathan Cameron wrote:
> On 02/09/16 09:05, Pavel Andrianov wrote:
>>
> 
>> Hi!
> Hi Pavel,
>>
>> 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?
>>
> 
> As Alison observed, mlock is not a general purpose lock for use by
> drivers. It's there to prevent state changes between direct reads
> (polled) and buffered/triggered reads (pushed).
> 
> The write raw simply sets the sampling frequency. That's not a problem
> whilst buffered capture is running or otherwise.  Interesting question
> of whether changing mode causes any trouble as well.  It's possible 
> something is undefined in the hardware during a mode change...
> 
> Anyhow, that covers mlock.  Next question: Is there a race condition in
> general?
> 
> Yes there definitely is as we have read modify write cycles
> on VF610_REG_ADC_CFG in both paths.  So what is needed is a local lock
> to protect these accesses.  Whilst in theory mlock could be used
> it should not be as it has a clearly stated purpose and using it
> for other purposes makes for much fiddlier and harder to read code!

Makes sense. What would be the best solution in this case? Should we
just introduce local lock for this module and use it for both or there
is anything we need to take care of while we have mlock for one?

> (as an aside IIRC there is no locking in sysfs attributes to prevent
> a single attribute being read twice at the same time.)
> 
> Jonathan
> 

-- 
Vaishali

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

[Vaishali Thakkar]

On Saturday 03 September 2016 08:53 PM, Jonathan Cameron wrote:
> On 02/09/16 09:05, Pavel Andrianov wrote:
>>
> 
>> Hi!
> Hi Pavel,
>>
>> 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?
>>
> 
> As Alison observed, mlock is not a general purpose lock for use by
> drivers. It's there to prevent state changes between direct reads
> (polled) and buffered/triggered reads (pushed).
> 
> The write raw simply sets the sampling frequency. That's not a problem
> whilst buffered capture is running or otherwise.  Interesting question
> of whether changing mode causes any trouble as well.  It's possible 
> something is undefined in the hardware during a mode change...
> 
> Anyhow, that covers mlock.  Next question: Is there a race condition in
> general?
> 
> Yes there definitely is as we have read modify write cycles
> on VF610_REG_ADC_CFG in both paths.  So what is needed is a local lock
> to protect these accesses.  Whilst in theory mlock could be used
> it should not be as it has a clearly stated purpose and using it
> for other purposes makes for much fiddlier and harder to read code!

Makes sense. What would be the best solution in this case? Should we
just introduce local lock for this module and use it for both or there
is anything we need to take care of while we have mlock for one?

> (as an aside IIRC there is no locking in sysfs attributes to prevent
> a single attribute being read twice at the same time.)
> 
> Jonathan
> 

-- 
Vaishali

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

[Jonathan Cameron]

On 02/09/16 09:05, Pavel Andrianov wrote:
> 

> Hi!
Hi Pavel,
> 
> 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?
> 

As Alison observed, mlock is not a general purpose lock for use by
drivers. It's there to prevent state changes between direct reads
(polled) and buffered/triggered reads (pushed).

The write raw simply sets the sampling frequency. That's not a problem
whilst buffered capture is running or otherwise.  Interesting question
of whether changing mode causes any trouble as well.  It's possible 
something is undefined in the hardware during a mode change...

Anyhow, that covers mlock.  Next question: Is there a race condition in
general?

Yes there definitely is as we have read modify write cycles
on VF610_REG_ADC_CFG in both paths.  So what is needed is a local lock
to protect these accesses.  Whilst in theory mlock could be used
it should not be as it has a clearly stated purpose and using it
for other purposes makes for much fiddlier and harder to read code!

(as an aside IIRC there is no locking in sysfs attributes to prevent
a single attribute being read twice at the same time.)

Jonathan

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

[Jonathan Cameron]

On 02/09/16 09:05, Pavel Andrianov wrote:
> 

> Hi!
Hi Pavel,
> 
> 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?
> 

As Alison observed, mlock is not a general purpose lock for use by
drivers. It's there to prevent state changes between direct reads
(polled) and buffered/triggered reads (pushed).

The write raw simply sets the sampling frequency. That's not a problem
whilst buffered capture is running or otherwise.  Interesting question
of whether changing mode causes any trouble as well.  It's possible 
something is undefined in the hardware during a mode change...

Anyhow, that covers mlock.  Next question: Is there a race condition in
general?

Yes there definitely is as we have read modify write cycles
on VF610_REG_ADC_CFG in both paths.  So what is needed is a local lock
to protect these accesses.  Whilst in theory mlock could be used
it should not be as it has a clearly stated purpose and using it
for other purposes makes for much fiddlier and harder to read code!

(as an aside IIRC there is no locking in sysfs attributes to prevent
a single attribute being read twice at the same time.)

Jonathan

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

[Alison Schofield]

On Fri, Sep 02, 2016 at 11:05:09AM +0300, Pavel Andrianov wrote:
> 
> 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?
> 
Hi Pavel,
I'm not familiar with the conversion_mode interface, so I'll leave your
specific question for someone with that knowledge.

Just wanted to point out that if you're going to update the locking
in the driver, there are 2 things to consider:
1) Use iio_device_claim_direct_mode() helper functions instead of
checking iio_buffer_enabled and grabbing mlock.
2) Any other uses of indio_dev->mlock are best moved to a private data
lock.  We want to return that mlock to an INTERNAL (core) use only.

alisons





> -- 
> Pavel Andrianov
> Linux Verification Center, ISPRAS
> web: http://linuxtesting.org
> e-mail: andria...@ispras.ru
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Alison Schofield]

On Fri, Sep 02, 2016 at 11:05:09AM +0300, Pavel Andrianov wrote:
> 
> 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?
> 
Hi Pavel,
I'm not familiar with the conversion_mode interface, so I'll leave your
specific question for someone with that knowledge.

Just wanted to point out that if you're going to update the locking
in the driver, there are 2 things to consider:
1) Use iio_device_claim_direct_mode() helper functions instead of
checking iio_buffer_enabled and grabbing mlock.
2) Any other uses of indio_dev->mlock are best moved to a private data
lock.  We want to return that mlock to an INTERNAL (core) use only.

alisons





> -- 
> Pavel Andrianov
> Linux Verification Center, ISPRAS
> web: http://linuxtesting.org
> e-mail: andria...@ispras.ru
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[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

Re: A potential race

[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

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

Re: A potential race

[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

Re: A potential race

[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