LinuxLists.cc - [PATCH 00/14] Fix bug about invalid wake up problem

2013-08-29 14:04:39

Subject: [PATCH 00/14] Fix bug about invalid wake up problem

1)Problem Description:
The prototype of invalid wakeup problem is as follows:
========================================================================
----------------------------
Consumer Thread
----------------------------
...
if (list_empty(&list)){
//location a
set_current_state(TASK_INTERRUPTIBLE);
schedule();
}
...
----------------------------
Producer Thread
----------------------------
...
list_add_tail(&item, &list);
wake_up_process(A);
...
========================================================================

Invalid wakeup problem occurs in preemptable kernel environment. The list
is empty initially, if consumer is preempted after condition validated at
location a, and at this time producer is woken up, adding a node to the
list and calling wake_up_process to wake up consumer. After that when
consumer being scheduled by scheduler, it calls set_current_state to set
itself as state TASK_INTERRUPTIBLE, and calling schedule() to request
scheduled. After consumer being scheduled out, it has no condition to be
woken up, and causing invalid wakeup problem.

In most cases, the kernel codes use a form similar to the following:
--------------------------------------------
...
//location a
...
set_current_state(TASK_INTERRUPTIBLE);
//location b
while (list_empty(&product_list)){
schedule(); //location c
set_current_state(TASK_INTERRUPTIBLE);
//location d
}
__set_current_state(TASK_RUNNING);
...
--------------------------------------------
At location a, consumer is preempted, and producer is scheduled,
adding item to product_list and waking up consumer. After consumer is
re-scheduled, calling set_current_state to set itself as state
TASK_INTERRUPTIBLE, if it is preempted again before __set_current_state(TASK_RUNNING),
also trigger the invalid wakeup problem that the consumer will not be scheduled
and the item be added by producer can't be consumed.

The following circumstance will also trigger this issue:
At location c, consumer is scheduled out, and be preempted after calling
set_current_state(TASK_INTERRUPTIBLE) when it be re-schdeuled.

2)Test:
I have written a test module to trigger the problem by adding some
synchronization condition. I will post it in the form of an attachment soon.

Test result as follows:
[103135.332683] wakeup_test: create two kernel threads - producer & consumer
[103135.332686] wakeup_test: module loaded successfully
[103135.332692] wakeup_test: kthread producer try to wake up the kthread consumer
[103165.299865] wakeup_test: kthread consumer have waited for 30s, indicating
trigger an invalid wakeup problem!

3)Solution:
Using preempt_disable() to bound the operaion that setting the task state
and the conditions(set by the wake thread) validation.

----------------------------------------------
...
preempt_disable();
set_current_state(TASK_INTERRUPTIBLE);
while (list_empty(&product_list)){
preempt_enable();
schedule();
preempt_disable();
set_current_state(TASK_INTERRUPTIBLE);
}
__set_current_state(TASK_RUNNING);
preempt_enable();
...
----------------------------------------------
And it also can be solved as follows:
----------------------------------------------
...
preempt_disable();
while (list_empty(&product_list)){
set_current_state(TASK_INTERRUPTIBLE);
preempt_enable();
schedule();
preempt_disable();
}
preempt_enable();
...
----------------------------------------------

Libin (14):
kthread: Fix invalid wakeup in kthreadd
audit: Fix invalid wakeup in kauditd_thread
audit: Fix invalid wakeup in wait_for_auditd
exit: Fix invalid wakeup in do_wait
hrtimer: Fix invalid wakeup in do_nanosleep
irq: Fix invalid wakeup in irq_wait_for_interrupt
module: Fix invalid wakeup in wait_for_zero_refcount
namespace: Fix invalid wakeup in zap_pid_ns_processes
rcutree: Fix invalid wakeup in rcu_wait
time: Fix invalid wakeup in alarmtimer_do_nsleep
trace: Fix invalid wakeup in wait_to_die
trace: Fix invalid wakeup in ring_buffer_consumer_thread
workqueue: Fix invalid wakeup in rescuer_thread
klist: Fix invalid wakeup in klist_remove

kernel/audit.c | 11 ++++-
kernel/exit.c | 7 ++-
kernel/hrtimer.c | 7 ++-
kernel/irq/manage.c | 5 ++
kernel/kthread.c | 7 ++-
kernel/module.c | 6 ++-
kernel/pid_namespace.c | 4 ++
kernel/rcutree.h | 4 ++
kernel/time/alarmtimer.c | 7 ++-
kernel/trace/ring_buffer_benchmark.c | 14 ++++++
kernel/workqueue.c | 92 +++++++++++++++++++-----------------
lib/klist.c | 4 ++
12 files changed, 118 insertions(+), 50 deletions(-)

--
1.8.2.1

2013-08-29 14:01:37

by Li Bin

[permalink] [raw]

Subject: [PATCH 04/14] exit: Fix invalid wakeup in do_wait

2013-08-29 14:01:41

by Li Bin

[permalink] [raw]

Subject: [PATCH 02/14] audit: Fix invalid wakeup in kauditd_thread

If this thread is preempted at(or before) location a, and the other thread
set the condition followed with wake_up_process. After that
when this thread is re-scheduled, calling set_current_state to set itself as
state TASK_INTERRUPTIBLE, if it is preempted again after that and before
__set_current_state(TASK_RUNNING), it triggers the invalid wakeup problem.
----------------
kauditd_thread()
----------------
while (!kthread_should_stop()) {
...
//location a
set_current_state(TASK_INTERRUPTIBLE);
...

if (!skb_queue_len(&audit_skb_queue)) {
//location b
try_to_freeze();
schedule();
//location c
}

__set_current_state(TASK_RUNNING);
//location d
...
}

To solve this problem, using preempt_disable() to bound the operaion that
setting the task state and the conditions(set by the wake thread) validation.
----------------
kauditd_thread()
----------------
while (!kthread_should_stop()) {
...
preempt_disable();
set_current_state(TASK_INTERRUPTIBLE);
...

if (!skb_queue_len(&audit_skb_queue)) {
preempt_enable();
try_to_freeze();
schedule();
preempt_disable();
}

__set_current_state(TASK_RUNNING);
preempt_enable();
...
}

Signed-off-by: Libin <[email protected]>
---
kernel/audit.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/kernel/audit.c b/kernel/audit.c
index 91e53d0..a7d1346 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -455,15 +455,19 @@ static int kauditd_thread(void *dummy)
audit_printk_skb(skb);
continue;
}
+ preempt_disable();
set_current_state(TASK_INTERRUPTIBLE);
add_wait_queue(&kauditd_wait, &wait);

if (!skb_queue_len(&audit_skb_queue)) {
+ preempt_enable();
try_to_freeze();
schedule();
+ preempt_disable();
}

__set_current_state(TASK_RUNNING);
+ preempt_enable();
remove_wait_queue(&kauditd_wait, &wait);
}
return 0;
--
1.8.2.1

2013-08-29 14:01:50

by Li Bin

[permalink] [raw]

Subject: [PATCH 01/14] kthread: Fix invalid wakeup in kthreadd

If kthreadd is preempted at(or before) location a, and the other thread,
such as calling kthread_create_on_node(), adds a list item to
the kthread_create_list followed with wake_up_process(kthread). After that
when kthreadd is re-scheduled, calling set_current_state to set itself as
state TASK_INTERRUPTIBLE, if it is preempted again after that and before
__set_current_state(TASK_RUNNING), it triggers the invalid wakeup problem.
------------------------
kthreadd()
------------------------
...
for (;;) {
//location a
set_current_state(TASK_INTERRUPTIBLE);
if (list_empty(&kthread_create_list)) {
//location b
schedule();
//location c
}
__set_current_state(TASK_RUNNING);
//location d
...
------------------------
kthread_create_on_node()
------------------------
...
spin_lock(&kthread_create_lock);
list_add_tail(&create.list, &kthread_create_list);
spin_unlock(&kthread_create_lock);
...
wake_up_process(kthreadd_task);
...

To solve this problem, using preempt_disable() to bound the operaion that
setting the task state and the conditions(set by the wake thread) validation.
------------------------
kthreadd()
------------------------
...
for (;;) {
preempt_disable();
set_current_state(TASK_INTERRUPTIBLE);
if (list_empty(&kthread_create_list)) {
preempt_enable();
schedule();
preempt_disable();
}
...

Signed-off-by: Libin <[email protected]>
---
kernel/kthread.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/kernel/kthread.c b/kernel/kthread.c
index 760e86d..25c3fed 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -456,10 +456,15 @@ int kthreadd(void *unused)
current->flags |= PF_NOFREEZE;

for (;;) {
+ preempt_disable();
set_current_state(TASK_INTERRUPTIBLE);
- if (list_empty(&kthread_create_list))
+ if (list_empty(&kthread_create_list)) {
+ preempt_enable();
schedule();
+ preempt_disable();
+ }
__set_current_state(TASK_RUNNING);
+ preempt_enable();

spin_lock(&kthread_create_lock);
while (!list_empty(&kthread_create_list)) {
--
1.8.2.1

2013-08-29 14:01:55

by Li Bin

[permalink] [raw]

Subject: [PATCH 03/14] audit: Fix invalid wakeup in wait_for_auditd

If this thread is preempted at(or before) location a, and the other thread
set the condition followed with wake_up_process. After that
when this thread is re-scheduled, calling set_current_state to set itself as
state TASK_UNINTERRUPTIBLE, if it is preempted again after that and before
__set_current_state(TASK_RUNNING), it triggers the invalid wakeup problem.
----------------
wait_for_auditd()
----------------
...
//location a
set_current_state(TASK_UNINTERRUPTIBLE);
...

if (audit_backlog_limit &&
skb_queue_len(&audit_skb_queue) > audit_backlog_limit)
//location b
schedule_timeout(sleep_time);
//location c

__set_current_state(TASK_RUNNING);
//location d
...

To solve this problem, using preempt_disable() to bound the operaion that
setting the task state and the conditions(set by the wake thread) validation.
----------------
wait_for_auditd()
----------------
...
preempt_disable();
set_current_state(TASK_UNINTERRUPTIBLE);
...

if (audit_backlog_limit &&
skb_queue_len(&audit_skb_queue) > audit_backlog_limit) {
preempt_enable();
schedule_timeout(sleep_time);
preempt_disable();
}

__set_current_state(TASK_RUNNING);
preempt_enable();
...

Signed-off-by: Libin <[email protected]>
---
kernel/audit.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index a7d1346..48d2f76 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1060,14 +1060,19 @@ static inline void audit_get_stamp(struct audit_context *ctx,
static void wait_for_auditd(unsigned long sleep_time)
{
DECLARE_WAITQUEUE(wait, current);
+ preempt_disable();
set_current_state(TASK_UNINTERRUPTIBLE);
add_wait_queue(&audit_backlog_wait, &wait);

if (audit_backlog_limit &&
- skb_queue_len(&audit_skb_queue) > audit_backlog_limit)
+ skb_queue_len(&audit_skb_queue) > audit_backlog_limit) {
+ preempt_enable();
schedule_timeout(sleep_time);
+ preempt_disable();
+ }

__set_current_state(TASK_RUNNING);
+ preempt_enable();
remove_wait_queue(&audit_backlog_wait, &wait);
}

--
1.8.2.1

2013-08-29 14:01:46

by Li Bin

[permalink] [raw]

Subject: [PATCH 12/14] trace: Fix invalid wakeup in ring_buffer_consumer_thread

2013-08-29 14:01:52

by Li Bin

[permalink] [raw]

Subject: [PATCH 08/14] namespace: Fix invalid wakeup in zap_pid_ns_processes

If thread is preempted before calling set_current_state(TASK_UNINTERRUPTIBLE),
and the other thread set the condition followed with wake_up_process. After
that when this thread is re-scheduled, calling set_current_state to set itself
as state TASK_UNINTERRUPTIBLE, if it is preempted again after that and before
__set_current_state(TASK_RUNNING), it triggers the invalid wakeup problem.
----------------------
zap_pid_ns_processes()
----------------------
...
for (;;) {
set_current_state(TASK_UNINTERRUPTIBLE);
if (pid_ns->nr_hashed == init_pids)
break;
schedule();
}
__set_current_state(TASK_RUNNING);
...

To solve this problem, using preempt_disable() to bound the operaion that
setting the task state and the conditions(set by the wake thread) validation.

Signed-off-by: Libin <[email protected]>
---
kernel/pid_namespace.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
index 6917e8e..e3696a1 100644
--- a/kernel/pid_namespace.c
+++ b/kernel/pid_namespace.c
@@ -227,13 +227,17 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)
* sys_wait4() above can't reap the TASK_DEAD children.
* Make sure they all go away, see free_pid().
*/
+ preempt_disable();
for (;;) {
set_current_state(TASK_UNINTERRUPTIBLE);
if (pid_ns->nr_hashed == init_pids)
break;
+ preempt_enable();
schedule();
+ preempt_disable();
}
__set_current_state(TASK_RUNNING);
+ preempt_enable();

if (pid_ns->reboot)
current->signal->group_exit_code = pid_ns->reboot;
--
1.8.2.1

2013-08-29 14:02:04

by Li Bin

[permalink] [raw]

Subject: [PATCH 07/14] module: Fix invalid wakeup in wait_for_zero_refcount

2013-08-29 14:02:28

by Li Bin

[permalink] [raw]

Subject: [PATCH 13/14] workqueue: Fix invalid wakeup in rescuer_thread

If thread is preempted before calling set_current_state(TASK_INTERRUPTIBLE),
and the other thread set the condition followed with wake_up_process. After
that when this thread is re-scheduled, calling set_current_state to set itself
as state TASK_INTERRUPTIBLE, if it is preempted again after that and before
__set_current_state(TASK_RUNNING), it triggers the invalid wakeup problem.

To solve this problem, using preempt_disable() to bound the operaion that
setting the task state and the conditions(set by the wake thread) validation.

Signed-off-by: Libin <[email protected]>
---
kernel/workqueue.c | 92 +++++++++++++++++++++++++++++-------------------------
1 file changed, 49 insertions(+), 43 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 7f5d4be..2dcdd30 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2358,63 +2358,69 @@ static int rescuer_thread(void *__rescuer)
* doesn't participate in concurrency management.
*/
rescuer->task->flags |= PF_WQ_WORKER;
-repeat:
- set_current_state(TASK_INTERRUPTIBLE);
+ for (;;) {

- if (kthread_should_stop()) {
+ if (kthread_should_stop()) {
+ rescuer->task->flags &= ~PF_WQ_WORKER;
+ return 0;
+ }
+
+ preempt_disable();
+ set_current_state(TASK_INTERRUPTIBLE);
+ if (list_empty(&wq->maydays)) {
+ preempt_enable();
+ schedule();
+ preempt_disable();
+ }
__set_current_state(TASK_RUNNING);
- rescuer->task->flags &= ~PF_WQ_WORKER;
- return 0;
- }
+ preempt_enable();

- /* see whether any pwq is asking for help */
- spin_lock_irq(&wq_mayday_lock);
+ /* see whether any pwq is asking for help */
+ spin_lock_irq(&wq_mayday_lock);

- while (!list_empty(&wq->maydays)) {
- struct pool_workqueue *pwq = list_first_entry(&wq->maydays,
+ while (!list_empty(&wq->maydays)) {
+ struct pool_workqueue *pwq = list_first_entry(&wq->maydays,
struct pool_workqueue, mayday_node);
- struct worker_pool *pool = pwq->pool;
- struct work_struct *work, *n;
+ struct worker_pool *pool = pwq->pool;
+ struct work_struct *work, *n;

- __set_current_state(TASK_RUNNING);
- list_del_init(&pwq->mayday_node);
+ list_del_init(&pwq->mayday_node);

- spin_unlock_irq(&wq_mayday_lock);
+ spin_unlock_irq(&wq_mayday_lock);

- /* migrate to the target cpu if possible */
- worker_maybe_bind_and_lock(pool);
- rescuer->pool = pool;
+ /* migrate to the target cpu if possible */
+ worker_maybe_bind_and_lock(pool);
+ rescuer->pool = pool;

- /*
- * Slurp in all works issued via this workqueue and
- * process'em.
- */
- WARN_ON_ONCE(!list_empty(&rescuer->scheduled));
- list_for_each_entry_safe(work, n, &pool->worklist, entry)
- if (get_work_pwq(work) == pwq)
- move_linked_works(work, scheduled, &n);
+ /*
+ * Slurp in all works issued via this workqueue and
+ * process'em.
+ */
+ WARN_ON_ONCE(!list_empty(&rescuer->scheduled));
+ list_for_each_entry_safe(work, n, &pool->worklist, entry)
+ if (get_work_pwq(work) == pwq)
+ move_linked_works(work, scheduled, &n);

- process_scheduled_works(rescuer);
+ process_scheduled_works(rescuer);

- /*
- * Leave this pool. If keep_working() is %true, notify a
- * regular worker; otherwise, we end up with 0 concurrency
- * and stalling the execution.
- */
- if (keep_working(pool))
- wake_up_worker(pool);
+ /*
+ * Leave this pool. If keep_working() is %true, notify a
+ * regular worker; otherwise, we end up with 0 concurrency
+ * and stalling the execution.
+ */
+ if (keep_working(pool))
+ wake_up_worker(pool);

- rescuer->pool = NULL;
- spin_unlock(&pool->lock);
- spin_lock(&wq_mayday_lock);
- }
+ rescuer->pool = NULL;
+ spin_unlock(&pool->lock);
+ spin_lock(&wq_mayday_lock);
+ }

- spin_unlock_irq(&wq_mayday_lock);
+ spin_unlock_irq(&wq_mayday_lock);

- /* rescuers should never participate in concurrency management */
- WARN_ON_ONCE(!(rescuer->flags & WORKER_NOT_RUNNING));
- schedule();
- goto repeat;
+ /* rescuers should never participate in concurrency management */
+ WARN_ON_ONCE(!(rescuer->flags & WORKER_NOT_RUNNING));
+ }
}

struct wq_barrier {
--
1.8.2.1

2013-08-29 14:02:27

by Li Bin

[permalink] [raw]

Subject: [PATCH 11/14] trace: Fix invalid wakeup in wait_to_die

If thread is preempted before calling set_current_state(TASK_INTERRUPTIBLE),
and the other thread set the condition(kthread_stop). After that when this
thread is re-scheduled, calling set_current_state to set itself as state
TASK_INTERRUPTIBLE, if it is preempted again after that and before
__set_current_state(TASK_RUNNING), it triggers the invalid wakeup problem.
-------------
wait_to_die()
-------------
set_current_state(TASK_INTERRUPTIBLE);
while (!kthread_should_stop()) {
schedule();
set_current_state(TASK_INTERRUPTIBLE);
}
__set_current_state(TASK_RUNNING);

To solve this problem, using preempt_disable() to bound the operaion that
setting the task state and the conditions(set by the wake thread) validation.
-------------
wait_to_die()
-------------
preempt_disable();
set_current_state(TASK_INTERRUPTIBLE);
while (!kthread_should_stop()) {
preempt_enable();
schedule();
preempt_disable();
set_current_state(TASK_INTERRUPTIBLE);
}
__set_current_state(TASK_RUNNING);
preempt_enable();

Signed-off-by: Libin <[email protected]>
---
kernel/trace/ring_buffer_benchmark.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/kernel/trace/ring_buffer_benchmark.c b/kernel/trace/ring_buffer_benchmark.c
index a5457d5..3c0bc03 100644
--- a/kernel/trace/ring_buffer_benchmark.c
+++ b/kernel/trace/ring_buffer_benchmark.c
@@ -359,12 +359,16 @@ static void ring_buffer_producer(void)

static void wait_to_die(void)
{
+ preempt_disable();
set_current_state(TASK_INTERRUPTIBLE);
while (!kthread_should_stop()) {
+ preempt_enable();
schedule();
+ preempt_disable();
set_current_state(TASK_INTERRUPTIBLE);
}
__set_current_state(TASK_RUNNING);
+ preempt_enable();
}

static int ring_buffer_consumer_thread(void *arg)
--
1.8.2.1

2013-08-29 14:02:55

by Li Bin

[permalink] [raw]

Subject: [PATCH 10/14] time: Fix invalid wakeup in alarmtimer_do_nsleep

2013-08-29 14:03:17

by Li Bin

[permalink] [raw]

Subject: [PATCH 09/14] rcutree: Fix invalid wakeup in rcu_wait

2013-08-29 14:03:49

by Li Bin

[permalink] [raw]

Subject: [PATCH 05/14] hrtimer: Fix invalid wakeup in do_nanosleep

2013-08-29 14:04:09

by Li Bin

[permalink] [raw]

Subject: [PATCH 14/14] klist: Fix invalid wakeup in klist_remove

If thread is preempted before calling set_current_state(TASK_UNINTERRUPTIBLE),
and the other thread set the condition followed with wake_up_process. After
that when this thread is re-scheduled, calling set_current_state to set itself
as state TASK_UNINTERRUPTIBLE, if it is preempted again after that and before
__set_current_state(TASK_RUNNING), it triggers the invalid wakeup problem.
--------------
klist_remove()
--------------
...
for (;;) {
set_current_state(TASK_UNINTERRUPTIBLE);
if (waiter.woken)
break;
schedule();
}
__set_current_state(TASK_RUNNING);
...

To solve this problem, using preempt_disable() to bound the operaion that
setting the task state and the conditions(set by the wake thread) validation.
--------------
klist_remove()
--------------
...
preempt_disable();
for (;;) {
set_current_state(TASK_UNINTERRUPTIBLE);
if (waiter.woken)
break;
preempt_enable();
schedule();
preempt_disable();
}
__set_current_state(TASK_RUNNING);
preempt_enable();
...

Signed-off-by: Libin <[email protected]>
---
lib/klist.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/lib/klist.c b/lib/klist.c
index 358a368..e7c7208 100644
--- a/lib/klist.c
+++ b/lib/klist.c
@@ -249,13 +249,17 @@ void klist_remove(struct klist_node *n)

klist_del(n);

+ preempt_disable();
for (;;) {
set_current_state(TASK_UNINTERRUPTIBLE);
if (waiter.woken)
break;
+ preempt_enable();
schedule();
+ preempt_disable();
}
__set_current_state(TASK_RUNNING);
+ preempt_enable();
}
EXPORT_SYMBOL_GPL(klist_remove);

--
1.8.2.1

2013-08-29 14:04:43

by Li Bin

[permalink] [raw]

Subject: [PATCH 06/14] irq: Fix invalid wakeup in irq_wait_for_interrupt

If this thread is preempted at(or before) location a, and the other thread
set the condition(kthread_stop). After that when this thread is re-scheduled,
calling set_current_state to set itself as state TASK_INTERRUPTIBLE, if it is
preempted again after that and before __set_current_state(TASK_RUNNING), it
triggers the invalid wakeup problem.
-----------------------
irq_wait_for_interrupt()
-----------------------
...
//location a
set_current_state(TASK_INTERRUPTIBLE);
//location b
while (!kthread_should_stop()) {

if (test_and_clear_bit(IRQTF_RUNTHREAD,
&action->thread_flags)) {
__set_current_state(TASK_RUNNING);
return 0;
}
schedule();//location c
set_current_state(TASK_INTERRUPTIBLE);
//location d
}
__set_current_state(TASK_RUNNING);
...

The following circumstance will also trigger this issue:
At location c, consumer is scheduled out, and be preempted after calling
set_current_state(TASK_INTERRUPTIBLE) when it be re-schdeuled.

To solve this problem, using preempt_disable() to bound the operaion that
setting the task state and the conditions(set by the wake thread) validation.

Signed-off-by: Libin <[email protected]>
---
kernel/irq/manage.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 514bcfd..09cb02f 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -655,6 +655,7 @@ static irqreturn_t irq_nested_primary_handler(int irq, void *dev_id)

static int irq_wait_for_interrupt(struct irqaction *action)
{
+ preempt_disable();
set_current_state(TASK_INTERRUPTIBLE);

while (!kthread_should_stop()) {
@@ -662,12 +663,16 @@ static int irq_wait_for_interrupt(struct irqaction *action)
if (test_and_clear_bit(IRQTF_RUNTHREAD,
&action->thread_flags)) {
__set_current_state(TASK_RUNNING);
+ preempt_enable();
return 0;
}
+ preempt_enable();
schedule();
+ preempt_disable();
set_current_state(TASK_INTERRUPTIBLE);
}
__set_current_state(TASK_RUNNING);
+ preempt_enable();
return -1;
}

--
1.8.2.1

2013-08-29 14:10:43

by Tejun Heo

[permalink] [raw]

Subject: Re: [PATCH 00/14] Fix bug about invalid wake up problem

Hello, Libin.

I'm completely confused by this series....

On Thu, Aug 29, 2013 at 09:57:35PM +0800, Libin wrote:
> 1)Problem Description:
> The prototype of invalid wakeup problem is as follows:
> ========================================================================
> ----------------------------
> Consumer Thread
> ----------------------------
> ...
> if (list_empty(&list)){
> //location a
> set_current_state(TASK_INTERRUPTIBLE);
> schedule();
> }
> ...
> ----------------------------
> Producer Thread
> ----------------------------
> ...
> list_add_tail(&item, &list);
> wake_up_process(A);
> ...

This is of course broken. set_current_state() should of course come
*before* the conditions is checked. This is just plain broken code.

> In most cases, the kernel codes use a form similar to the following:
> --------------------------------------------
> ...
> //location a
> ...
> set_current_state(TASK_INTERRUPTIBLE);
> //location b
> while (list_empty(&product_list)){
> schedule(); //location c
> set_current_state(TASK_INTERRUPTIBLE);
> //location d
> }
> __set_current_state(TASK_RUNNING);
> ...
> --------------------------------------------
> At location a, consumer is preempted, and producer is scheduled,
> adding item to product_list and waking up consumer. After consumer is
> re-scheduled, calling set_current_state to set itself as state
> TASK_INTERRUPTIBLE, if it is preempted again before __set_current_state(TASK_RUNNING),
> also trigger the invalid wakeup problem that the consumer will not be scheduled

Why? Getting preempted != calling schedule(). A preempted task will
be rescheduled *regardless* of ifs current->state; otherwise, the
whole kernel is severely broken. Tasks never get deactivated while
preempted.

> and the item be added by producer can't be consumed.
>
> The following circumstance will also trigger this issue:
> At location c, consumer is scheduled out, and be preempted after calling
> set_current_state(TASK_INTERRUPTIBLE) when it be re-schdeuled.

What?

> 2)Test:
> I have written a test module to trigger the problem by adding some
> synchronization condition. I will post it in the form of an attachment soon.
>
> Test result as follows:
> [103135.332683] wakeup_test: create two kernel threads - producer & consumer
> [103135.332686] wakeup_test: module loaded successfully
> [103135.332692] wakeup_test: kthread producer try to wake up the kthread consumer
> [103165.299865] wakeup_test: kthread consumer have waited for 30s, indicating
> trigger an invalid wakeup problem!

The most interesting question here is "what were you testing?". Can
you please post the test code?

For the whole series,

Nacked-by: Tejun Heo <[email protected]>

Thanks.

--
tejun

2013-08-29 14:11:10

by Li Bin

[permalink] [raw]

Subject: Re: [PATCH 00/14] Fix bug about invalid wake up problem

On 2013/8/29 21:57, Libin wrote:
....
> 2)Test:
> I have written a test module to trigger the problem by adding some
> synchronization condition. I will post it in the form of an attachment soon.
>
> Test result as follows:
> [103135.332683] wakeup_test: create two kernel threads - producer & consumer
> [103135.332686] wakeup_test: module loaded successfully
> [103135.332692] wakeup_test: kthread producer try to wake up the kthread consumer
> [103165.299865] wakeup_test: kthread consumer have waited for 30s, indicating
> trigger an invalid wakeup problem!
>
....

Attachments:

wakeup_test.c (2.97 kB)

2013-08-29 14:12:53

by Tejun Heo

[permalink] [raw]

Subject: Re: [PATCH 00/14] Fix bug about invalid wake up problem

Hello, again.

On Thu, Aug 29, 2013 at 10:08:13PM +0800, Libin wrote:
...
> static void simulate_preempted(void)
> {
> schedule();
> }

Gees... of course, your test code will stall. Preemption !=
schedule(). Please take a look at PREEMPT_ACTIVE handling in
__schedule().

Thanks.

--
tejun

2013-08-29 23:42:16

by Li Bin

[permalink] [raw]

Subject: Re: [PATCH 00/14] Fix bug about invalid wake up problem

Hi all,
I'm so sorry, please ignore this patch set!
I have realized that there is no this problem in our kernel.
Preempt_schedule() has set PREEMPT_ACTIVE before calling
__schedule() and __schedule will check it if current state is not
TASK_RUNNING, avoiding this preemption.

Libin

On 2013/8/29 21:57, Libin wrote:
> 1)Problem Description:
> The prototype of invalid wakeup problem is as follows:
> ========================================================================
> ----------------------------
> Consumer Thread
> ----------------------------
> ...

2013-08-30 00:19:10

by Paul E. McKenney

[permalink] [raw]

Subject: Re: [PATCH 00/14] Fix bug about invalid wake up problem

On Thu, Aug 29, 2013 at 09:57:35PM +0800, Libin wrote:
> 1)Problem Description:
> The prototype of invalid wakeup problem is as follows:
> ========================================================================
> ----------------------------
> Consumer Thread
> ----------------------------
> ...
> if (list_empty(&list)){
> //location a
> set_current_state(TASK_INTERRUPTIBLE);

You need the list_empty() test here, not above. Or you at least need
to retest here.

The reason for this is that set_current_state() has a memory
barrier following the assignment, which matches the memory barrier in
try_to_wake_up(). This means that if the test at this point sees the
list as empty, then the checks in try_to_wake_up() must see the task in
TASK_INTERRUPTIBLE state and must therefore wake it up.

Thanx, Paul

> schedule();
> }
> ...
> ----------------------------
> Producer Thread
> ----------------------------
> ...
> list_add_tail(&item, &list);
> wake_up_process(A);
> ...
> ========================================================================
>
> Invalid wakeup problem occurs in preemptable kernel environment. The list
> is empty initially, if consumer is preempted after condition validated at
> location a, and at this time producer is woken up, adding a node to the
> list and calling wake_up_process to wake up consumer. After that when
> consumer being scheduled by scheduler, it calls set_current_state to set
> itself as state TASK_INTERRUPTIBLE, and calling schedule() to request
> scheduled. After consumer being scheduled out, it has no condition to be
> woken up, and causing invalid wakeup problem.
>
> In most cases, the kernel codes use a form similar to the following:
> --------------------------------------------
> ...
> //location a
> ...
> set_current_state(TASK_INTERRUPTIBLE);
> //location b
> while (list_empty(&product_list)){
> schedule(); //location c
> set_current_state(TASK_INTERRUPTIBLE);
> //location d
> }
> __set_current_state(TASK_RUNNING);
> ...
> --------------------------------------------
> At location a, consumer is preempted, and producer is scheduled,
> adding item to product_list and waking up consumer. After consumer is
> re-scheduled, calling set_current_state to set itself as state
> TASK_INTERRUPTIBLE, if it is preempted again before __set_current_state(TASK_RUNNING),
> also trigger the invalid wakeup problem that the consumer will not be scheduled
> and the item be added by producer can't be consumed.
>
> The following circumstance will also trigger this issue:
> At location c, consumer is scheduled out, and be preempted after calling
> set_current_state(TASK_INTERRUPTIBLE) when it be re-schdeuled.
>
> 2)Test:
> I have written a test module to trigger the problem by adding some
> synchronization condition. I will post it in the form of an attachment soon.
>
> Test result as follows:
> [103135.332683] wakeup_test: create two kernel threads - producer & consumer
> [103135.332686] wakeup_test: module loaded successfully
> [103135.332692] wakeup_test: kthread producer try to wake up the kthread consumer
> [103165.299865] wakeup_test: kthread consumer have waited for 30s, indicating
> trigger an invalid wakeup problem!
>
> 3)Solution:
> Using preempt_disable() to bound the operaion that setting the task state
> and the conditions(set by the wake thread) validation.
>
> ----------------------------------------------
> ...
> preempt_disable();
> set_current_state(TASK_INTERRUPTIBLE);
> while (list_empty(&product_list)){
> preempt_enable();
> schedule();
> preempt_disable();
> set_current_state(TASK_INTERRUPTIBLE);
> }
> __set_current_state(TASK_RUNNING);
> preempt_enable();
> ...
> ----------------------------------------------
> And it also can be solved as follows:
> ----------------------------------------------
> ...
> preempt_disable();
> while (list_empty(&product_list)){
> set_current_state(TASK_INTERRUPTIBLE);
> preempt_enable();
> schedule();
> preempt_disable();
> }
> preempt_enable();
> ...
> ----------------------------------------------
>
> Libin (14):
> kthread: Fix invalid wakeup in kthreadd
> audit: Fix invalid wakeup in kauditd_thread
> audit: Fix invalid wakeup in wait_for_auditd
> exit: Fix invalid wakeup in do_wait
> hrtimer: Fix invalid wakeup in do_nanosleep
> irq: Fix invalid wakeup in irq_wait_for_interrupt
> module: Fix invalid wakeup in wait_for_zero_refcount
> namespace: Fix invalid wakeup in zap_pid_ns_processes
> rcutree: Fix invalid wakeup in rcu_wait
> time: Fix invalid wakeup in alarmtimer_do_nsleep
> trace: Fix invalid wakeup in wait_to_die
> trace: Fix invalid wakeup in ring_buffer_consumer_thread
> workqueue: Fix invalid wakeup in rescuer_thread
> klist: Fix invalid wakeup in klist_remove
>
> kernel/audit.c | 11 ++++-
> kernel/exit.c | 7 ++-
> kernel/hrtimer.c | 7 ++-
> kernel/irq/manage.c | 5 ++
> kernel/kthread.c | 7 ++-
> kernel/module.c | 6 ++-
> kernel/pid_namespace.c | 4 ++
> kernel/rcutree.h | 4 ++
> kernel/time/alarmtimer.c | 7 ++-
> kernel/trace/ring_buffer_benchmark.c | 14 ++++++
> kernel/workqueue.c | 92 +++++++++++++++++++-----------------
> lib/klist.c | 4 ++
> 12 files changed, 118 insertions(+), 50 deletions(-)
>
> --
> 1.8.2.1
>
>

2013-08-30 00:20:43

by Paul E. McKenney

[permalink] [raw]

Subject: Re: [PATCH 01/14] kthread: Fix invalid wakeup in kthreadd

On Thu, Aug 29, 2013 at 09:57:36PM +0800, Libin wrote:
> If kthreadd is preempted at(or before) location a, and the other thread,
> such as calling kthread_create_on_node(), adds a list item to
> the kthread_create_list followed with wake_up_process(kthread). After that
> when kthreadd is re-scheduled, calling set_current_state to set itself as
> state TASK_INTERRUPTIBLE, if it is preempted again after that and before
> __set_current_state(TASK_RUNNING), it triggers the invalid wakeup problem.
> ------------------------
> kthreadd()
> ------------------------
> ...
> for (;;) {
> //location a
> set_current_state(TASK_INTERRUPTIBLE);
> if (list_empty(&kthread_create_list)) {
> //location b
> schedule();
> //location c
> }
> __set_current_state(TASK_RUNNING);
> //location d
> ...
> ------------------------
> kthread_create_on_node()
> ------------------------
> ...
> spin_lock(&kthread_create_lock);
> list_add_tail(&create.list, &kthread_create_list);
> spin_unlock(&kthread_create_lock);
> ...
> wake_up_process(kthreadd_task);
> ...
>
> To solve this problem, using preempt_disable() to bound the operaion that
> setting the task state and the conditions(set by the wake thread) validation.
> ------------------------
> kthreadd()
> ------------------------
> ...
> for (;;) {
> preempt_disable();
> set_current_state(TASK_INTERRUPTIBLE);
> if (list_empty(&kthread_create_list)) {
> preempt_enable();
> schedule();
> preempt_disable();
> }
> ...
>
> Signed-off-by: Libin <[email protected]>
> ---
> kernel/kthread.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/kthread.c b/kernel/kthread.c
> index 760e86d..25c3fed 100644
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -456,10 +456,15 @@ int kthreadd(void *unused)
> current->flags |= PF_NOFREEZE;
>
> for (;;) {
> + preempt_disable();
> set_current_state(TASK_INTERRUPTIBLE);
> - if (list_empty(&kthread_create_list))

And this one already has the list_empty() check after the call to
set_current_state(), so no change should be needed here.

On the other hand, if your testing shows that you are losing wakeups
with this exact code, please let us know!

Thanx, Paul

> + if (list_empty(&kthread_create_list)) {
> + preempt_enable();
> schedule();
> + preempt_disable();
> + }
> __set_current_state(TASK_RUNNING);
> + preempt_enable();
>
> spin_lock(&kthread_create_lock);
> while (!list_empty(&kthread_create_list)) {
> --
> 1.8.2.1
>
>

2013-08-30 00:25:26

by Paul E. McKenney

[permalink] [raw]

Subject: Re: [PATCH 09/14] rcutree: Fix invalid wakeup in rcu_wait

On Thu, Aug 29, 2013 at 09:57:44PM +0800, Libin wrote:
> If thread is preempted before calling set_current_state(TASK_INTERRUPTIBLE),
> and the other thread set the condition followed with wake_up_process. After
> that when this thread is re-scheduled, calling set_current_state to set itself
> as state TASK_INTERRUPTIBLE, if it is preempted again after that and before
> __set_current_state(TASK_RUNNING), it triggers the invalid wakeup problem.
>
> To solve this problem, using preempt_disable() to bound the operaion that
> setting the task state and the conditions(set by the wake thread) validation.
>
> Signed-off-by: Libin <[email protected]>
> ---
> kernel/rcutree.h | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/kernel/rcutree.h b/kernel/rcutree.h
> index b383258..e3f1278 100644
> --- a/kernel/rcutree.h
> +++ b/kernel/rcutree.h
> @@ -357,13 +357,17 @@ struct rcu_data {
>
> #define rcu_wait(cond) \
> do { \
> + preempt_disable(); \
> for (;;) { \
> set_current_state(TASK_INTERRUPTIBLE); \
> if (cond) \
> break; \

Here also the condition check follows the call to set_current_state(),
so this patch should not be needed.

Again, if you are seeing lost wakeups on this exact code path, please
let me know.

Please re-check your other patches. If they really do follow the
buggy pattern you called out in your patch 0, namely the test preceding
the call to set_current_state(), please send patches that restore the
correct ordering.

Your use of preemption is not fixing anything. It is at best hiding
the problem by making it less likely to occur.

Thanx, Paul

> + preempt_enable(); \
> schedule(); \
> + preempt_disable(); \
> } \
> __set_current_state(TASK_RUNNING); \
> + preempt_enable(); \
> } while (0)
>
> /*
> --
> 1.8.2.1
>
>

2013-09-12 13:34:28

by Thomas Gleixner

[permalink] [raw]

Subject: Re: [PATCH 05/14] hrtimer: Fix invalid wakeup in do_nanosleep

On Thu, 29 Aug 2013, Libin wrote:

> If thread is preempted before calling set_current_state(TASK_INTERRUPTIBLE),
> and the other thread set the condition followed with wake_up_process. After
> that when this thread is re-scheduled, calling set_current_state to set itself
> as state TASK_INTERRUPTIBLE, if it is preempted again after that and before
> __set_current_state(TASK_RUNNING), it triggers the invalid wakeup problem.

do_nanosleep() is only called from sys_nanosleep() and
sys_clock_nanosleep() user space interfaces.

So the task is not going to be woken up by some magic other thread
except by a signal. The latter is handled by the scheduler which will
return with state running. So what kind of problem are you trying to
solve?

Thanks,

tglx

2013-09-12 13:36:37

by Thomas Gleixner

[permalink] [raw]

Subject: Re: [PATCH 06/14] irq: Fix invalid wakeup in irq_wait_for_interrupt

On Thu, 29 Aug 2013, Libin wrote:
> If this thread is preempted at(or before) location a, and the other thread
> set the condition(kthread_stop). After that when this thread is re-scheduled,
> calling set_current_state to set itself as state TASK_INTERRUPTIBLE, if it is
> preempted again after that and before __set_current_state(TASK_RUNNING), it
> triggers the invalid wakeup problem.
> -----------------------
> irq_wait_for_interrupt()
> -----------------------
> ...
> //location a
> set_current_state(TASK_INTERRUPTIBLE);
> //location b
> while (!kthread_should_stop()) {
>
> if (test_and_clear_bit(IRQTF_RUNTHREAD,
> &action->thread_flags)) {
> __set_current_state(TASK_RUNNING);
> return 0;
> }
> schedule();//location c
> set_current_state(TASK_INTERRUPTIBLE);
> //location d
> }
> __set_current_state(TASK_RUNNING);
> ...
>
> The following circumstance will also trigger this issue:
> At location c, consumer is scheduled out, and be preempted after calling
> set_current_state(TASK_INTERRUPTIBLE) when it be re-schdeuled.
>
> To solve this problem, using preempt_disable() to bound the operaion that
> setting the task state and the conditions(set by the wake thread) validation.

This is a completely pointless exercise. The irq threads are only
woken from the core code itself and not by some random other threads.

The core code already handles the legit wakeups correctly without
having the need for preemption disable.

Thanks,

tglx

2013-09-12 13:37:28

by Thomas Gleixner

[permalink] [raw]

Subject: Re: [PATCH 10/14] time: Fix invalid wakeup in alarmtimer_do_nsleep

On Thu, 29 Aug 2013, Libin wrote:

> If thread is preempted before calling set_current_state(TASK_INTERRUPTIBLE),
> and the other thread set the condition followed with wake_up_process. After
> that when this thread is re-scheduled, calling set_current_state to set itself
> as state TASK_INTERRUPTIBLE, if it is preempted again after that and before
> __set_current_state(TASK_RUNNING), it triggers the invalid wakeup problem.
>
> To solve this problem, using preempt_disable() to bound the operaion that
> setting the task state and the conditions(set by the wake thread) validation.

See the reply to the hrtimer patch. You're solving a non issue.

Thanks,

tglx