watchdog: core: Fix circular locking dependency

PATCH In review Guenter Roeck 4 participants 9 messages Apr 21, 2016 – May 14, 2016 Watchdog device drivers

lore

Share & tools

lore

Guenter Roeck

author maintainer

2016-04-21 14:38 UTC #0 lore raw

PATCH 1/1 watchdog: core: Fix circular locking dependency

lockdep reports the following circular locking dependency.

======================================================

INFO: possible circular locking dependency detected ]

4.6.0-rc3-00191-gfabf418 #162 Not tainted
-------------------------------------------------------
systemd/1 is trying to acquire lock:
((&(&wd_data->work)->work)){+.+...}, at: [<80141650>] flush_work+0x0/0x280

but task is already holding lock:

(&wd_data->lock){+.+...}, at: [<804acfa8>] watchdog_release+0x18/0x190

which lock already depends on the new lock. the existing dependency chain (in reverse order) is:

-> #1 (&wd_data->lock){+.+...}:

	[<80662310>] mutex_lock_nested+0x64/0x4a8
	[<804aca4c>] watchdog_ping_work+0x18/0x4c
	[<80143128>] process_one_work+0x1ac/0x500
	[<801434b4>] worker_thread+0x38/0x554
	[<80149510>] kthread+0xf4/0x108
	[<80107c10>] ret_from_fork+0x14/0x24

-> #0 ((&(&wd_data->work)->work)){+.+...}:

	[<8017c4e8>] lock_acquire+0x70/0x90
	[<8014169c>] flush_work+0x4c/0x280
	[<801440f8>] __cancel_work_timer+0x9c/0x1e0
	[<804acfcc>] watchdog_release+0x3c/0x190
	[<8022c5e8>] __fput+0x80/0x1c8
	[<80147b28>] task_work_run+0x94/0xc8
	[<8010b998>] do_work_pending+0x8c/0xb4
	[<80107ba8>] slow_work_pending+0xc/0x20

other info that might help us debug this:

Possible unsafe locking scenario:

CPU0 CPU1
---- ----

lock(&wd_data->lock);
                        lock((&(&wd_data->work)->work));
                        lock(&wd_data->lock);

lock((&(&wd_data->work)->work));

*** DEADLOCK ***

1 lock held by systemd/1:

stack backtrace:
CPU: 2 PID: 1 Comm: systemd Not tainted 4.6.0-rc3-00191-gfabf418 #162
Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[<8010f5e4>] (unwind_backtrace) from [<8010c038>] (show_stack+0x10/0x14)
[<8010c038>] (show_stack) from [<8039d7fc>] (dump_stack+0xa8/0xd4)
[<8039d7fc>] (dump_stack) from [<80177ee0>] (print_circular_bug+0x214/0x334)
[<80177ee0>] (print_circular_bug) from [<80179230>] (check_prevs_add+0x4dc/0x8e8)
[<80179230>] (check_prevs_add) from [<8017b3d8>] (__lock_acquire+0xc6c/0x14ec)
[<8017b3d8>] (__lock_acquire) from [<8017c4e8>] (lock_acquire+0x70/0x90)
[<8017c4e8>] (lock_acquire) from [<8014169c>] (flush_work+0x4c/0x280)
[<8014169c>] (flush_work) from [<801440f8>] (__cancel_work_timer+0x9c/0x1e0)
[<801440f8>] (__cancel_work_timer) from [<804acfcc>] (watchdog_release+0x3c/0x190)
[<804acfcc>] (watchdog_release) from [<8022c5e8>] (__fput+0x80/0x1c8)
[<8022c5e8>] (__fput) from [<80147b28>] (task_work_run+0x94/0xc8)
[<80147b28>] (task_work_run) from [<8010b998>] (do_work_pending+0x8c/0xb4)
[<8010b998>] (do_work_pending) from [<80107ba8>] (slow_work_pending+0xc/0x20)

Turns out the call to cancel_delayed_work_sync() in watchdog_release() is not necessary and can be dropped. If the worker is no longer necessary, the subsequent call to watchdog_update_worker() will cancel it. If it is already running, it won't do anything, since the worker function checks if it needs to ping the watchdog or not.

Reported-by Clemens Gruber <[email protected]>

Tested-by Clemens Gruber <[email protected]>

Fixes 11d7aba9ceb7 ("watchdog: imx2: Convert to use infrastructure triggered keepalives")

Signed-off-by Guenter Roeck <[email protected]>

---
 drivers/watchdog/watchdog_dev.c | 1 -
 1 file changed, 1 deletion(-)

drivers/watchdog/watchdog_dev.c +0 -1

index e2c5abbb45ff..3595cffa24ea 100644
@@ -736,7 +736,6 @@ static int watchdog_release(struct inode *inode, struct file *file)
 		watchdog_ping(wdd);
 	}
 
-	cancel_delayed_work_sync(&wd_data->work);
 	watchdog_update_worker(wdd);
 
 	/* make sure that /dev/watchdog can be re-opened */

Signature · 1 line

2.5.0

One Thousand Gnomes

2016-04-21 14:50 UTC #1 lore raw

Quoted 5 lines

Turns out the call to cancel_delayed_work_sync() in watchdog_release() is not necessary and can be dropped. If the worker is no longer necessary, the subsequent call to watchdog_update_worker() will cancel it. If it is already running, it won't do anything, since the worker function checks if it needs to ping the watchdog or not.

Is this actually true. Consider the pathalogical case of the device being closed and the modue unloaded. In that case the close completes, we drop the module count but could still do work on it.

Alan

Guenter Roeck

author maintainer

2016-04-21 23:30 UTC #2 lore raw

Quoted 10 lines · One Thousand Gnomes

Quoted 5 lines
Turns out the call to cancel_delayed_work_sync() in watchdog_release() is not necessary and can be dropped. If the worker is no longer necessary, the subsequent call to watchdog_update_worker() will cancel it. If it is already running, it won't do anything, since the worker function checks if it needs to ping the watchdog or not.
Is this actually true. Consider the pathalogical case of the device being closed and the modue unloaded. In that case the close completes, we drop the module count but could still do work on it.

Module unload is handled separately and also calls cancel_delayed_work_sync().

Guenter

Clemens Gruber

2016-05-09 13:53 UTC #3 lore raw

In reply to Guenter Roeck

Quoted 94 lines · Guenter Roeck

lockdep reports the following circular locking dependency.
======================================================
INFO: possible circular locking dependency detected ]
4.6.0-rc3-00191-gfabf418 #162 Not tainted
-------------------------------------------------------
systemd/1 is trying to acquire lock:
((&(&wd_data->work)->work)){+.+...}, at: [<80141650>] flush_work+0x0/0x280
but task is already holding lock:
(&wd_data->lock){+.+...}, at: [<804acfa8>] watchdog_release+0x18/0x190
which lock already depends on the new lock. the existing dependency chain (in reverse order) is:
-> #1 (&wd_data->lock){+.+...}:
	[<80662310>] mutex_lock_nested+0x64/0x4a8
	[<804aca4c>] watchdog_ping_work+0x18/0x4c
	[<80143128>] process_one_work+0x1ac/0x500
	[<801434b4>] worker_thread+0x38/0x554
	[<80149510>] kthread+0xf4/0x108
	[<80107c10>] ret_from_fork+0x14/0x24
-> #0 ((&(&wd_data->work)->work)){+.+...}:
	[<8017c4e8>] lock_acquire+0x70/0x90
	[<8014169c>] flush_work+0x4c/0x280
	[<801440f8>] __cancel_work_timer+0x9c/0x1e0
	[<804acfcc>] watchdog_release+0x3c/0x190
	[<8022c5e8>] __fput+0x80/0x1c8
	[<80147b28>] task_work_run+0x94/0xc8
	[<8010b998>] do_work_pending+0x8c/0xb4
	[<80107ba8>] slow_work_pending+0xc/0x20
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&wd_data->lock);
                        lock((&(&wd_data->work)->work));
                        lock(&wd_data->lock);
lock((&(&wd_data->work)->work));
*** DEADLOCK ***
1 lock held by systemd/1:
stack backtrace:
CPU: 2 PID: 1 Comm: systemd Not tainted 4.6.0-rc3-00191-gfabf418 #162
Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[<8010f5e4>] (unwind_backtrace) from [<8010c038>] (show_stack+0x10/0x14)
[<8010c038>] (show_stack) from [<8039d7fc>] (dump_stack+0xa8/0xd4)
[<8039d7fc>] (dump_stack) from [<80177ee0>] (print_circular_bug+0x214/0x334)
[<80177ee0>] (print_circular_bug) from [<80179230>] (check_prevs_add+0x4dc/0x8e8)
[<80179230>] (check_prevs_add) from [<8017b3d8>] (__lock_acquire+0xc6c/0x14ec)
[<8017b3d8>] (__lock_acquire) from [<8017c4e8>] (lock_acquire+0x70/0x90)
[<8017c4e8>] (lock_acquire) from [<8014169c>] (flush_work+0x4c/0x280)
[<8014169c>] (flush_work) from [<801440f8>] (__cancel_work_timer+0x9c/0x1e0)
[<801440f8>] (__cancel_work_timer) from [<804acfcc>] (watchdog_release+0x3c/0x190)
[<804acfcc>] (watchdog_release) from [<8022c5e8>] (__fput+0x80/0x1c8)
[<8022c5e8>] (__fput) from [<80147b28>] (task_work_run+0x94/0xc8)
[<80147b28>] (task_work_run) from [<8010b998>] (do_work_pending+0x8c/0xb4)
[<8010b998>] (do_work_pending) from [<80107ba8>] (slow_work_pending+0xc/0x20)
Turns out the call to cancel_delayed_work_sync() in watchdog_release() is not necessary and can be dropped. If the worker is no longer necessary, the subsequent call to watchdog_update_worker() will cancel it. If it is already running, it won't do anything, since the worker function checks if it needs to ping the watchdog or not.
Reported-by Clemens Gruber <[email protected]>
Tested-by Clemens Gruber <[email protected]>
Fixes 11d7aba9ceb7 ("watchdog: imx2: Convert to use infrastructure triggered keepalives")
Signed-off-by Guenter Roeck <[email protected]>
---
 drivers/watchdog/watchdog_dev.c | 1 -
 1 file changed, 1 deletion(-)
drivers/watchdog/watchdog_dev.c +0 -1
index e2c5abbb45ff..3595cffa24ea 100644
@@ -736,7 +736,6 @@ static int watchdog_release(struct inode *inode, struct file *file)
 		watchdog_ping(wdd);
 	}
 
-	cancel_delayed_work_sync(&wd_data->work);
 	watchdog_update_worker(wdd);
 
 	/* make sure that /dev/watchdog can be re-opened */
Signature · 1 line
2.5.0

Hi,

I don't see this patch in the torvalds/linux tree.

Will this get in before the 4.6 release?

Thanks,
Clemens

Guenter Roeck

author maintainer

2016-05-09 14:18 UTC #4 lore raw

Quoted 98 lines · Clemens Gruber

Quoted 92 lines · Guenter Roeck
lockdep reports the following circular locking dependency.
======================================================
INFO: possible circular locking dependency detected ]
4.6.0-rc3-00191-gfabf418 #162 Not tainted
-------------------------------------------------------
systemd/1 is trying to acquire lock:
((&(&wd_data->work)->work)){+.+...}, at: [<80141650>] flush_work+0x0/0x280
but task is already holding lock:
(&wd_data->lock){+.+...}, at: [<804acfa8>] watchdog_release+0x18/0x190
which lock already depends on the new lock. the existing dependency chain (in reverse order) is:
-> #1 (&wd_data->lock){+.+...}:
	[<80662310>] mutex_lock_nested+0x64/0x4a8
	[<804aca4c>] watchdog_ping_work+0x18/0x4c
	[<80143128>] process_one_work+0x1ac/0x500
	[<801434b4>] worker_thread+0x38/0x554
	[<80149510>] kthread+0xf4/0x108
	[<80107c10>] ret_from_fork+0x14/0x24
-> #0 ((&(&wd_data->work)->work)){+.+...}:
	[<8017c4e8>] lock_acquire+0x70/0x90
	[<8014169c>] flush_work+0x4c/0x280
	[<801440f8>] __cancel_work_timer+0x9c/0x1e0
	[<804acfcc>] watchdog_release+0x3c/0x190
	[<8022c5e8>] __fput+0x80/0x1c8
	[<80147b28>] task_work_run+0x94/0xc8
	[<8010b998>] do_work_pending+0x8c/0xb4
	[<80107ba8>] slow_work_pending+0xc/0x20
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&wd_data->lock);
                         lock((&(&wd_data->work)->work));
                         lock(&wd_data->lock);
lock((&(&wd_data->work)->work));
*** DEADLOCK ***
1 lock held by systemd/1:
stack backtrace:
CPU: 2 PID: 1 Comm: systemd Not tainted 4.6.0-rc3-00191-gfabf418 #162
Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[<8010f5e4>] (unwind_backtrace) from [<8010c038>] (show_stack+0x10/0x14)
[<8010c038>] (show_stack) from [<8039d7fc>] (dump_stack+0xa8/0xd4)
[<8039d7fc>] (dump_stack) from [<80177ee0>] (print_circular_bug+0x214/0x334)
[<80177ee0>] (print_circular_bug) from [<80179230>] (check_prevs_add+0x4dc/0x8e8)
[<80179230>] (check_prevs_add) from [<8017b3d8>] (__lock_acquire+0xc6c/0x14ec)
[<8017b3d8>] (__lock_acquire) from [<8017c4e8>] (lock_acquire+0x70/0x90)
[<8017c4e8>] (lock_acquire) from [<8014169c>] (flush_work+0x4c/0x280)
[<8014169c>] (flush_work) from [<801440f8>] (__cancel_work_timer+0x9c/0x1e0)
[<801440f8>] (__cancel_work_timer) from [<804acfcc>] (watchdog_release+0x3c/0x190)
[<804acfcc>] (watchdog_release) from [<8022c5e8>] (__fput+0x80/0x1c8)
[<8022c5e8>] (__fput) from [<80147b28>] (task_work_run+0x94/0xc8)
[<80147b28>] (task_work_run) from [<8010b998>] (do_work_pending+0x8c/0xb4)
[<8010b998>] (do_work_pending) from [<80107ba8>] (slow_work_pending+0xc/0x20)
Turns out the call to cancel_delayed_work_sync() in watchdog_release() is not necessary and can be dropped. If the worker is no longer necessary, the subsequent call to watchdog_update_worker() will cancel it. If it is already running, it won't do anything, since the worker function checks if it needs to ping the watchdog or not.
Reported-by Clemens Gruber <[email protected]>
Tested-by Clemens Gruber <[email protected]>
Fixes 11d7aba9ceb7 ("watchdog: imx2: Convert to use infrastructure triggered keepalives")
Signed-off-by Guenter Roeck <[email protected]>
---
  drivers/watchdog/watchdog_dev.c | 1 -
  1 file changed, 1 deletion(-)
drivers/watchdog/watchdog_dev.c +0 -2
index e2c5abbb45ff..3595cffa24ea 100644
@@ -736,7 +736,6 @@ static int watchdog_release(struct inode *inode, struct file *file)
  		watchdog_ping(wdd);
  	}
 
-	cancel_delayed_work_sync(&wd_data->work);
  	watchdog_update_worker(wdd);
 
  	/* make sure that /dev/watchdog can be re-opened */
--
Quoted 2 lines
2.5.0
Hi,

Quoted 4 lines

I don't see this patch in the torvalds/linux tree.
Will this get in before the 4.6 release?

Hopefully. If not, I assume Wim will pick it up in the next commit window and it will be applied to -stable.

Guenter

Wim Van Sebroeck

2016-05-14 16:41 UTC #5 lore raw

In reply to Guenter Roeck

Hi Guenter,

Quoted 94 lines

lockdep reports the following circular locking dependency.
======================================================
INFO: possible circular locking dependency detected ]
4.6.0-rc3-00191-gfabf418 #162 Not tainted
-------------------------------------------------------
systemd/1 is trying to acquire lock:
((&(&wd_data->work)->work)){+.+...}, at: [<80141650>] flush_work+0x0/0x280
but task is already holding lock:
(&wd_data->lock){+.+...}, at: [<804acfa8>] watchdog_release+0x18/0x190
which lock already depends on the new lock. the existing dependency chain (in reverse order) is:
-> #1 (&wd_data->lock){+.+...}:
	[<80662310>] mutex_lock_nested+0x64/0x4a8
	[<804aca4c>] watchdog_ping_work+0x18/0x4c
	[<80143128>] process_one_work+0x1ac/0x500
	[<801434b4>] worker_thread+0x38/0x554
	[<80149510>] kthread+0xf4/0x108
	[<80107c10>] ret_from_fork+0x14/0x24
-> #0 ((&(&wd_data->work)->work)){+.+...}:
	[<8017c4e8>] lock_acquire+0x70/0x90
	[<8014169c>] flush_work+0x4c/0x280
	[<801440f8>] __cancel_work_timer+0x9c/0x1e0
	[<804acfcc>] watchdog_release+0x3c/0x190
	[<8022c5e8>] __fput+0x80/0x1c8
	[<80147b28>] task_work_run+0x94/0xc8
	[<8010b998>] do_work_pending+0x8c/0xb4
	[<80107ba8>] slow_work_pending+0xc/0x20
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&wd_data->lock);
                        lock((&(&wd_data->work)->work));
                        lock(&wd_data->lock);
lock((&(&wd_data->work)->work));
*** DEADLOCK ***
1 lock held by systemd/1:
stack backtrace:
CPU: 2 PID: 1 Comm: systemd Not tainted 4.6.0-rc3-00191-gfabf418 #162
Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[<8010f5e4>] (unwind_backtrace) from [<8010c038>] (show_stack+0x10/0x14)
[<8010c038>] (show_stack) from [<8039d7fc>] (dump_stack+0xa8/0xd4)
[<8039d7fc>] (dump_stack) from [<80177ee0>] (print_circular_bug+0x214/0x334)
[<80177ee0>] (print_circular_bug) from [<80179230>] (check_prevs_add+0x4dc/0x8e8)
[<80179230>] (check_prevs_add) from [<8017b3d8>] (__lock_acquire+0xc6c/0x14ec)
[<8017b3d8>] (__lock_acquire) from [<8017c4e8>] (lock_acquire+0x70/0x90)
[<8017c4e8>] (lock_acquire) from [<8014169c>] (flush_work+0x4c/0x280)
[<8014169c>] (flush_work) from [<801440f8>] (__cancel_work_timer+0x9c/0x1e0)
[<801440f8>] (__cancel_work_timer) from [<804acfcc>] (watchdog_release+0x3c/0x190)
[<804acfcc>] (watchdog_release) from [<8022c5e8>] (__fput+0x80/0x1c8)
[<8022c5e8>] (__fput) from [<80147b28>] (task_work_run+0x94/0xc8)
[<80147b28>] (task_work_run) from [<8010b998>] (do_work_pending+0x8c/0xb4)
[<8010b998>] (do_work_pending) from [<80107ba8>] (slow_work_pending+0xc/0x20)
Turns out the call to cancel_delayed_work_sync() in watchdog_release() is not necessary and can be dropped. If the worker is no longer necessary, the subsequent call to watchdog_update_worker() will cancel it. If it is already running, it won't do anything, since the worker function checks if it needs to ping the watchdog or not.
Reported-by Clemens Gruber <[email protected]>
Tested-by Clemens Gruber <[email protected]>
Fixes 11d7aba9ceb7 ("watchdog: imx2: Convert to use infrastructure triggered keepalives")
Signed-off-by Guenter Roeck <[email protected]>
---
 drivers/watchdog/watchdog_dev.c | 1 -
 1 file changed, 1 deletion(-)
drivers/watchdog/watchdog_dev.c +0 -1
index e2c5abbb45ff..3595cffa24ea 100644
@@ -736,7 +736,6 @@ static int watchdog_release(struct inode *inode, struct file *file)
 		watchdog_ping(wdd);
 	}
 
-	cancel_delayed_work_sync(&wd_data->work);
 	watchdog_update_worker(wdd);
 
 	/* make sure that /dev/watchdog can be re-opened */
Signature · 1 line
2.5.0

This patch has been added to linux-watchdog-next.

Kind regards,
Wim.

Guenter Roeck

author maintainer

2016-05-14 17:07 UTC #6 lore raw

Hi Wim,

Quoted 4 lines · Wim Van Sebroeck

Hi Guenter,
Quoted 2 lines
lockdep reports the following circular locking dependency.

You are faster than me this time. I was just about to send you a pull request. Sorry for being late. The watchdog-next branch in my repository on kernel.org has all patches in my queue, in case you want to have a look.

I'll rebase my branch to yours and see if anything is missing, then send you a pull request on top of it if needed.

Quoted 3 lines

This patch has been added to linux-watchdog-next.

This patch should probably be tagged for v4.6.

Thanks,
Guenter

Guenter Roeck

author maintainer

2016-05-14 17:24 UTC #7 lore raw

Quoted 8 lines · Guenter Roeck

Hi Wim,
Quoted 4 lines · Wim Van Sebroeck
Hi Guenter,
Quoted 2 lines
lockdep reports the following circular locking dependency.

Quoted 7 lines

You are faster than me this time. I was just about to send you a pull request. Sorry for being late. The watchdog-next branch in my repository on kernel.org has all patches in my queue, in case you want to have a look.
I'll rebase my branch to yours and see if anything is missing, then send you a pull request on top of it if needed.

All there. Looks like you picked it up right after I updated the branch this morning.

Quoted 6 lines

Quoted 3 lines
This patch has been added to linux-watchdog-next.
This patch should probably be tagged for v4.6.

Turns out it has a Fixes: tag, so an additional tag should not be necessary.

Thanks,
Guenter

Wim Van Sebroeck

2016-05-14 18:29 UTC #8 lore raw

Hi Guenter,

Quoted 15 lines · Guenter Roeck

Quoted 14 lines
Hi Wim,
Quoted 4 lines · Wim Van Sebroeck
Hi Guenter,
Quoted 2 lines
lockdep reports the following circular locking dependency.
You are faster than me this time. I was just about to send you a pull request.
Sorry for being late. The watchdog-next branch in my repository on kernel.org
has all patches in my queue, in case you want to have a look.

Quoted 6 lines

Quoted 3 lines
I'll rebase my branch to yours and see if anything is missing, then send you a pull request on top of it if needed.
All there. Looks like you picked it up right after I updated the branch this morning.

Correct ;-)

Kind regards,
Wim.