[PATCH] watchdog: mena21_wdt: Fix a possible data race in a21_wdt_set_timeout

[PATCH] watchdog: mena21_wdt: Fix a possible data race in a21_wdt_set_timeout

[Jia-Ju Bai]

The write operation to "wdt->timeout" is protected by
the lock on line 118, but the read operation to
this data on line 105 is not protected by the lock.
Thus, there may exist a data race for "wdt->timeout".

To fix this data race, the read operation to "wdt->timeout" 
should be also protected by the lock.

Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
---
 drivers/watchdog/mena21_wdt.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/mena21_wdt.c b/drivers/watchdog/mena21_wdt.c
index 25d5d2b8cfbe..05ca69042829 100644
--- a/drivers/watchdog/mena21_wdt.c
+++ b/drivers/watchdog/mena21_wdt.c
@@ -102,14 +102,15 @@ static int a21_wdt_set_timeout(struct watchdog_device *wdt,
 		return -EINVAL;
 	}
 
+	mutex_lock(&drv->lock);
+
 	if (timeout == 30 && wdt->timeout == 1) {
+		mutex_unlock(&drv->lock);
 		dev_err(wdt->parent,
 			"Transition from fast to slow mode not allowed\n");
 		return -EINVAL;
 	}
 
-	mutex_lock(&drv->lock);
-
 	if (timeout == 1)
 		gpio_set_value(drv->gpios[GPIO_WD_FAST], 1);
 	else
-- 
2.17.0

Re: [PATCH] watchdog: mena21_wdt: Fix a possible data race in a21_wdt_set_timeout

[Guenter Roeck]

On 05/07/2018 08:18 PM, Jia-Ju Bai wrote:
> The write operation to "wdt->timeout" is protected by
> the lock on line 118, but the read operation to
> this data on line 105 is not protected by the lock.
> Thus, there may exist a data race for "wdt->timeout".
> 
> To fix this data race, the read operation to "wdt->timeout"
> should be also protected by the lock.
> 

There is no race. There is already a mutex in the watchdog core which serializes
calls to the various API functions. It would make more sense to drop drv->lock
from the driver.

Guenter

> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
> ---
>   drivers/watchdog/mena21_wdt.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/mena21_wdt.c b/drivers/watchdog/mena21_wdt.c
> index 25d5d2b8cfbe..05ca69042829 100644
> --- a/drivers/watchdog/mena21_wdt.c
> +++ b/drivers/watchdog/mena21_wdt.c
> @@ -102,14 +102,15 @@ static int a21_wdt_set_timeout(struct watchdog_device *wdt,
>   		return -EINVAL;
>   	}
>   
> +	mutex_lock(&drv->lock);
> +
>   	if (timeout == 30 && wdt->timeout == 1) {
> +		mutex_unlock(&drv->lock);
>   		dev_err(wdt->parent,
>   			"Transition from fast to slow mode not allowed\n");
>   		return -EINVAL;
>   	}
>   
> -	mutex_lock(&drv->lock);
> -
>   	if (timeout == 1)
>   		gpio_set_value(drv->gpios[GPIO_WD_FAST], 1);
>   	else
> 

Re: [PATCH] watchdog: mena21_wdt: Fix a possible data race in a21_wdt_set_timeout

[Jia-Ju Bai]

On 2018/5/8 11:28, Guenter Roeck wrote:
> On 05/07/2018 08:18 PM, Jia-Ju Bai wrote:
>> The write operation to "wdt->timeout" is protected by
>> the lock on line 118, but the read operation to
>> this data on line 105 is not protected by the lock.
>> Thus, there may exist a data race for "wdt->timeout".
>>
>> To fix this data race, the read operation to "wdt->timeout"
>> should be also protected by the lock.
>>
>
> There is no race. There is already a mutex in the watchdog core which 
> serializes
> calls to the various API functions. It would make more sense to drop 
> drv->lock
> from the driver.
>

Thanks for your reply :)
Need I submit a patch of dropping all calls to "drv->lock"?


Best wishes,
Jia-Ju Bai

Re: [PATCH] watchdog: mena21_wdt: Fix a possible data race in a21_wdt_set_timeout

[Guenter Roeck]

On 05/07/2018 08:32 PM, Jia-Ju Bai wrote:
> 
> 
> On 2018/5/8 11:28, Guenter Roeck wrote:
>> On 05/07/2018 08:18 PM, Jia-Ju Bai wrote:
>>> The write operation to "wdt->timeout" is protected by
>>> the lock on line 118, but the read operation to
>>> this data on line 105 is not protected by the lock.
>>> Thus, there may exist a data race for "wdt->timeout".
>>>
>>> To fix this data race, the read operation to "wdt->timeout"
>>> should be also protected by the lock.
>>>
>>
>> There is no race. There is already a mutex in the watchdog core which serializes
>> calls to the various API functions. It would make more sense to drop drv->lock
>> from the driver.
>>
> 
> Thanks for your reply :)
> Need I submit a patch of dropping all calls to "drv->lock"?
> 

You don't _need_ to, but I would happily give it my Reviewed-by: tag if you do.

Guenter

Re: [PATCH] watchdog: mena21_wdt: Fix a possible data race in a21_wdt_set_timeout

[Jia-Ju Bai]

On 2018/5/8 11:42, Guenter Roeck wrote:
> On 05/07/2018 08:32 PM, Jia-Ju Bai wrote:
>>
>>
>> On 2018/5/8 11:28, Guenter Roeck wrote:
>>> On 05/07/2018 08:18 PM, Jia-Ju Bai wrote:
>>>> The write operation to "wdt->timeout" is protected by
>>>> the lock on line 118, but the read operation to
>>>> this data on line 105 is not protected by the lock.
>>>> Thus, there may exist a data race for "wdt->timeout".
>>>>
>>>> To fix this data race, the read operation to "wdt->timeout"
>>>> should be also protected by the lock.
>>>>
>>>
>>> There is no race. There is already a mutex in the watchdog core 
>>> which serializes
>>> calls to the various API functions. It would make more sense to drop 
>>> drv->lock
>>> from the driver.
>>>
>>
>> Thanks for your reply :)
>> Need I submit a patch of dropping all calls to "drv->lock"?
>>
>
> You don't _need_ to, but I would happily give it my Reviewed-by: tag 
> if you do.
>

Okay, I will write a patch today :)


Best wishes,
Jia-Ju Bai