* [PATCH] watchdog: mena21_wdt: Fix a possible data race in a21_wdt_set_timeout
@ 2018-05-08 3:18 Jia-Ju Bai
2018-05-08 3:28 ` Guenter Roeck
0 siblings, 1 reply; 5+ messages in thread
From: Jia-Ju Bai @ 2018-05-08 3:18 UTC (permalink / raw)
To: morbidrsa, wim, linux; +Cc: linux-watchdog, linux-kernel, 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] watchdog: mena21_wdt: Fix a possible data race in a21_wdt_set_timeout
2018-05-08 3:18 [PATCH] watchdog: mena21_wdt: Fix a possible data race in a21_wdt_set_timeout Jia-Ju Bai
@ 2018-05-08 3:28 ` Guenter Roeck
2018-05-08 3:32 ` Jia-Ju Bai
0 siblings, 1 reply; 5+ messages in thread
From: Guenter Roeck @ 2018-05-08 3:28 UTC (permalink / raw)
To: Jia-Ju Bai, morbidrsa, wim; +Cc: linux-watchdog, linux-kernel
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] watchdog: mena21_wdt: Fix a possible data race in a21_wdt_set_timeout
2018-05-08 3:28 ` Guenter Roeck
@ 2018-05-08 3:32 ` Jia-Ju Bai
2018-05-08 3:42 ` Guenter Roeck
0 siblings, 1 reply; 5+ messages in thread
From: Jia-Ju Bai @ 2018-05-08 3:32 UTC (permalink / raw)
To: Guenter Roeck, morbidrsa, wim; +Cc: linux-watchdog, linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] watchdog: mena21_wdt: Fix a possible data race in a21_wdt_set_timeout
2018-05-08 3:32 ` Jia-Ju Bai
@ 2018-05-08 3:42 ` Guenter Roeck
2018-05-08 3:47 ` Jia-Ju Bai
0 siblings, 1 reply; 5+ messages in thread
From: Guenter Roeck @ 2018-05-08 3:42 UTC (permalink / raw)
To: Jia-Ju Bai, morbidrsa, wim; +Cc: linux-watchdog, linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] watchdog: mena21_wdt: Fix a possible data race in a21_wdt_set_timeout
2018-05-08 3:42 ` Guenter Roeck
@ 2018-05-08 3:47 ` Jia-Ju Bai
0 siblings, 0 replies; 5+ messages in thread
From: Jia-Ju Bai @ 2018-05-08 3:47 UTC (permalink / raw)
To: Guenter Roeck, morbidrsa, wim; +Cc: linux-watchdog, linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-05-08 3:47 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-08 3:18 [PATCH] watchdog: mena21_wdt: Fix a possible data race in a21_wdt_set_timeout Jia-Ju Bai
2018-05-08 3:28 ` Guenter Roeck
2018-05-08 3:32 ` Jia-Ju Bai
2018-05-08 3:42 ` Guenter Roeck
2018-05-08 3:47 ` Jia-Ju Bai
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.