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

[PATCH] 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.

  [Interrupt]: s3c2410wdt_irq
    -->/root/linux/drivers/watchdog/s3c2410_wdt.c:547
    -->/root/linux/drivers/watchdog/s3c2410_wdt.c:383
    -->spin_lock(&wdt->lock);

  [Process Context]: s3c2410wdt_suspend
    -->/root/linux/drivers/watchdog/s3c2410_wdt.c:764
    -->/root/linux/drivers/watchdog/s3c2410_wdt.c:403
    -->spin_lock(&wdt->lock);

  [Process Context]: s3c2410wdt_probe
    -->/root/linux/drivers/watchdog/s3c2410_wdt.c:710
    -->/root/linux/drivers/watchdog/s3c2410_wdt.c:415
    -->spin_lock(&wdt->lock);

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().

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] watchdog: s3c2410: Fix potential deadlock on &wdt->lock

[Krzysztof Kozlowski]

On 28/06/2023 18:47, 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.
> 
>   [Interrupt]: s3c2410wdt_irq
>     -->/root/linux/drivers/watchdog/s3c2410_wdt.c:547
>     -->/root/linux/drivers/watchdog/s3c2410_wdt.c:383
>     -->spin_lock(&wdt->lock);

This interrupt is a threaded interrupt. Therefore the
s3c2410wdt_keepalive() will be called again from process thread. Are you
sure there is deadlock?

Anyway, please also strip unrelated paths and rather use function names,
not references to lines, because these might be not accurate.


Best regards,
Krzysztof

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

[Chengfeng Ye]

Please kindly note that I am resending the last email since it did not
reach maillist.

> Hi, Krzysztof
> Thanks for the reply.
> > This interrupt is a threaded interrupt. Therefore the
> > s3c2410wdt_keepalive() will be called again from process thread. Are you
> > sure there is deadlock?
> Is it really that s3c2410wdt_irq is a threaded interrupt? I could be wrong but I can
> see that the interrupt is registered via the following code. It is the third argument
> of devm_request_irq but not devm_request_threaded_irq or request_threaded_irq,
> as far as I know, it should be an interrupt handler for the interrupt line wdt_irq
> executed under irq context.
> ret = devm_request_irq(dev, wdt_irq, s3c2410wdt_irq, 0, pdev->name, pdev);
> > Anyway, please also strip unrelated paths and rather use function names,
> > not references to lines, because these might be not accurate.
> No problem, I will provide a new patch with the function name soon.
> Best Regards,
> Chengfeng

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

[Krzysztof Kozlowski]

On 04/07/2023 18:10, Chengfeng Ye wrote:
> Hi, Krzysztof
> 
> Thanks for the reply.
> 
>> This interrupt is a threaded interrupt. Therefore the
>> s3c2410wdt_keepalive() will be called again from process thread. Are you
>> sure there is deadlock?
> 
> Is it really that s3c2410wdt_irq is a threaded interrupt? I could be wrong
> but I can
> see that the interrupt is registered via the following code. It is the
> third argument
> of devm_request_irq but not devm_request_threaded_irq or
> request_threaded_irq,
> as far as I know, it should be an interrupt handler for the interrupt line
> wdt_irq
> executed under irq context.
> 
> ret = devm_request_irq(dev, wdt_irq, s3c2410wdt_irq, 0, pdev->name, pdev);

Yes, you are right.

Best regards,
Krzysztof