* [PATCH v3] watchdog: s3c2410: Fix potential deadlock on &wdt->lock
@ 2023-07-05 9:09 Chengfeng Ye
2023-07-05 9:37 ` Krzysztof Kozlowski
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Chengfeng Ye @ 2023-07-05 9:09 UTC (permalink / raw)
To: krzysztof.kozlowski, alim.akhtar, wim, linux
Cc: linux-arm-kernel, linux-samsung-soc, linux-watchdog,
linux-kernel, Chengfeng Ye
As &wdt->lock is acquired by hard irq s3c2410wdt_irq(),
other acquisition of the same lock under process context should
disable irq, otherwise deadlock could happen if the
irq preempt the execution while the lock is held in process context
on the same CPU.
[Deadlock Scenario]
s3c2410wdt_suspend()
-> s3c2410wdt_stop()
-> spin_lock(&wdt->lock)
<irq iterrupt>
-> s3c2410wdt_irq()
-> s3c2410wdt_keepalive()
-> spin_lock(&wdt->lock) (deadlock here)
[Deadlock Scenario]
s3c2410wdt_probe()
-> s3c2410wdt_start()
-> spin_lock(&wdt->lock)
<irq iterrupt>
-> s3c2410wdt_irq()
-> s3c2410wdt_keepalive()
-> spin_lock(&wdt->lock) (deadlock here)
[Deadlock Scenario]
s3c2410wdt_keepalive()
-> spin_lock(&wdt->lock)
<irq iterrupt>
-> s3c2410wdt_irq()
-> s3c2410wdt_keepalive()
-> spin_lock(&wdt->lock) (deadlock here)
This flaw was found by an experimental static analysis tool I am
developing for irq-related deadlock, which reported the above
warning when analyzing the linux kernel 6.4-rc7 release.
The tentative patch fix the potential deadlock by spin_lock_irqsave()
under process context.
Signed-off-by: Chengfeng Ye <dg573847474@gmail.com>
---
drivers/watchdog/s3c2410_wdt.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 95416a9bdd4b..e1dc71ece01e 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -379,10 +379,11 @@ static int s3c2410wdt_enable(struct s3c2410_wdt *wdt, bool en)
static int s3c2410wdt_keepalive(struct watchdog_device *wdd)
{
struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
+ unsigned long flags;
- spin_lock(&wdt->lock);
+ spin_lock_irqsave(&wdt->lock, flags);
writel(wdt->count, wdt->reg_base + S3C2410_WTCNT);
- spin_unlock(&wdt->lock);
+ spin_unlock_irqrestore(&wdt->lock, flags);
return 0;
}
@@ -399,10 +400,11 @@ static void __s3c2410wdt_stop(struct s3c2410_wdt *wdt)
static int s3c2410wdt_stop(struct watchdog_device *wdd)
{
struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
+ unsigned long flags;
- spin_lock(&wdt->lock);
+ spin_lock_irqsave(&wdt->lock, flags);
__s3c2410wdt_stop(wdt);
- spin_unlock(&wdt->lock);
+ spin_unlock_irqrestore(&wdt->lock, flags);
return 0;
}
@@ -411,8 +413,9 @@ static int s3c2410wdt_start(struct watchdog_device *wdd)
{
unsigned long wtcon;
struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
+ unsigned long flags;
- spin_lock(&wdt->lock);
+ spin_lock_irqsave(&wdt->lock, flags);
__s3c2410wdt_stop(wdt);
@@ -433,7 +436,7 @@ static int s3c2410wdt_start(struct watchdog_device *wdd)
writel(wdt->count, wdt->reg_base + S3C2410_WTDAT);
writel(wdt->count, wdt->reg_base + S3C2410_WTCNT);
writel(wtcon, wdt->reg_base + S3C2410_WTCON);
- spin_unlock(&wdt->lock);
+ spin_unlock_irqrestore(&wdt->lock, flags);
return 0;
}
--
2.17.1
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v3] watchdog: s3c2410: Fix potential deadlock on &wdt->lock
2023-07-05 9:09 [PATCH v3] watchdog: s3c2410: Fix potential deadlock on &wdt->lock Chengfeng Ye
@ 2023-07-05 9:37 ` Krzysztof Kozlowski
2023-07-05 12:51 ` Guenter Roeck
2023-07-05 14:53 ` Guenter Roeck
2 siblings, 0 replies; 6+ messages in thread
From: Krzysztof Kozlowski @ 2023-07-05 9:37 UTC (permalink / raw)
To: Chengfeng Ye, alim.akhtar, wim, linux
Cc: linux-arm-kernel, linux-samsung-soc, linux-watchdog, linux-kernel
On 05/07/2023 11:09, Chengfeng Ye wrote:
> As &wdt->lock is acquired by hard irq s3c2410wdt_irq(),
> other acquisition of the same lock under process context should
> disable irq, otherwise deadlock could happen if the
> irq preempt the execution while the lock is held in process context
> on the same CPU.
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] watchdog: s3c2410: Fix potential deadlock on &wdt->lock
2023-07-05 9:09 [PATCH v3] watchdog: s3c2410: Fix potential deadlock on &wdt->lock Chengfeng Ye
2023-07-05 9:37 ` Krzysztof Kozlowski
@ 2023-07-05 12:51 ` Guenter Roeck
2023-07-05 14:40 ` Chengfeng Ye
2023-07-05 14:53 ` Guenter Roeck
2 siblings, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2023-07-05 12:51 UTC (permalink / raw)
To: Chengfeng Ye, krzysztof.kozlowski, alim.akhtar, wim
Cc: linux-arm-kernel, linux-samsung-soc, linux-watchdog, linux-kernel
On 7/5/23 02:09, Chengfeng Ye wrote:
> As &wdt->lock is acquired by hard irq s3c2410wdt_irq(),
> other acquisition of the same lock under process context should
> disable irq, otherwise deadlock could happen if the
> irq preempt the execution while the lock is held in process context
> on the same CPU.
>
> [Deadlock Scenario]
> s3c2410wdt_suspend()
> -> s3c2410wdt_stop()
> -> spin_lock(&wdt->lock)
> <irq iterrupt>
> -> s3c2410wdt_irq()
> -> s3c2410wdt_keepalive()
> -> spin_lock(&wdt->lock) (deadlock here)
>
> [Deadlock Scenario]
> s3c2410wdt_probe()
> -> s3c2410wdt_start()
> -> spin_lock(&wdt->lock)
> <irq iterrupt>
> -> s3c2410wdt_irq()
> -> s3c2410wdt_keepalive()
> -> spin_lock(&wdt->lock) (deadlock here)
>
> [Deadlock Scenario]
> s3c2410wdt_keepalive()
> -> spin_lock(&wdt->lock)
> <irq iterrupt>
> -> s3c2410wdt_irq()
> -> s3c2410wdt_keepalive()
> -> spin_lock(&wdt->lock) (deadlock here)
>
> This flaw was found by an experimental static analysis tool I am
> developing for irq-related deadlock, which reported the above
> warning when analyzing the linux kernel 6.4-rc7 release.
>
> The tentative patch fix the potential deadlock by spin_lock_irqsave()
> under process context.
>
> Signed-off-by: Chengfeng Ye <dg573847474@gmail.com>
I am sure you know what you changed in each version of your patches. I don't.
Please provide change logs when you send new versions of your patches.
Guenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] watchdog: s3c2410: Fix potential deadlock on &wdt->lock
2023-07-05 12:51 ` Guenter Roeck
@ 2023-07-05 14:40 ` Chengfeng Ye
0 siblings, 0 replies; 6+ messages in thread
From: Chengfeng Ye @ 2023-07-05 14:40 UTC (permalink / raw)
To: Guenter Roeck
Cc: krzysztof.kozlowski, alim.akhtar, wim, linux-arm-kernel,
linux-samsung-soc, linux-watchdog, linux-kernel
> I am sure you know what you changed in each version of your patches. I don't.
> Please provide change logs when you send new versions of your patches.
No problem, this is the change log for this patch.
---
Changes in v3:
- Also use spin_lock_irqsave() in s3c2410wdt_keepalive().
---
---
Changes in v2:
- Use function name instead of line number in the commit message.
---
Best Regards,
Chengfeng
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] watchdog: s3c2410: Fix potential deadlock on &wdt->lock
2023-07-05 9:09 [PATCH v3] watchdog: s3c2410: Fix potential deadlock on &wdt->lock Chengfeng Ye
2023-07-05 9:37 ` Krzysztof Kozlowski
2023-07-05 12:51 ` Guenter Roeck
@ 2023-07-05 14:53 ` Guenter Roeck
2023-07-19 18:39 ` Chengfeng Ye
2 siblings, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2023-07-05 14:53 UTC (permalink / raw)
To: Chengfeng Ye, krzysztof.kozlowski, alim.akhtar, wim
Cc: linux-arm-kernel, linux-samsung-soc, linux-watchdog, linux-kernel
On 7/5/23 02:09, Chengfeng Ye wrote:
> As &wdt->lock is acquired by hard irq s3c2410wdt_irq(),
> other acquisition of the same lock under process context should
> disable irq, otherwise deadlock could happen if the
> irq preempt the execution while the lock is held in process context
> on the same CPU.
>
> [Deadlock Scenario]
> s3c2410wdt_suspend()
> -> s3c2410wdt_stop()
> -> spin_lock(&wdt->lock)
> <irq iterrupt>
> -> s3c2410wdt_irq()
> -> s3c2410wdt_keepalive()
> -> spin_lock(&wdt->lock) (deadlock here)
>
> [Deadlock Scenario]
> s3c2410wdt_probe()
> -> s3c2410wdt_start()
> -> spin_lock(&wdt->lock)
> <irq iterrupt>
> -> s3c2410wdt_irq()
> -> s3c2410wdt_keepalive()
> -> spin_lock(&wdt->lock) (deadlock here)
>
> [Deadlock Scenario]
> s3c2410wdt_keepalive()
> -> spin_lock(&wdt->lock)
> <irq iterrupt>
> -> s3c2410wdt_irq()
> -> s3c2410wdt_keepalive()
> -> spin_lock(&wdt->lock) (deadlock here)
>
> This flaw was found by an experimental static analysis tool I am
> developing for irq-related deadlock, which reported the above
> warning when analyzing the linux kernel 6.4-rc7 release.
>
> The tentative patch fix the potential deadlock by spin_lock_irqsave()
> under process context.
>
> Signed-off-by: Chengfeng Ye <dg573847474@gmail.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> ---
> drivers/watchdog/s3c2410_wdt.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index 95416a9bdd4b..e1dc71ece01e 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -379,10 +379,11 @@ static int s3c2410wdt_enable(struct s3c2410_wdt *wdt, bool en)
> static int s3c2410wdt_keepalive(struct watchdog_device *wdd)
> {
> struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
> + unsigned long flags;
>
> - spin_lock(&wdt->lock);
> + spin_lock_irqsave(&wdt->lock, flags);
> writel(wdt->count, wdt->reg_base + S3C2410_WTCNT);
> - spin_unlock(&wdt->lock);
> + spin_unlock_irqrestore(&wdt->lock, flags);
>
> return 0;
> }
> @@ -399,10 +400,11 @@ static void __s3c2410wdt_stop(struct s3c2410_wdt *wdt)
> static int s3c2410wdt_stop(struct watchdog_device *wdd)
> {
> struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
> + unsigned long flags;
>
> - spin_lock(&wdt->lock);
> + spin_lock_irqsave(&wdt->lock, flags);
> __s3c2410wdt_stop(wdt);
> - spin_unlock(&wdt->lock);
> + spin_unlock_irqrestore(&wdt->lock, flags);
>
> return 0;
> }
> @@ -411,8 +413,9 @@ static int s3c2410wdt_start(struct watchdog_device *wdd)
> {
> unsigned long wtcon;
> struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
> + unsigned long flags;
>
> - spin_lock(&wdt->lock);
> + spin_lock_irqsave(&wdt->lock, flags);
>
> __s3c2410wdt_stop(wdt);
>
> @@ -433,7 +436,7 @@ static int s3c2410wdt_start(struct watchdog_device *wdd)
> writel(wdt->count, wdt->reg_base + S3C2410_WTDAT);
> writel(wdt->count, wdt->reg_base + S3C2410_WTCNT);
> writel(wtcon, wdt->reg_base + S3C2410_WTCON);
> - spin_unlock(&wdt->lock);
> + spin_unlock_irqrestore(&wdt->lock, flags);
>
> return 0;
> }
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v3] watchdog: s3c2410: Fix potential deadlock on &wdt->lock
2023-07-05 14:53 ` Guenter Roeck
@ 2023-07-19 18:39 ` Chengfeng Ye
0 siblings, 0 replies; 6+ messages in thread
From: Chengfeng Ye @ 2023-07-19 18:39 UTC (permalink / raw)
To: Guenter Roeck, krzysztof.kozlowski, alim.akhtar, wim
Cc: linux-arm-kernel, linux-samsung-soc, linux-watchdog, linux-kernel
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Thanks much for your time in reviewing the patch :)
Best Regards,
Chengfeng
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-07-19 18:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-05 9:09 [PATCH v3] watchdog: s3c2410: Fix potential deadlock on &wdt->lock Chengfeng Ye
2023-07-05 9:37 ` Krzysztof Kozlowski
2023-07-05 12:51 ` Guenter Roeck
2023-07-05 14:40 ` Chengfeng Ye
2023-07-05 14:53 ` Guenter Roeck
2023-07-19 18:39 ` Chengfeng Ye
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®