Re: A potential race in drivers/iio/adc/vf610_adc.ko
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
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
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
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
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
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
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
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
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
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
A potential race in drivers/iio/adc/vf610_adc.ko
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
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
