[PATCH v2] watchdog: s3c2410: Fix potential deadlock on &wdt->lock

[PATCH v2] watchdog: s3c2410: Fix potential deadlock on &wdt->lock

[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)

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 | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 95416a9bdd4b..2dfc0d6a3004 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -399,10 +399,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 +412,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 +435,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

Re: [PATCH v2] watchdog: s3c2410: Fix potential deadlock on &wdt->lock

[Krzysztof Kozlowski]

On 05/07/2023 09:23, 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)
> 
> 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 | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index 95416a9bdd4b..2dfc0d6a3004 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -399,10 +399,11 @@ static void __s3c2410wdt_stop(struct s3c2410_wdt *wdt)

Why you didn't update also s3c2410wdt_keepalive()?

Best regards,
Krzysztof

Re: [PATCH v2] watchdog: s3c2410: Fix potential deadlock on &wdt->lock

[Chengfeng Ye]

Hi Krzysztof,

Thanks for the reminder!

I didn't notice that s3c2410wdt_keepalive() can also be called from
.ping callback. Would send a v3 patch soon.

Best Regards,
Chengfeng

Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 于2023年7月5日周三 16:58写道：
>
> On 05/07/2023 09:23, 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)
> >
> > 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 | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> > index 95416a9bdd4b..2dfc0d6a3004 100644
> > --- a/drivers/watchdog/s3c2410_wdt.c
> > +++ b/drivers/watchdog/s3c2410_wdt.c
> > @@ -399,10 +399,11 @@ static void __s3c2410wdt_stop(struct s3c2410_wdt *wdt)
>
> Why you didn't update also s3c2410wdt_keepalive()?
>
> Best regards,
> Krzysztof
>