BUG: ktpacpi_nvramd/446 still has locks held!

REGRESSION In review Maciej Rutecki 8 participants 22 messages Mar 4, 2013 – Mar 26, 2013

Subsystems (2)

ACPI x86 platform drivers

lore

Share & tools

lore

Maciej Rutecki

author

2013-03-04 19:55 UTC #0 lore raw

Last known good: 3.8.0
Bad version: 3.9-rc1

[    6.116492] =====================================
[    6.116614] [ BUG: ktpacpi_nvramd/446 still has locks held! ]
[    6.116737] 3.9.0-rc1 #1 Not tainted
[    6.116821] -------------------------------------
[    6.116900] 1 lock held by ktpacpi_nvramd/446:
[    6.116973]  #0:  (&hotkey_thread_mutex){+.+...}, at: [<ffffffffa024e87e>] 
hotkey_kthread+0x1f/0x354 [thinkpad_acpi]
[    6.117193]
[    6.117193] stack backtrace:
[    6.117268] Pid: 446, comm: ktpacpi_nvramd Not tainted 3.9.0-rc1 #1
[    6.117381] Call Trace:
[    6.117445]  [<ffffffff810877ea>] debug_check_no_locks_held+0x8f/0x93
[    6.117600]  [<ffffffff8107d8ac>] set_freezable+0x3e/0x64
[    6.117703] input: ThinkPad Extra Buttons as 
/devices/platform/thinkpad_acpi/input/input5
[    6.117918]  [<ffffffffa024e890>] hotkey_kthread+0x31/0x354 [thinkpad_acpi]
[    6.118088]  [<ffffffffa024e85f>] ? issue_volchange.29885+0x54/0x54 
[thinkpad_acpi]
[    6.118250]  [<ffffffff8105e296>] kthread+0xac/0xb4
[    6.118356]  [<ffffffff8105e1ea>] ? __kthread_parkme+0x60/0x60
[    6.118491]  [<ffffffff814b243c>] ret_from_fork+0x7c/0xb0
[    6.118614]  [<ffffffff8105e1ea>] ? __kthread_parkme+0x60/0x60

Config:
http://mrutecki.pl/download/kernel/3.9.0-rc1/config-3.9.0-rc1

full dmesg:
http://mrutecki.pl/download/kernel/3.9.0-rc1/dmesg-3.9.0-rc1.txt

Signature · 2 lines

Maciej Rutecki
http://www.mrutecki.pl

Aaron Lu

2013-03-05 07:27 UTC #1 lore raw

Quoted 26 lines · Maciej Rutecki

Last known good: 3.8.0
Bad version: 3.9-rc1

[    6.116492] =====================================
[    6.116614] [ BUG: ktpacpi_nvramd/446 still has locks held! ]
[    6.116737] 3.9.0-rc1 #1 Not tainted
[    6.116821] -------------------------------------
[    6.116900] 1 lock held by ktpacpi_nvramd/446:
[    6.116973]  #0:  (&hotkey_thread_mutex){+.+...}, at: [<ffffffffa024e87e>] 
hotkey_kthread+0x1f/0x354 [thinkpad_acpi]
[    6.117193]
[    6.117193] stack backtrace:
[    6.117268] Pid: 446, comm: ktpacpi_nvramd Not tainted 3.9.0-rc1 #1
[    6.117381] Call Trace:
[    6.117445]  [<ffffffff810877ea>] debug_check_no_locks_held+0x8f/0x93
[    6.117600]  [<ffffffff8107d8ac>] set_freezable+0x3e/0x64
[    6.117703] input: ThinkPad Extra Buttons as 
/devices/platform/thinkpad_acpi/input/input5
[    6.117918]  [<ffffffffa024e890>] hotkey_kthread+0x31/0x354 [thinkpad_acpi]
[    6.118088]  [<ffffffffa024e85f>] ? issue_volchange.29885+0x54/0x54 
[thinkpad_acpi]
[    6.118250]  [<ffffffff8105e296>] kthread+0xac/0xb4
[    6.118356]  [<ffffffff8105e1ea>] ? __kthread_parkme+0x60/0x60
[    6.118491]  [<ffffffff814b243c>] ret_from_fork+0x7c/0xb0
[    6.118614]  [<ffffffff8105e1ea>] ? __kthread_parkme+0x60/0x60

Quoted 6 lines

Config:
http://mrutecki.pl/download/kernel/3.9.0-rc1/config-3.9.0-rc1
full dmesg:
http://mrutecki.pl/download/kernel/3.9.0-rc1/dmesg-3.9.0-rc1.txt

Thanks for the report!

Looks like the following commit is related:
commit 6aa9707099c4b25700940eb3d016f16c4434360d
Author: Mandeep Singh Baines <[email protected]> Thu Feb 28 09:03:18 2013

lockdep: check that no locks held at freeze time

And the code to trigger this problem is here:
static int hotkey_kthread(void *data)

{
	struct tp_nvram_state s[2];
	u32 poll_mask, event_mask;
	unsigned int si, so;
	unsigned long t;
	unsigned int change_detector;
	unsigned int poll_freq;
	bool was_frozen;

	mutex_lock(&hotkey_thread_mutex);

	if (tpacpi_lifecycle == TPACPI_LIFE_EXITING)
		goto exit;

	set_freezable();

~~~~~~~~~~~~~~~~~~~~~~~~~~
in thinkpad_acpi.c.

I don't know much about freeze, I've no idea what is the problem. So Mandeep and Henrique, can you please kindly take a look? Thanks.

-Aaron

Mandeep Singh Baines

2013-03-05 16:04 UTC #2 lore raw

Quoted 59 lines · Aaron Lu

Quoted 26 lines · Maciej Rutecki

Last known good: 3.8.0
Bad version: 3.9-rc1

[    6.116492] =====================================
[    6.116614] [ BUG: ktpacpi_nvramd/446 still has locks held! ]
[    6.116737] 3.9.0-rc1 #1 Not tainted
[    6.116821] -------------------------------------
[    6.116900] 1 lock held by ktpacpi_nvramd/446:
[    6.116973]  #0:  (&hotkey_thread_mutex){+.+...}, at: [<ffffffffa024e87e>]
hotkey_kthread+0x1f/0x354 [thinkpad_acpi]
[    6.117193]
[    6.117193] stack backtrace:
[    6.117268] Pid: 446, comm: ktpacpi_nvramd Not tainted 3.9.0-rc1 #1
[    6.117381] Call Trace:
[    6.117445]  [<ffffffff810877ea>] debug_check_no_locks_held+0x8f/0x93
[    6.117600]  [<ffffffff8107d8ac>] set_freezable+0x3e/0x64
[    6.117703] input: ThinkPad Extra Buttons as
/devices/platform/thinkpad_acpi/input/input5
[    6.117918]  [<ffffffffa024e890>] hotkey_kthread+0x31/0x354 [thinkpad_acpi]
[    6.118088]  [<ffffffffa024e85f>] ? issue_volchange.29885+0x54/0x54
[thinkpad_acpi]
[    6.118250]  [<ffffffff8105e296>] kthread+0xac/0xb4
[    6.118356]  [<ffffffff8105e1ea>] ? __kthread_parkme+0x60/0x60
[    6.118491]  [<ffffffff814b243c>] ret_from_fork+0x7c/0xb0
[    6.118614]  [<ffffffff8105e1ea>] ? __kthread_parkme+0x60/0x60

Quoted 6 lines

Config:
http://mrutecki.pl/download/kernel/3.9.0-rc1/config-3.9.0-rc1
full dmesg:
http://mrutecki.pl/download/kernel/3.9.0-rc1/dmesg-3.9.0-rc1.txt

Thanks for the report!

Looks like the following commit is related:
commit 6aa9707099c4b25700940eb3d016f16c4434360d
Author: Mandeep Singh Baines <[email protected]> Thu Feb 28 09:03:18 2013

lockdep: check that no locks held at freeze time

And the code to trigger this problem is here:
static int hotkey_kthread(void *data)

{
        struct tp_nvram_state s[2];
        u32 poll_mask, event_mask;
        unsigned int si, so;
        unsigned long t;
        unsigned int change_detector;
        unsigned int poll_freq;
        bool was_frozen;

        mutex_lock(&hotkey_thread_mutex);

        if (tpacpi_lifecycle == TPACPI_LIFE_EXITING)
                goto exit;

        set_freezable();

Quoted 6 lines

~~~~~~~~~~~~~~~~~~~~~~~~~~
in thinkpad_acpi.c.
I don't know much about freeze, I've no idea what is the problem. So Mandeep and Henrique, can you please kindly take a look? Thanks.

Hi Aaron,

I will take a look at this.

In general, you don't want to grab a lock and freeze. You will block suspend because another thread may try go grab the lock. In the case of cgroup freezer, you could cause a deadlock.

There is also discussion happening on this thread:

https://lkml.org/lkml/2013/3/4/221

Regards,
Mandeep

Quoted 2 lines

-Aaron

Mandeep Singh Baines

2013-03-05 17:34 UTC #3 lore raw

PATCH 1/1 thinkpad-acpi: fix potential suspend blocking issue

In reply to Maciej Rutecki

Fixes the following lockdep error:

[ BUG: ktpacpi_nvramd/446 still has locks held! ]

hotkey_kthread() calls set_freezable() after acquiring the hotkey_kthread_mutex(). set_freezable() calls try_to_freeze(). This could block suspend if we were to freeze at this point and another task were to block on the mutex, potentially via writing to one of the sysfs attrs. This race is unlikely but can be easily fixed by moving the set_freezable() call.

Reported-by Maciej Rutecki <[email protected]>

Signed-off-by Mandeep Singh Baines <[email protected]>

CC Aaron Lu <[email protected]>

CC Henrique de Moraes Holschuh <[email protected]>

CC Tejun Heo <[email protected]>

CC Oleg Nesterov <[email protected]>

CC Andrew Morton <[email protected]>

---
 drivers/platform/x86/thinkpad_acpi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/platform/x86/thinkpad_acpi.c +2 -2

index 9a90756..69870a841 100644
@@ -2462,13 +2462,13 @@ static int hotkey_kthread(void *data)
 	unsigned int poll_freq;
 	bool was_frozen;
 
+	set_freezable();
+
 	mutex_lock(&hotkey_thread_mutex);
 
 	if (tpacpi_lifecycle == TPACPI_LIFE_EXITING)
 		goto exit;
 
-	set_freezable();
-
 	so = 0;
 	si = 1;
 	t = 0;

Signature · 1 line

1.7.12.4

Oleg Nesterov

2013-03-05 17:48 UTC #4 lore raw

Re: [PATCH] thinkpad-acpi: fix potential suspend blocking issue

Quoted 4 lines · Mandeep Singh Baines

drivers/platform/x86/thinkpad_acpi.c +0 -0

@@ -2462,13 +2462,13 @@ static int hotkey_kthread(void *data)
 	unsigned int poll_freq;
 	bool was_frozen;

drivers/platform/x86/thinkpad_acpi.c static int hotkey_kthread(void *data) · line 2471

Quoted 10 lines

drivers/platform/x86/thinkpad_acpi.c +2 -2

 
+	set_freezable();
+
 	mutex_lock(&hotkey_thread_mutex);
 
 	if (tpacpi_lifecycle == TPACPI_LIFE_EXITING)
 		goto exit;
 
-	set_freezable();
-

I don't understand this code... but don't we have the same problem with kthread_freezable_should_stop() below? It can call __refrigerator() too under the same lock.

Oleg.

Mandeep Singh Baines

2013-03-05 17:59 UTC #5 lore raw

Re: [PATCH] thinkpad-acpi: fix potential suspend blocking issue

Quoted 13 lines · Oleg Nesterov

Quoted 12 lines · Mandeep Singh Baines

drivers/platform/x86/thinkpad_acpi.c +2 -0

@@ -2462,13 +2462,13 @@ static int hotkey_kthread(void *data)
      unsigned int poll_freq;
      bool was_frozen;
 
+     set_freezable();
+
      mutex_lock(&hotkey_thread_mutex);
 
      if (tpacpi_lifecycle == TPACPI_LIFE_EXITING)
              goto exit;

Quoted 7 lines

drivers/platform/x86/thinkpad_acpi.c static int hotkey_kthread(void *data) · line 2471
Quoted 2 lines
drivers/platform/x86/thinkpad_acpi.c +0 -2
-     set_freezable();
-
I don't understand this code... but don't we have the same problem with kthread_freezable_should_stop() below? It can call __refrigerator() too under the same lock.

I don't think the lock is held at that point. There is an unlock right before entering the while loop and at the bottom of the loop.

Quoted 2 lines

Oleg.

Oleg Nesterov

2013-03-05 18:05 UTC #6 lore raw

Re: [PATCH] thinkpad-acpi: fix potential suspend blocking issue

Quoted 23 lines · Mandeep Singh Baines

Quoted 13 lines · Oleg Nesterov
Quoted 12 lines · Mandeep Singh Baines
drivers/platform/x86/thinkpad_acpi.c +2 -0
@@ -2462,13 +2462,13 @@ static int hotkey_kthread(void *data)
      unsigned int poll_freq;
      bool was_frozen;
 
+     set_freezable();
+
      mutex_lock(&hotkey_thread_mutex);
 
      if (tpacpi_lifecycle == TPACPI_LIFE_EXITING)
              goto exit;
 
Quoted 7 lines
drivers/platform/x86/thinkpad_acpi.c static int hotkey_kthread(void *data) · line 2471
Quoted 2 lines
drivers/platform/x86/thinkpad_acpi.c +0 -2
-     set_freezable();
-
I don't understand this code... but don't we have the same problem with kthread_freezable_should_stop() below? It can call __refrigerator() too under the same lock.

Quoted 2 lines

I don't think the lock is held at that point. There is an unlock right before entering the while loop and at the bottom of the loop.

Hmm... Afaics this is another lock, hotkey_thread_data_mutex. But hotkey_thread_mutex is still held.

Oleg.

Mandeep Singh Baines

2013-03-05 20:55 UTC #7 lore raw

Re: [PATCH] thinkpad-acpi: fix potential suspend blocking issue

Quoted 24 lines · Oleg Nesterov

Quoted 23 lines · Mandeep Singh Baines
Quoted 20 lines · Oleg Nesterov
Quoted 4 lines · Mandeep Singh Baines
drivers/platform/x86/thinkpad_acpi.c +0 -0
@@ -2462,13 +2462,13 @@ static int hotkey_kthread(void *data)
      unsigned int poll_freq;
      bool was_frozen;
drivers/platform/x86/thinkpad_acpi.c static int hotkey_kthread(void *data) · line 2471
Quoted 10 lines
drivers/platform/x86/thinkpad_acpi.c +2 -2
 
+     set_freezable();
+
      mutex_lock(&hotkey_thread_mutex);
 
      if (tpacpi_lifecycle == TPACPI_LIFE_EXITING)
              goto exit;
 
-     set_freezable();
-
I don't understand this code... but don't we have the same problem with kthread_freezable_should_stop() below? It can call __refrigerator() too under the same lock.

Quoted 6 lines

Quoted 2 lines
I don't think the lock is held at that point. There is an unlock right before entering the while loop and at the bottom of the loop.
Hmm... Afaics this is another lock, hotkey_thread_data_mutex. But hotkey_thread_mutex is still held.

Ah. You're right. The two names were similar so that confused me. I'm also looking at this code for the first time:)

This mutex seems wrong. Its held the entire time the kthread is running. I think its used to synchronize on the exit of the kthread. A completion would more appropriate in that case.

Regards,
Mandeep

Quoted 2 lines

Oleg.

Henrique de Moraes Holschuh

2013-03-05 23:26 UTC #8 lore raw

Re: [PATCH] thinkpad-acpi: fix potential suspend blocking issue

Quoted 5 lines · Mandeep Singh Baines

This mutex seems wrong. Its held the entire time the kthread is running. I think its used to synchronize on the exit of the kthread. A completion would more appropriate in that case.
From the top of the driver source:

/* Acquired while the poller kthread is running, use to sync start/stop */ static struct mutex hotkey_thread_mutex;

/*
* Acquire mutex to write poller control variables as an
* atomic block.
*
* Increment hotkey_config_change when changing them if you
* want the kthread to forget old state.
*
* See HOTKEY_CONFIG_CRITICAL_START/HOTKEY_CONFIG_CRITICAL_END */
static struct mutex hotkey_thread_data_mutex;
static unsigned int hotkey_config_change;

#define HOTKEY_CONFIG_CRITICAL_START \
        do { \
                mutex_lock(&hotkey_thread_data_mutex); \
                hotkey_config_change++; \
        } while (0);
#define HOTKEY_CONFIG_CRITICAL_END \
        mutex_unlock(&hotkey_thread_data_mutex);

This can likely be modernized a lot. This code is from 2008, I think it first shipped in 2.6.25-rc1.

Signature · 4 lines

  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Oleg Nesterov

2013-03-06 15:44 UTC #9 lore raw

Re: [PATCH] thinkpad-acpi: fix potential suspend blocking issue

Quoted 10 lines · Henrique de Moraes Holschuh

Quoted 3 lines · Mandeep Singh Baines
This mutex seems wrong. Its held the entire time the kthread is running. I think its used to synchronize on the exit of the kthread. A completion would more appropriate in that case.
From the top of the driver source:
/* Acquired while the poller kthread is running, use to sync start/stop */ static struct mutex hotkey_thread_mutex;

I simply can't understand what this "sync start/stop" means...

Ignoring hotkey_kthread(), the only user is

	static void hotkey_poll_stop_sync(void)
	{
		if (tpacpi_hotkey_task) {
			kthread_stop(tpacpi_hotkey_task);
			tpacpi_hotkey_task = NULL;
			mutex_lock(&hotkey_thread_mutex);
			/* at this point, the thread did exit */
			mutex_unlock(&hotkey_thread_mutex);
		}
	}

And I simply do not understand the comment. This thread has already exited when kthread_stop() returns (OK, it can be running do_exit() paths but this doesn't matter). So this mutex_lock() buys nothing afaics.

As for serializing with hotkey_poll_setup/etc, looks like this code relies on hotkey_mutex.

So I think hotkey_thread_mutex can be simply removed?

Oleg.

Henrique de Moraes Holschuh

2013-03-06 23:32 UTC #10 lore raw

Re: [PATCH] thinkpad-acpi: fix potential suspend blocking issue

Quoted 26 lines · Oleg Nesterov

Quoted 9 lines · Henrique de Moraes Holschuh
Quoted 3 lines · Mandeep Singh Baines
This mutex seems wrong. Its held the entire time the kthread is running. I think its used to synchronize on the exit of the kthread. A completion would more appropriate in that case.
From the top of the driver source:
/* Acquired while the poller kthread is running, use to sync start/stop */ static struct mutex hotkey_thread_mutex;
I simply can't understand what this "sync start/stop" means...
Ignoring hotkey_kthread(), the only user is
	static void hotkey_poll_stop_sync(void)
	{
		if (tpacpi_hotkey_task) {
			kthread_stop(tpacpi_hotkey_task);
			tpacpi_hotkey_task = NULL;
			mutex_lock(&hotkey_thread_mutex);
			/* at this point, the thread did exit */
			mutex_unlock(&hotkey_thread_mutex);
		}
	}

Quoted 3 lines

And I simply do not understand the comment. This thread has already exited when kthread_stop() returns (OK, it can be running do_exit() paths but this doesn't matter). So this mutex_lock() buys nothing afaics.

It was added due to an oops, waaaaay back then. If it is not needed anymore, and there is zero chance of the kthread still being active when hotkey_poll_stop_sync() ends, hotkey_thread_mutex can be simply removed.

Note that hotkey_thread_data_mutex is still required.

Quoted 4 lines

As for serializing with hotkey_poll_setup/etc, looks like this code relies on hotkey_mutex.
So I think hotkey_thread_mutex can be simply removed?

Looks like it, if the current semanthics of ktread_stop() are syncronous.

Signature · 4 lines

  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Oleg Nesterov

2013-03-07 17:53 UTC #11 lore raw

cover thinkpad-acpi: kill hotkey_thread_mutex

Quoted 14 lines · Henrique de Moraes Holschuh

Quoted 12 lines · Oleg Nesterov

	static void hotkey_poll_stop_sync(void)
	{
		if (tpacpi_hotkey_task) {
			kthread_stop(tpacpi_hotkey_task);
			tpacpi_hotkey_task = NULL;
			mutex_lock(&hotkey_thread_mutex);
			/* at this point, the thread did exit */
			mutex_unlock(&hotkey_thread_mutex);
		}
	}

Quoted 7 lines

Quoted 3 lines
And I simply do not understand the comment. This thread has already exited when kthread_stop() returns (OK, it can be running do_exit() paths but this doesn't matter). So this mutex_lock() buys nothing afaics.
It was added due to an oops, waaaaay back then. If it is not needed anymore, and there is zero chance of the kthread still being active when hotkey_poll_stop_sync() ends, hotkey_thread_mutex can be simply removed.

Well, there could be another bug. Say, hotkey_poll_stop_sync() can block on hotkey_thread_mutex if another thread was started. But at first glance this can't happen (hotkey_mutex), and even _if_ it can this needs another fix.

Quoted 1 line

Looks like it, if the current semanthics of ktread_stop() are syncronous.

IIRC, it always was... But at least currently it is certainly syncronous. kthread_stop(t) does wait_for_completion(t->vfork_done), complete(vfork_done) can't happen unless this task calls do_exit().

Hmm. I just noticed that the recent changes in kthread_stop() are not correct... But this is offtopic and doesn't affect thinkpad_acpi.c, I'll write another email later.

So, what do you think about (UNTESTED) 1/1 ?

Oleg.

------------------------------------------------------------------------------
Symantec Endpoint Protection 12 positioned as A LEADER in The Forrester Wave(TM): Endpoint Security, Q1 2013 and "remains a good choice" in the endpoint security space. For insight on selecting the right partner to tackle endpoint security challenges, access the full report.
http://p.sf.net/sfu/symantec-dev2dev

Oleg Nesterov

2013-03-07 17:53 UTC #12 lore raw

PATCH 1/1 thinkpad-acpi: kill hotkey_thread_mutex

hotkey_kthread() does try_to_freeze() under hotkey_thread_mutex.

We can simply kill this mutex, hotkey_poll_stop_sync() does not need to serialize with hotkey_kthread(). When kthread_stop() returns the thread is already dead, it called do_exit()->complete_vfork_done().

Reported-by Artem Savkov <[email protected]>

Reported-by Maciej Rutecki <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

drivers/platform/x86/thinkpad_acpi.c +0 ~~-10~~

@@ -1965,9 +1965,6 @@ struct tp_nvram_state {
 /* kthread for the hotkey poller */
 static struct task_struct *tpacpi_hotkey_task;
 
-/* Acquired while the poller kthread is running, use to sync start/stop */
-static struct mutex hotkey_thread_mutex;
-
 /*
  * Acquire mutex to write poller control variables as an
  * atomic block.

@@ -2463,8 +2460,6 @@ static int hotkey_kthread(void *data)
 	unsigned int poll_freq;
 	bool was_frozen;
 
-	mutex_lock(&hotkey_thread_mutex);
-
 	if (tpacpi_lifecycle == TPACPI_LIFE_EXITING)
 		goto exit;

@@ -2524,7 +2519,6 @@ static int hotkey_kthread(void *data)
 	}
 
 exit:
-	mutex_unlock(&hotkey_thread_mutex);
 	return 0;
 }

@@ -2534,9 +2528,6 @@ static void hotkey_poll_stop_sync(void)
 	if (tpacpi_hotkey_task) {
 		kthread_stop(tpacpi_hotkey_task);
 		tpacpi_hotkey_task = NULL;
-		mutex_lock(&hotkey_thread_mutex);
-		/* at this point, the thread did exit */
-		mutex_unlock(&hotkey_thread_mutex);
 	}
 }

@@ -3235,7 +3226,6 @@ static int __init hotkey_init(struct ibm
 	mutex_init(&hotkey_mutex);
 
 #ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
-	mutex_init(&hotkey_thread_mutex);
 	mutex_init(&hotkey_thread_data_mutex);
 #endif

Mandeep Singh Baines

Reviewed-by on v1 1/1

2013-03-07 18:42 UTC #13 lore raw

Re: [PATCH 1/1] thinkpad-acpi: kill hotkey_thread_mutex

Quoted 10 lines · Oleg Nesterov

hotkey_kthread() does try_to_freeze() under hotkey_thread_mutex.
We can simply kill this mutex, hotkey_poll_stop_sync() does not need to serialize with hotkey_kthread(). When kthread_stop() returns the thread is already dead, it called do_exit()->complete_vfork_done().
Reported-by Artem Savkov <[email protected]>
Reported-by Maciej Rutecki <[email protected]>
Signed-off-by Oleg Nesterov <[email protected]>

Reviewed-by Mandeep Singh Baines <[email protected]>

Quoted 48 lines

drivers/platform/x86/thinkpad_acpi.c +0 ~~-10~~

@@ -1965,9 +1965,6 @@ struct tp_nvram_state {
 /* kthread for the hotkey poller */
 static struct task_struct *tpacpi_hotkey_task;
 
-/* Acquired while the poller kthread is running, use to sync start/stop */
-static struct mutex hotkey_thread_mutex;
-
 /*
  * Acquire mutex to write poller control variables as an
  * atomic block.

@@ -2463,8 +2460,6 @@ static int hotkey_kthread(void *data)
        unsigned int poll_freq;
        bool was_frozen;
 
-       mutex_lock(&hotkey_thread_mutex);
-
        if (tpacpi_lifecycle == TPACPI_LIFE_EXITING)
                goto exit;

@@ -2524,7 +2519,6 @@ static int hotkey_kthread(void *data)
        }
 
 exit:
-       mutex_unlock(&hotkey_thread_mutex);
        return 0;
 }

@@ -2534,9 +2528,6 @@ static void hotkey_poll_stop_sync(void)
        if (tpacpi_hotkey_task) {
                kthread_stop(tpacpi_hotkey_task);
                tpacpi_hotkey_task = NULL;
-               mutex_lock(&hotkey_thread_mutex);
-               /* at this point, the thread did exit */
-               mutex_unlock(&hotkey_thread_mutex);
        }
 }

@@ -3235,7 +3226,6 @@ static int __init hotkey_init(struct ibm
        mutex_init(&hotkey_mutex);
 
 #ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
-       mutex_init(&hotkey_thread_mutex);
        mutex_init(&hotkey_thread_data_mutex);
 #endif

Henrique de Moraes Holschuh

Acked-by

2013-03-09 21:34 UTC #14 lore raw

Re: [PATCH 1/1] thinkpad-acpi: kill hotkey_thread_mutex

Quoted 7 lines · Mandeep Singh Baines

Quoted 6 lines · Oleg Nesterov
hotkey_kthread() does try_to_freeze() under hotkey_thread_mutex.
We can simply kill this mutex, hotkey_poll_stop_sync() does not need to serialize with hotkey_kthread(). When kthread_stop() returns the thread is already dead, it called do_exit()->complete_vfork_done().

Quoted 6 lines

Quoted 4 lines
Reported-by Artem Savkov <[email protected]>
Reported-by Maciej Rutecki <[email protected]>
Signed-off-by Oleg Nesterov <[email protected]>
Reviewed-by Mandeep Singh Baines <[email protected]>

Acked-by Henrique de Moraes Holschuh <hmh-N3TV7GIv+o9fyO9Q7EP/[email protected]>

Signature · 11 lines

  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

------------------------------------------------------------------------------
Symantec Endpoint Protection 12 positioned as A LEADER in The Forrester  
Wave(TM): Endpoint Security, Q1 2013 and "remains a good choice" in the  
endpoint security space. For insight on selecting the right partner to 
tackle endpoint security challenges, access the full report. 
http://p.sf.net/sfu/symantec-dev2dev

Rafael J. Wysocki

2013-03-26 13:06 UTC #15 lore raw

Re: [PATCH 1/1] thinkpad-acpi: kill hotkey_thread_mutex

In reply to Oleg Nesterov

Quoted 9 lines · Oleg Nesterov

hotkey_kthread() does try_to_freeze() under hotkey_thread_mutex.
We can simply kill this mutex, hotkey_poll_stop_sync() does not need to serialize with hotkey_kthread(). When kthread_stop() returns the thread is already dead, it called do_exit()->complete_vfork_done().
Reported-by Artem Savkov <[email protected]>
Reported-by Maciej Rutecki <[email protected]>
Signed-off-by Oleg Nesterov <[email protected]>

I wonder if anyone is taking care of this patch?

Rafael

Quoted 52 lines

drivers/platform/x86/thinkpad_acpi.c +0 ~~-11~~

@@ -1965,9 +1965,6 @@ struct tp_nvram_state {
 /* kthread for the hotkey poller */
 static struct task_struct *tpacpi_hotkey_task;
 
-/* Acquired while the poller kthread is running, use to sync start/stop */
-static struct mutex hotkey_thread_mutex;
-
 /*
  * Acquire mutex to write poller control variables as an
  * atomic block.

@@ -2463,8 +2460,6 @@ static int hotkey_kthread(void *data)
 	unsigned int poll_freq;
 	bool was_frozen;
 
-	mutex_lock(&hotkey_thread_mutex);
-
 	if (tpacpi_lifecycle == TPACPI_LIFE_EXITING)
 		goto exit;

@@ -2524,7 +2519,6 @@ static int hotkey_kthread(void *data)
 	}
 
 exit:
-	mutex_unlock(&hotkey_thread_mutex);
 	return 0;
 }

@@ -2534,9 +2528,6 @@ static void hotkey_poll_stop_sync(void)
 	if (tpacpi_hotkey_task) {
 		kthread_stop(tpacpi_hotkey_task);
 		tpacpi_hotkey_task = NULL;
-		mutex_lock(&hotkey_thread_mutex);
-		/* at this point, the thread did exit */
-		mutex_unlock(&hotkey_thread_mutex);
 	}
 }

@@ -3235,7 +3226,6 @@ static int __init hotkey_init(struct ibm
 	mutex_init(&hotkey_mutex);
 
 #ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
-	mutex_init(&hotkey_thread_mutex);
 	mutex_init(&hotkey_thread_data_mutex);
 #endif
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 2 lines

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Oleg Nesterov

2013-03-26 13:09 UTC #16 lore raw

Re: [PATCH 1/1] thinkpad-acpi: kill hotkey_thread_mutex

Quoted 8 lines · Rafael J. Wysocki

Quoted 6 lines · Oleg Nesterov
hotkey_kthread() does try_to_freeze() under hotkey_thread_mutex.
We can simply kill this mutex, hotkey_poll_stop_sync() does not need to serialize with hotkey_kthread(). When kthread_stop() returns the thread is already dead, it called do_exit()->complete_vfork_done().

Quoted 5 lines

Quoted 3 lines
Reported-by Artem Savkov <[email protected]>
Reported-by Maciej Rutecki <[email protected]>
Signed-off-by Oleg Nesterov <[email protected]>
I wonder if anyone is taking care of this patch?

IIRC, it is already in -mm tree.

Quoted 58 lines

Rafael

Quoted 52 lines

drivers/platform/x86/thinkpad_acpi.c +0 ~~-11~~

@@ -1965,9 +1965,6 @@ struct tp_nvram_state {
 /* kthread for the hotkey poller */
 static struct task_struct *tpacpi_hotkey_task;
 
-/* Acquired while the poller kthread is running, use to sync start/stop */
-static struct mutex hotkey_thread_mutex;
-
 /*
  * Acquire mutex to write poller control variables as an
  * atomic block.

@@ -2463,8 +2460,6 @@ static int hotkey_kthread(void *data)
 	unsigned int poll_freq;
 	bool was_frozen;
 
-	mutex_lock(&hotkey_thread_mutex);
-
 	if (tpacpi_lifecycle == TPACPI_LIFE_EXITING)
 		goto exit;

@@ -2524,7 +2519,6 @@ static int hotkey_kthread(void *data)
 	}
 
 exit:
-	mutex_unlock(&hotkey_thread_mutex);
 	return 0;
 }

@@ -2534,9 +2528,6 @@ static void hotkey_poll_stop_sync(void)
 	if (tpacpi_hotkey_task) {
 		kthread_stop(tpacpi_hotkey_task);
 		tpacpi_hotkey_task = NULL;
-		mutex_lock(&hotkey_thread_mutex);
-		/* at this point, the thread did exit */
-		mutex_unlock(&hotkey_thread_mutex);
 	}
 }

@@ -3235,7 +3226,6 @@ static int __init hotkey_init(struct ibm
 	mutex_init(&hotkey_mutex);
 
 #ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
-	mutex_init(&hotkey_thread_mutex);
 	mutex_init(&hotkey_thread_data_mutex);
 #endif
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 2 lines

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Rafael J. Wysocki

2013-03-26 13:43 UTC #17 lore raw

Re: [PATCH 1/1] thinkpad-acpi: kill hotkey_thread_mutex

Quoted 9 lines · Oleg Nesterov

Quoted 8 lines · Rafael J. Wysocki
Quoted 6 lines · Oleg Nesterov
hotkey_kthread() does try_to_freeze() under hotkey_thread_mutex.
We can simply kill this mutex, hotkey_poll_stop_sync() does not need to serialize with hotkey_kthread(). When kthread_stop() returns the thread is already dead, it called do_exit()->complete_vfork_done().

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
Reported-by Artem Savkov <[email protected]>
Reported-by Maciej Rutecki <[email protected]>
Signed-off-by Oleg Nesterov <[email protected]>
I wonder if anyone is taking care of this patch?
IIRC, it is already in -mm tree.

OK, thanks for letting me know.

Quoted 53 lines

Quoted 52 lines

drivers/platform/x86/thinkpad_acpi.c +0 ~~-11~~

@@ -1965,9 +1965,6 @@ struct tp_nvram_state {
 /* kthread for the hotkey poller */
 static struct task_struct *tpacpi_hotkey_task;
 
-/* Acquired while the poller kthread is running, use to sync start/stop */
-static struct mutex hotkey_thread_mutex;
-
 /*
  * Acquire mutex to write poller control variables as an
  * atomic block.

@@ -2463,8 +2460,6 @@ static int hotkey_kthread(void *data)
 	unsigned int poll_freq;
 	bool was_frozen;
 
-	mutex_lock(&hotkey_thread_mutex);
-
 	if (tpacpi_lifecycle == TPACPI_LIFE_EXITING)
 		goto exit;

@@ -2524,7 +2519,6 @@ static int hotkey_kthread(void *data)
 	}
 
 exit:
-	mutex_unlock(&hotkey_thread_mutex);
 	return 0;
 }

@@ -2534,9 +2528,6 @@ static void hotkey_poll_stop_sync(void)
 	if (tpacpi_hotkey_task) {
 		kthread_stop(tpacpi_hotkey_task);
 		tpacpi_hotkey_task = NULL;
-		mutex_lock(&hotkey_thread_mutex);
-		/* at this point, the thread did exit */
-		mutex_unlock(&hotkey_thread_mutex);
 	}
 }

@@ -3235,7 +3226,6 @@ static int __init hotkey_init(struct ibm
 	mutex_init(&hotkey_mutex);
 
 #ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
-	mutex_init(&hotkey_thread_mutex);
 	mutex_init(&hotkey_thread_data_mutex);
 #endif
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 2 lines

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Maciej Rutecki

author Tested-by on v1 1/1

2013-03-05 19:18 UTC #18 lore raw

Re: [PATCH] thinkpad-acpi: fix potential suspend blocking issue

In reply to Mandeep Singh Baines

Quoted 11 lines · On wtorek, 5 marca 2013 o 18:34:43 Mandeep Singh Baines wrote:

Fixes the following lockdep error:
[ BUG: ktpacpi_nvramd/446 still has locks held! ]
hotkey_kthread() calls set_freezable() after acquiring the hotkey_kthread_mutex(). set_freezable() calls try_to_freeze(). This could block suspend if we were to freeze at this point and another task were to block on the mutex, potentially via writing to one of the sysfs attrs. This race is unlikely but can be easily fixed by moving the set_freezable() call.

Quoted 7 lines

Reported-by Maciej Rutecki <[email protected]>
Signed-off-by Mandeep Singh Baines <[email protected]>
CC Aaron Lu <[email protected]>
CC Henrique de Moraes Holschuh <[email protected]>
CC Tejun Heo <[email protected]>
CC Oleg Nesterov <[email protected]>
CC Andrew Morton <[email protected]>

Tested-by Maciej Rutecki <[email protected]>

Patch fixes the problem. Thanks!

Regards

Signature · 2 lines

Maciej Rutecki
http://www.mrutecki.pl

Andrew Morton

2013-03-05 22:18 UTC #19 lore raw

Re: [PATCH] thinkpad-acpi: fix potential suspend blocking issue

In reply to Mandeep Singh Baines

Quoted 21 lines · On Tue, 5 Mar 2013 09:34:43 -0800 Mandeep Singh Baines <[email protected]>…

Fixes the following lockdep error:
[ BUG: ktpacpi_nvramd/446 still has locks held! ]
hotkey_kthread() calls set_freezable() after acquiring the hotkey_kthread_mutex(). set_freezable() calls try_to_freeze(). This could block suspend if we were to freeze at this point and another task were to block on the mutex, potentially via writing to one of the sysfs attrs. This race is unlikely but can be easily fixed by moving the set_freezable() call.
...
drivers/platform/x86/thinkpad_acpi.c +2 -0
@@ -2462,13 +2462,13 @@ static int hotkey_kthread(void *data)
 	unsigned int poll_freq;
 	bool was_frozen;
 
+	set_freezable();
+

drivers/platform/x86/thinkpad_acpi.c static int hotkey_kthread(void *data) · line 2474

Quoted 10 lines

drivers/platform/x86/thinkpad_acpi.c +0 -2

 	mutex_lock(&hotkey_thread_mutex);
 
 	if (tpacpi_lifecycle == TPACPI_LIFE_EXITING)
 		goto exit;
 
-	set_freezable();
-
 	so = 0;
 	si = 1;
 	t = 0;

Basically the same as
http://ozlabs.org/~akpm/mmots/broken-out/drivers-platform-x86-thinkpad_acpic-move-hotkey_thread_mutex-lock-after-set_freezable.patch.
I think Artem's patch is a little better. There doesn't appear to be
any locking protocol for tpacpi_lifecycle.

I'll move Artem's patch into my for-3.9-rc2 queue.

Oleg Nesterov

2013-03-06 15:50 UTC #20 lore raw

Re: [PATCH] thinkpad-acpi: fix potential suspend blocking issue

Quoted 5 lines · Andrew Morton

Basically the same as
http://ozlabs.org/~akpm/mmots/broken-out/drivers-platform-x86-thinkpad_acpic-move-hotkey_thread_mutex-lock-after-set_freezable.patch.
I think Artem's patch is a little better. There doesn't appear to be
any locking protocol for tpacpi_lifecycle.

Which seems to have the same problem, hotkey_kthread() still calls kthread_freezable_should_stop() under hotkey_thread_mutex.

IOW, we have two try_to_freeze's here, the patch moves only one of them outside of the hotkey_thread_mutex.

Oleg.

Artem Savkov

2013-03-06 20:18 UTC #21 lore raw

Re: [PATCH] thinkpad-acpi: fix potential suspend blocking issue

Quoted 7 lines · Oleg Nesterov

Quoted 5 lines · Andrew Morton
Basically the same as
http://ozlabs.org/~akpm/mmots/broken-out/drivers-platform-x86-thinkpad_acpic-move-hotkey_thread_mutex-lock-after-set_freezable.patch.
I think Artem's patch is a little better. There doesn't appear to be
any locking protocol for tpacpi_lifecycle.

Quoted 5 lines

Which seems to have the same problem, hotkey_kthread() still calls kthread_freezable_should_stop() under hotkey_thread_mutex.
IOW, we have two try_to_freeze's here, the patch moves only one of them outside of the hotkey_thread_mutex.

It's hard for me to judge but this lock does indeed look like it has been used to block until the thread exits. I'm trying out the "remove hotkey_thread_mutex completely" approach and everything looks fine so far.

Signature · 2 lines

Regards,
Artem