fix the racy check_hung_uninterruptible_tasks()->rcu_lock_break() logic

check_hung_uninterruptible_tasks()->rcu_lock_break() introduced by "softlockup: check all tasks in hung_task" commit ce9dbe24 looks absolutely wrong.

- rcu_lock_break() does put_task_struct(). If the task has exited it is not safe to even read its ->state, nothing protects this task_struct.

- The TASK_DEAD checks are wrong too. Contrary to the comment, we can't use it to check if the task was unhashed. It can be unhashed without TASK_DEAD, or it can be valid with TASK_DEAD.

For example, an autoreaping task can do release_task(current) long before it sets TASK_DEAD in do_exit().

Or, a zombie task can have ->state == TASK_DEAD but release_task() was not called, and in this case we must not break the loop.

Change this code to check pid_alive() instead, and do this before we drop the reference to the task_struct.

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

---

 kernel/hung_task.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

kernel/hung_task.c renamed from kernel/hung_task.c~CHT_FIX_RCU_LOCK_BREAK +7 -4

@@ -113,15 +113,20 @@ static void check_hung_task(struct task_
  * For preemptible RCU it is sufficient to call rcu_read_unlock in order
  * exit the grace period. For classic RCU, a reschedule is required.
  */
-static void rcu_lock_break(struct task_struct *g, struct task_struct *t)
+static bool rcu_lock_break(struct task_struct *g, struct task_struct *t)
 {
+	bool can_cont;
+
 	get_task_struct(g);
 	get_task_struct(t);
 	rcu_read_unlock();
 	cond_resched();
 	rcu_read_lock();
+	can_cont = pid_alive(g) && pid_alive(t);
 	put_task_struct(t);
 	put_task_struct(g);
+
+	return can_cont;
 }
 
 /*

@@ -148,9 +153,7 @@ static void check_hung_uninterruptible_t
 			goto unlock;
 		if (!--batch_count) {
 			batch_count = HUNG_TASK_BATCHING;
-			rcu_lock_break(g, t);
-			/* Exit if t or g was unhashed during refresh. */
-			if (t->state == TASK_DEAD || g->state == TASK_DEAD)
+			if (!rcu_lock_break(g, t))
 				goto unlock;
 		}
 		/* use "==" to skip the TASK_KILLABLE tasks waiting on NFS */

I think you're right. I can't see what would prevent that race.

So for_each_process

and do_each_thread are safe only under read_lock(&tasklist_lock) and while_each_thread is only safe under either that or siglock.

similar loops outside those macros.)

Perhaps we could move those del's from __unhash_process to __put_task_struct (or just delayed_put_task_struct?)

I think de_thread() in exec-by-nonleader is the only case where this can happen, right? So then perhaps we could make it call release_task only via call_rcu?

(add cc's)

Hmm. Once I sent this patch, I suddenly realized with horror that while_each_thread() is NOT safe under rcu_read_lock(). Both do_each_thread/while_each_thread or do/while_each_thread() can race with exec().

Yes, it is safe to do next_thread() or next_task(). But:

	#define while_each_thread(g, t) \
		while ((t = next_thread(t)) != g)

suppose that t is not the group leader, and it does de_thread() and then release_task(g). After that next_thread(t) returns t, not g, and the loop will never stop.

I _really_ hope I missed something, will recheck tomorrow with the fresh head. Still I'd like to share my concerns...

If I am right, probably we can fix this, something like

	#define while_each_thread(g, t) \
		while ((t = next_thread(t)) != g && pid_alive(g))

[we can't do while (!thread_group_leadr(t = next_thread(t)))]. but this needs barrires, and we should validate the callers anyway.

Or, perhaps,

	#define XXX(t)	({
		struct task_struct *__prev = t;
		t = next_thread(t);
		t != g && t != __prev;
	})

	#define while_each_thread(g, t) \
		while (XXX(t))

Please tell me I am wrong!

Oleg.

Quoted 63 lines · Oleg Nesterov

check_hung_uninterruptible_tasks()->rcu_lock_break() introduced by "softlockup: check all tasks in hung_task" commit ce9dbe24 looks absolutely wrong.

- rcu_lock_break() does put_task_struct(). If the task has exited it is not safe to even read its ->state, nothing protects this task_struct.

- The TASK_DEAD checks are wrong too. Contrary to the comment, we can't use it to check if the task was unhashed. It can be unhashed without TASK_DEAD, or it can be valid with TASK_DEAD.

For example, an autoreaping task can do release_task(current) long before it sets TASK_DEAD in do_exit().

Or, a zombie task can have ->state == TASK_DEAD but release_task() was not called, and in this case we must not break the loop.

Change this code to check pid_alive() instead, and do this before we drop the reference to the task_struct.

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

---

 kernel/hung_task.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

kernel/hung_task.c renamed from kernel/hung_task.c~CHT_FIX_RCU_LOCK_BREAK +7 -4

@@ -113,15 +113,20 @@ static void check_hung_task(struct task_
  * For preemptible RCU it is sufficient to call rcu_read_unlock in order
  * exit the grace period. For classic RCU, a reschedule is required.
  */
-static void rcu_lock_break(struct task_struct *g, struct task_struct *t)
+static bool rcu_lock_break(struct task_struct *g, struct task_struct *t)
 {
+	bool can_cont;
+
 	get_task_struct(g);
 	get_task_struct(t);
 	rcu_read_unlock();
 	cond_resched();
 	rcu_read_lock();
+	can_cont = pid_alive(g) && pid_alive(t);
 	put_task_struct(t);
 	put_task_struct(g);
+
+	return can_cont;
 }
 
 /*

@@ -148,9 +153,7 @@ static void check_hung_uninterruptible_t
 			goto unlock;
 		if (!--batch_count) {
 			batch_count = HUNG_TASK_BATCHING;
-			rcu_lock_break(g, t);
-			/* Exit if t or g was unhashed during refresh. */
-			if (t->state == TASK_DEAD || g->state == TASK_DEAD)
+			if (!rcu_lock_break(g, t))
 				goto unlock;
 		}
 		/* use "==" to skip the TASK_KILLABLE tasks waiting on NFS */

Quoted 7 lines · Oleg Nesterov

	#define XXX(t)	({
		struct task_struct *__prev = t;
		t = next_thread(t);
		t != g && t != __prev;
	})

Quoted 2 lines

	#define while_each_thread(g, t) \
		while (XXX(t))

Isn't the above vulnerable to a pthread_create() immediately following the offending exec()? Especially if the task doing the traversal is preempted?

here are some techniques that might (or might not) help:

o Check ACCESS_ONCE(p->group_leader == g),

if false, restart
the traversal.

This of course might
require careful attention of the order of updates to ->group_leader and the list pointers.

o Maintain an ->oldnext field that tracks the old pointer to
the next task for one RCU grace period after a de_thread() operation. When the grace period expires (presumably via call_rcu()), the ->oldnext field is set to NULL.

o Do the de_thread() incrementally. So if the list is tasks A,
B, and C, in that order, and if we are de-thread()ing B, then make A's pointer refer to C,

Quoted 1 line

Please tell me I am wrong!

It would take a braver man than me to say that Oleg Nesterov is wrong!

So, I am thinking about the first attempt

	#define while_each_thread(g, t) \
		while ((t = next_thread(t)) != g && pid_alive(g))

again. But this means while_each_thread() can miss more threads than it currently can under the same conditions. Correct, but not good.

Quoted 9 lines · Oleg Nesterov

So, I am thinking about the first attempt

	#define while_each_thread(g, t) \
		while ((t = next_thread(t)) != g && pid_alive(g))

again. But this means while_each_thread() can miss more threads than it currently can under the same conditions. Correct, but not good.

Not good, but correct ;) Probably it makes sense to fix the problem anyway, then think about the more optimal fix.

	static inline struct task_struct *
	next_thread_careful(const struct task_struct *g, const struct task_struct *t)
	{
		t = next_thread(t);
		/*
		 * this pairs with the implicit barrier between detach_pid()
		 * and list_del_rcu(g->thread_group) in __unhash_process(g).
		 */
		smp_rmb();
		if (likely(pid_alive(g)))
			return t;
		else
			return g;
	}

	#define while_each_thread(g, t) \
		while ((t = next_thread_careful(t)) != g)

I think this should work. detach_pid() does unlock + lock at least once and thus we have the barrier (this worth a comment or we can add the explicit wmb() in __unhash_process).

Paul, Roland, do you see any problems from the correctness pov, or a better fix for now?

Perhaps it also makes sense to keep the old variant renamed to while_each_thread_locked(), I dunno.

Paul, Roland, do you see any problems from the correctness pov, or a better fix for now?

Perhaps it also makes sense to keep the old variant renamed to while_each_thread_locked(), I dunno.

Quoted 6 lines

Paul, Roland, do you see any problems from the correctness pov, or a better fix for now?

Perhaps it also makes sense to keep the old variant renamed to while_each_thread_locked(), I dunno.

Did we verify that only de_thread() can create the situation where a while_each_thread-style loop without either lock can be confused?

If
that's so, then just changing it to avoid the situation seems like it would be less invasive overall.

Quoted 9 lines · Roland McGrath

Quoted 6 lines

Paul, Roland, do you see any problems from the correctness pov, or a better fix for now?

Perhaps it also makes sense to keep the old variant renamed to while_each_thread_locked(), I dunno.

Did we verify that only de_thread() can create the situation where a while_each_thread-style loop without either lock can be confused?

I think yes, this is is the only case.

I mean, while_each_thread(group_leader, t). If g != group_leader, then the lockless while_each_thread() has problems with the plain exit(g).

Afaics. The more I think about this, the more I feel confused ;)

But if we start from ->group_leader, then while_each_thread() must stop eventually. Otherwise we should assume that the dead (unhashed) tasks can create the circular list, obviously this is not possible.

Quoted 3 lines

If
that's so, then just changing it to avoid the situation seems like it would be less invasive overall.

How? We should change ->group_leader uner write_lock_irq(tasklist), synchronize_rcu() is not an option. We can't do call_rcu(release_task), we can't take tasklist for writing in the softirq context. But even if we could, this can't help in fact or I missed something.

Quoted 4 lines · Oleg Nesterov

Quoted 3 lines

If
that's so, then just changing it to avoid the situation seems like it would be less invasive overall.

Quoted 4 lines

How? We should change ->group_leader uner write_lock_irq(tasklist), synchronize_rcu() is not an option. We can't do call_rcu(release_task), we can't take tasklist for writing in the softirq context. But even if we could, this can't help in fact or I missed something.

We already do: call_rcu(&p->rcu, delayed_put_task_struct); in release_task. We don't call release_task until after we have removed it as leader and dropped the write lock.

At first glance it sounds like the group leader is safe as a stopping point for a rcu while_each_thread, and I expect the fact that de_thread takes everything down to a single thread, could have nice properties here. If pid_alive were only to fail on the group leader when de_thread is called I think we could legitimately say that an event we won't worry about. It is close enough to a new thread being created anyway.

We can't do call_rcu(release_task),
we can't take tasklist for writing in the softirq context. But even if we could, this can't help in fact or I missed something.

Quoted 3 lines

We can't do call_rcu(release_task),
we can't take tasklist for writing in the softirq context. But even if we could, this can't help in fact or I missed something.

Ah, I had missed that constraint of call_rcu. (It's not mentioned in the kerneldoc comment directly, and is only buried in a couple of brief mentions in Documentation/RCU/.) I was thinking that would suffice because it does it between rcu_read_lock critical sections, but I guess it actually only does it after the last one and doesn't prevent a new one from having started on another CPU. (And we can't use it anyway.)

Hmmm... The kerneldoc for rcu_read_lock() looks like it says this pretty unambiguously. Then again, that is easy for me to say, given
that I was the one who made most of this stuff up. ;-)

But it has been awhile since I looked after the kerneldoc. Please see below for the changes I would make if left to myself.

Any other suggestions for improvement to the kerneldoc?

Quoted 3 lines

Hmmm... The kerneldoc for rcu_read_lock() looks like it says this pretty unambiguously. Then again, that is easy for me to say, given
that I was the one who made most of this stuff up. ;-)

Yeah, well, I thought I knew what rcu_read_lock did and the only new thing I had in mind was using call_rcu, so that's the only place I looked. It wasn't entirely unreasonable to have to read through Documentation/RCU/ a bit for a subtlety of this kind, and I did find it quickly enough once Oleg's attentiveness alerted me to be freshly afraid of the subtleties.

Quoted 2 lines

But it has been awhile since I looked after the kerneldoc. Please see below for the changes I would make if left to myself.

Those look good to me!

Quoted 1 line

Any other suggestions for improvement to the kerneldoc?

Oh, I don't know, send over someone to read it to me in a sultry voice so I get appropriately motivated to cast about for all of it I can find and listen closely to every word? It already looks like if I'd actually read all of it twice that day, I would have had the information I needed.

Quoted 9 lines · Paul E. McKenney

Quoted 7 lines · Oleg Nesterov

	#define XXX(t)	({
		struct task_struct *__prev = t;
		t = next_thread(t);
		t != g && t != __prev;
	})

Quoted 6 lines

Quoted 2 lines

	#define while_each_thread(g, t) \
		while (XXX(t))

Isn't the above vulnerable to a pthread_create() immediately following the offending exec()? Especially if the task doing the traversal is preempted?

Yes, thanks!

Quoted 1 line

here are some techniques that might (or might not) help:

To simplify, let's consider the concrete example,

	rcu_read_lock();

	g = t = returns_the_rcu_safe_task_struct_ptr();

	do {
		printk("%d\n", t->pid);
	} while_each_thread(g, t);

	rcu_read_unlock();

Whatever we do, without tasklist/siglock this can obviously race with fork/exit/exec. It is OK to miss a thread, or print the pid of the already exited/released task.

But it should not loop forever (the subject), and it should not print the same pid twice (ignoring pid reuse, of course).

And, afaics, there are no problems with rcu magic per se, next_thread() always returns the task_struct we can safely dereference. The only problem is that while_each_thread() assumes that sooner or later next_thread() must reach the starting point, g.

(zap_threads() is different, it must not miss a thread with ->mm
we are going to dump, but it holds mmap_sem).

Quoted 1 line

o Check ACCESS_ONCE(p->group_leader == g),

	but this assumeas that while_each_thread(g, t) always
	uses g == group_leader. zap_other_threads(), for example,
	starts with g == current and not necessarily the leader.

Quoted 2 lines

if false, restart
the traversal.

I am not sure we should restart (don't pid the same pid twice), probably we should break the loop.

So, I am thinking about the first attempt

	#define while_each_thread(g, t) \
		while ((t = next_thread(t)) != g && pid_alive(g))

again. But this means while_each_thread() can miss more threads than it currently can under the same conditions. Correct, but not good.

And,

Quoted 3 lines

This of course might
require careful attention of the order of updates to ->group_leader and the list pointers.

Yes. At least the code above needs barriers, and I am not sure it can be really correct without more complications.

Quoted 4 lines

o Maintain an ->oldnext field that tracks the old pointer to
the next task for one RCU grace period after a de_thread() operation. When the grace period expires (presumably via call_rcu()), the ->oldnext field is set to NULL.

Well, another field in task_struct...

Quoted 3 lines

o Do the de_thread() incrementally. So if the list is tasks A,
B, and C, in that order, and if we are de-thread()ing B, then make A's pointer refer to C,

This breaks while_each_thread() under tasklist/siglock. It must see all unhashed tasks.

Oh. I'll try to think more, but I also hope someone else can suggest a simple solution.

Quoted 3 lines

Quoted 1 line

Please tell me I am wrong!

It would take a braver man than me to say that Oleg Nesterov is wrong!

Hmm. Given that you proved I was wrong in the first lines of your email, I do not know what to think ;)

Quoted 17 lines · Oleg Nesterov

Quoted 9 lines · Paul E. McKenney

Quoted 7 lines · Oleg Nesterov

	#define XXX(t)	({
		struct task_struct *__prev = t;
		t = next_thread(t);
		t != g && t != __prev;
	})

Quoted 6 lines

Quoted 2 lines

	#define while_each_thread(g, t) \
		while (XXX(t))

Isn't the above vulnerable to a pthread_create() immediately following the offending exec()? Especially if the task doing the traversal is preempted?

Quoted 5 lines

Yes, thanks!

Quoted 1 line

here are some techniques that might (or might not) help:

To simplify, let's consider the concrete example,

Sounds very good!

Quoted 3 lines

	rcu_read_lock();

	g = t = returns_the_rcu_safe_task_struct_ptr();

This returns a pointer to the task struct of the current thread? Or might this return a pointer some other thread's task struct?

Quoted 13 lines

	do {
		printk("%d\n", t->pid);
	} while_each_thread(g, t);

	rcu_read_unlock();

Whatever we do, without tasklist/siglock this can obviously race with fork/exit/exec. It is OK to miss a thread, or print the pid of the already exited/released task.

But it should not loop forever (the subject), and it should not print the same pid twice (ignoring pid reuse, of course).

Quoted 7 lines

And, afaics, there are no problems with rcu magic per se, next_thread() always returns the task_struct we can safely dereference. The only problem is that while_each_thread() assumes that sooner or later next_thread() must reach the starting point, g.

(zap_threads() is different, it must not miss a thread with ->mm
we are going to dump, but it holds mmap_sem).

Indeed, the tough part is figuring out when you are done given that things can come and go at will. Some additional tricks, in no particular order:

1. Always start at the group leader. Of course, the group leader
is probably permitted to leave any time it wants to, so this is not sufficient in and of itself.

2. Maintain a separate task structure that flags the head of the
list. This separate structure is freed one RCU grace period following the disappearance of the current group leader. This should be quite robust, but "holy overhead, Batman!!!" (Apologies for the American pop culture reference, but nothing else seemed appropriate.)

Quoted 57 lines · Paul E. McKenney

Quoted 17 lines · Oleg Nesterov

Quoted 9 lines · Paul E. McKenney

Quoted 7 lines · Oleg Nesterov

	#define XXX(t)	({
		struct task_struct *__prev = t;
		t = next_thread(t);
		t != g && t != __prev;
	})

Quoted 6 lines

Quoted 2 lines

	#define while_each_thread(g, t) \
		while (XXX(t))

Isn't the above vulnerable to a pthread_create() immediately following the offending exec()? Especially if the task doing the traversal is preempted?

Quoted 5 lines

Yes, thanks!

Quoted 1 line

here are some techniques that might (or might not) help:

To simplify, let's consider the concrete example,

Sounds very good!

Quoted 3 lines

	rcu_read_lock();

	g = t = returns_the_rcu_safe_task_struct_ptr();

This returns a pointer to the task struct of the current thread? Or might this return a pointer some other thread's task struct?

Quoted 13 lines

	do {
		printk("%d\n", t->pid);
	} while_each_thread(g, t);

	rcu_read_unlock();

Whatever we do, without tasklist/siglock this can obviously race with fork/exit/exec. It is OK to miss a thread, or print the pid of the already exited/released task.

But it should not loop forever (the subject), and it should not print the same pid twice (ignoring pid reuse, of course).

Quoted 7 lines

And, afaics, there are no problems with rcu magic per se, next_thread() always returns the task_struct we can safely dereference. The only problem is that while_each_thread() assumes that sooner or later next_thread() must reach the starting point, g.

(zap_threads() is different, it must not miss a thread with ->mm
we are going to dump, but it holds mmap_sem).

Indeed, the tough part is figuring out when you are done given that things can come and go at will. Some additional tricks, in no particular order:

Quoted 3 lines

1. Always start at the group leader. Of course, the group leader
is probably permitted to leave any time it wants to, so this is not sufficient in and of itself.

No. The group leader must exist as long as the group exists. Modulo de_thread weirdness. The group_leader can be a zombie but it can not go away completely.

Quoted 6 lines

2. Maintain a separate task structure that flags the head of the
list. This separate structure is freed one RCU grace period following the disappearance of the current group leader. This should be quite robust, but "holy overhead, Batman!!!" (Apologies for the American pop culture reference, but nothing else seemed appropriate.)

That is roughly what we have in the group leader right now.

Indeed, the tough part is figuring out when you are done given that things can come and go at will. Some additional tricks, in no particular order:

1. Always start at the group leader.

2. Maintain a separate task structure that flags the head of the
list. This separate structure is freed one RCU grace period following the disappearance of the current group leader.

Quoted 1 line

Well, another field in task_struct...

Yeah, would be good to avoid this. Not sure it can be avoided, though.

Quoted 6 lines

Quoted 3 lines

o Do the de_thread() incrementally. So if the list is tasks A,
B, and C, in that order, and if we are de-thread()ing B, then make A's pointer refer to C,

This breaks while_each_thread() under tasklist/siglock. It must see all unhashed tasks.

Could de_thread() hold those locks in order to avoid that breakage?

Quoted 5 lines · Paul E. McKenney

Indeed, the tough part is figuring out when you are done given that things can come and go at will. Some additional tricks, in no particular order:

1. Always start at the group leader.

We can't. We have users which start at the arbitrary thread.

Quoted 3 lines

2. Maintain a separate task structure that flags the head of the
list. This separate structure is freed one RCU grace period following the disappearance of the current group leader.

Even simpler, we can just add list_head into signal_struct. I thought about this, but this breaks thread_group_empty (this is fixeable) and, again, I'd like very much to avoid adding new fields into task_struct or signal_struct.

Quoted 3 lines

Quoted 1 line

Well, another field in task_struct...

Yeah, would be good to avoid this. Not sure it can be avoided, though.

Why? I think next_thread_careful() from

	http://marc.info/?l=linux-kernel&m=127714242731448

should work.

If the caller holds tasklist or siglock, this change has no effect.

If the caller does while_each_thread() under rcu_read_lock(), then it is OK to break the loop earlier than we do now. The lockless while_each_thread() works in a "best effort" manner anyway, if it races with exit_group() or exec() it can miss some/most/all sub-threads (including the new leader) with or without this change.

Yes, zap_threads() needs additional fixes. But I think it is better to complicate a couple of lockless callers (or just change them to take tasklist) which must not miss an "interesting" thread.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

o Do the de_thread() incrementally. So if the list is tasks A,
B, and C, in that order, and if we are de-thread()ing B, then make A's pointer refer to C,

This breaks while_each_thread() under tasklist/siglock. It must see all unhashed tasks.

Could de_thread() hold those locks in order to avoid that breakage?

How can it hold, say, siglock? We need to wait a grace period. To clarify. de_thread() kills all threads except the group_leader, so we have only 2 threads: group_leader A and B.

If we add synchronize_rcu() before release_task(leader) (as Roland suggested), then we don't need to change A's pointer. This probably fixes while_each_thread() in the common case. But this disallows the tricks like rcu_lock_break().

And. Whatever we do with de_thread(), this can't fix the lockless while_each_thread(not_a_group_leader, t). I do not know if there is any user which does this though. fastpath_timer_check()->thread_group_cputimer() does this, but this is wrong and we already have the patch which removes it.

Quoted 16 lines · Oleg Nesterov

Quoted 2 lines · Paul E. McKenney

Yeah, would be good to avoid this. Not sure it can be avoided, though.

Why? I think next_thread_careful() from

	http://marc.info/?l=linux-kernel&m=127714242731448

should work.

If the caller holds tasklist or siglock, this change has no effect.

If the caller does while_each_thread() under rcu_read_lock(), then it is OK to break the loop earlier than we do now. The lockless while_each_thread() works in a "best effort" manner anyway, if it races with exit_group() or exec() it can miss some/most/all sub-threads (including the new leader) with or without this change.

Quoted 3 lines

Yes, zap_threads() needs additional fixes. But I think it is better to complicate a couple of lockless callers (or just change them to take tasklist) which must not miss an "interesting" thread.

Is it the case that creating a new group leader from an existing group always destroys the old group? It certainly is the case for exec().

Anyway, if creating a new thread group implies destroying the old one, and if the thread group leader cannot be concurrently creating a new thread group and traversing the old one, then yes, I do believe your code at http://marc.info/?l=linux-kernel&m=127714242731448 will work.

Assuming that the call to next_thread_careful(t) in the definition of while_each_thread() is replaced with next_thread_careful(g,t).

And give or take memory barriers.

The implied memory barrier mentioned in the comment in your example code is the spin_lock_irqsave() and spin_unlock_irqrestore()

Quoted 5 lines

And. Whatever we do with de_thread(), this can't fix the lockless while_each_thread(not_a_group_leader, t). I do not know if there is any user which does this though. fastpath_timer_check()->thread_group_cputimer() does this, but this is wrong and we already have the patch which removes it.

Indeed. Suppose that the starting task is the one immediately preceding the task group leader. You get a pointer to the task in question and traverse to the next task (the task group leader), during which time the thread group leader does exec() and maybe a pthread_create() or two. Oops! You are now now traversing the wrong thread group!

There are ways of fixing this, but all the ones I know of require more fields in the task structure,

so best if we don't need to start somewhere
other than a group leader.

Quoted 18 lines · Paul E. McKenney

Quoted 16 lines · Oleg Nesterov

Quoted 2 lines · Paul E. McKenney

Yeah, would be good to avoid this. Not sure it can be avoided, though.

Why? I think next_thread_careful() from

	http://marc.info/?l=linux-kernel&m=127714242731448

should work.

If the caller holds tasklist or siglock, this change has no effect.

If the caller does while_each_thread() under rcu_read_lock(), then it is OK to break the loop earlier than we do now. The lockless while_each_thread() works in a "best effort" manner anyway, if it races with exit_group() or exec() it can miss some/most/all sub-threads (including the new leader) with or without this change.

Quoted 6 lines

Quoted 3 lines

Yes, zap_threads() needs additional fixes. But I think it is better to complicate a couple of lockless callers (or just change them to take tasklist) which must not miss an "interesting" thread.

Is it the case that creating a new group leader from an existing group always destroys the old group? It certainly is the case for exec().

Yes. And only exec() can change the leader.

Quoted 7 lines

Anyway, if creating a new thread group implies destroying the old one, and if the thread group leader cannot be concurrently creating a new thread group and traversing the old one, then yes, I do believe your code at http://marc.info/?l=linux-kernel&m=127714242731448 will work.

Assuming that the call to next_thread_careful(t) in the definition of while_each_thread() is replaced with next_thread_careful(g,t).

Great.

Quoted 4 lines

And give or take memory barriers.

The implied memory barrier mentioned in the comment in your example code is the spin_lock_irqsave() and spin_unlock_irqrestore()

Argh. No, no, no. I meant unlock + lock, not lock + unlock. But when I look at __change_pid() now I do not see the 2nd lock() after unlock(pidmap_lock). I was wrong, thanks.

And, even if I was correct it is not nice to rely on the internals of detach_pid(). If we use next_thread_careful(), __unhash_process() needs wmb.

Thanks!

Quoted 11 lines

Quoted 5 lines

And. Whatever we do with de_thread(), this can't fix the lockless while_each_thread(not_a_group_leader, t). I do not know if there is any user which does this though. fastpath_timer_check()->thread_group_cputimer() does this, but this is wrong and we already have the patch which removes it.

Indeed. Suppose that the starting task is the one immediately preceding the task group leader. You get a pointer to the task in question and traverse to the next task (the task group leader), during which time the thread group leader does exec() and maybe a pthread_create() or two. Oops! You are now now traversing the wrong thread group!

Yes, but there is another more simple and much more feasible race. while_each_thread(non_leader, t) will loop forever if non_leader simply exits and passes __unhash_process(). After that next_thread(t) can never return non_leader.

Quoted 2 lines

There are ways of fixing this, but all the ones I know of require more fields in the task structure,

Just in case, I hope that next_thread_careful() handles this case too.

Quoted 2 lines

so best if we don't need to start somewhere
other than a group leader.

(I assume, you mean the lockless case)

I am not sure. Because it is not trivial to enforce this rule even if we add a fat comment. Please look at check_hung_uninterruptible_tasks(). It does do_each_thread/while_each_thread and thus it always starts at the leader. But in fact this is not true due to rcu_lock_break().

Or proc_task_readdir(). It finds the leader, does get_task_struct(leader) and drops rcu lock. After that first_tid() takes rcu_read_lock() again and does the open coded while_each_thread(). At first glance this case looks fine, "for (pos = leader; nr > 0; --nr)" can't loop forever. But in fact it is not:

- proc_task_readdir() finds the leader L, does get_task_struct() and drops rcu lock.

Suppose that filp->f_pos >= 2.
Suppose also that the previous tid cached in filp->f_version is no longer valid.

The caller is preempted.

- a sub-thread T does exec, and does release_task(L).

But L->thread_group->next == T, so far everything is good

- T spawns other sub-threads (so that get_nr_threads() > nr) and exits.

- grace period passes, T's task_struct is freed/unmapped/reused

- proc_task_readdir() resumes and calls first_tid(L).

next_thread(L) returns T == nowhere

It is very possible that I missed something here, my only point is that I think it would be safer to assume nothing about the leaderness.

3. The thread-group leader might do pthread_exit(), removing itself
from the thread group -- and might do so while the hapless reader is referencing that thread.

But isn't this prohibited? Or is it really legal to do a pthread_create() to create a new thread and then have the parent thread call pthread_exit()? Not something I would consider trying in my own code! Well, I might, just to be perverse, but... ;-)

Quoted 9 lines · Paul E. McKenney

3. The thread-group leader might do pthread_exit(), removing itself
from the thread group -- and might do so while the hapless reader is referencing that thread.

But isn't this prohibited? Or is it really legal to do a pthread_create() to create a new thread and then have the parent thread call pthread_exit()? Not something I would consider trying in my own code! Well, I might, just to be perverse, but... ;-)

I believe SUS allows the main thread to explicitly call pthread_exit(), leaving the other threads to run. If the main() routine just returns then it implicitly calls exit().

Quoted 13 lines · Chris Friesen

Quoted 9 lines · Paul E. McKenney

3. The thread-group leader might do pthread_exit(), removing itself
from the thread group -- and might do so while the hapless reader is referencing that thread.

But isn't this prohibited? Or is it really legal to do a pthread_create() to create a new thread and then have the parent thread call pthread_exit()? Not something I would consider trying in my own code! Well, I might, just to be perverse, but... ;-)

Quoted 3 lines

I believe SUS allows the main thread to explicitly call pthread_exit(), leaving the other threads to run. If the main() routine just returns then it implicitly calls exit().

Correct.

But, to clarify, if the main thread does pthread_exit() (sys_exit, actually), it won't be removed from the group. It will be zombie until all other threads exit.

Quoted 20 lines · Oleg Nesterov

Quoted 13 lines · Chris Friesen

Quoted 9 lines · Paul E. McKenney

3. The thread-group leader might do pthread_exit(), removing itself
from the thread group -- and might do so while the hapless reader is referencing that thread.

But isn't this prohibited? Or is it really legal to do a pthread_create() to create a new thread and then have the parent thread call pthread_exit()? Not something I would consider trying in my own code! Well, I might, just to be perverse, but... ;-)

Quoted 3 lines

I believe SUS allows the main thread to explicitly call pthread_exit(), leaving the other threads to run. If the main() routine just returns then it implicitly calls exit().

Correct.

Quoted 3 lines

But, to clarify, if the main thread does pthread_exit() (sys_exit, actually), it won't be removed from the group. It will be zombie until all other threads exit.

That we don't cleanup that zombie leaders is unfortunate really, it means we have the entire de_thread special case. But short fixing libpthread to not make bad assumptions there is little we can do about it really.

I'm only half following this conversation.

If what we are looking for is a stable list_head that won't disappear on us we should be able to put one in sighand_struct or signal_struct (I forget which is which at the moment) and have a list_head that lives for the life of the longest living thread, and that won't get messed up by things like de_thread, and then next_thread could simply return NULL when we hit the end of the list.

Oleg Nesterov <[email protected]> writes:

If what we are looking for is a stable list_head that won't disappear on us we should be able to put one in sighand_struct or signal_struct (I forget which is which at the moment) and have a list_head that lives for the life of the longest living thread, and that won't get messed up by things like de_thread, and then next_thread could simply return NULL when we hit the end of the list.

That we don't cleanup that zombie leaders is unfortunate really, it means we have the entire de_thread special case. But short fixing libpthread to not make bad assumptions there is little we can do about it really.

I'm only half following this conversation.

First, what "bad things" can happen to a reader scanning a thread group?

1. The thread-group leader might do exec(), destroying the old
list and forming a new one. In this case, we want any readers to stop scanning.

2. Some other thread might do exec(), destroying the old list and
forming a new one. In this case, we also want any readers to stop scanning.

3. The thread-group leader might do pthread_exit(), removing itself
from the thread group -- and might do so while the hapless reader is referencing that thread.

4. Some other thread might do pthread_exit(), removing itself
from the thread group, and again might do so while the hapless reader is referencing that thread. In this case, we want the hapless reader to continue scanning the remainder of the thread group.

5. The thread-group leader might do exit(), destroying the old
list without forming a new one. In this case, we want any readers to stop scanning.

6. Some other thread might do exit(), destroying the old list
without forming a new one. In this case, we also want any readers to stop scanning.

Quoted 6 lines

First, what "bad things" can happen to a reader scanning a thread group?

1. The thread-group leader might do exec(), destroying the old
list and forming a new one. In this case, we want any readers to stop scanning.

This doesn't do anything different (for these concerns) from just all the other threads happening to exit right before the exec. There is no "destroying the old" and "forming the new", it's just that all the other threads are convinced to die now. There is no problem here.

Quoted 3 lines

2. Some other thread might do exec(), destroying the old list and
forming a new one. In this case, we also want any readers to stop scanning.

Again, the list is not really destroyed, just everybody dies. What is different here is that ->group_leader changes. This is the only time that ever happens. Moreover, it's the only time that a task that was previously pointed to by any ->group_leader can be reaped before the rest of the group has already been reaped first (and thus the thread_group made a singleton).

Quoted 3 lines

3. The thread-group leader might do pthread_exit(), removing itself
from the thread group -- and might do so while the hapless reader is referencing that thread.

This is called the delay_group_leader() case. It doesn't happen in a way that has the problems you are concerned with. The group_leader remains in EXIT_ZOMBIE state and can't be reaped until all the other threads have been reaped. There is no time at which any thread in the group is in any hashes or accessible by any means after the (final) group_leader is reaped.

Quoted 5 lines

4. Some other thread might do pthread_exit(), removing itself
from the thread group, and again might do so while the hapless reader is referencing that thread. In this case, we want the hapless reader to continue scanning the remainder of the thread group.

This is the most normal case (and #1 is effectively just this repeated by every thread in parallel).

Quoted 3 lines

5. The thread-group leader might do exit(), destroying the old
list without forming a new one. In this case, we want any readers to stop scanning.

All this means is everybody is convinced to die, and the group_leader dies too. It is not discernibly different from #6.

Quoted 3 lines

6. Some other thread might do exit(), destroying the old list
without forming a new one. In this case, we also want any readers to stop scanning.

This just means everybody is convinced to die and is not materially different from each individual thread all happening to die at the same time.

You've described all these cases as "we want any readers to stop scanning". That is far too strong, and sounds like some kind of guaranteed synchronization, which does not and need not exist. Any reader that needs a dead thread to be off the list holds siglock and/or tasklist_lock. For the casual readers that only use rcu_read_lock, we only "want any readers' loops eventually to terminate and never to dereference stale pointers". That's why normal RCU listiness is generally fine.

The only problem we have is in #2. This is only a problem because readers' loops may be using the old ->group_leader pointer as the anchor for their circular-list round-robin loop. Once the former leader is removed from the list, that loop termination condition can never be met.

The reason for my "destroying the old" and "forming the new" is the possibility of someone doing proc_task_readdir() when the group leader does exec(), which causes all to die, and then the new process does pthread_create(), forming a new thread group. Because proc_task_readdir() neither holds a lock nor stays in an RCU read-side critical section for the full /proc scan, the old group might really be destroyed from the reader's point of view.

Quoted 2 lines · Oleg Nesterov

It is very possible that I missed something here, my only point is that I think it would be safer to assume nothing about the leaderness.

It is past time that I list out my assumptions more carefully. ;-)

First, what "bad things" can happen to a reader scanning a thread group?

1. The thread-group leader might do exec(), destroying the old
list and forming a new one. In this case, we want any readers to stop scanning.

2. Some other thread might do exec(), destroying the old list and
forming a new one. In this case, we also want any readers to stop scanning.

3. The thread-group leader might do pthread_exit(), removing itself
from the thread group

4. Some other thread might do pthread_exit(), removing itself
from the thread group, and again might do so while the hapless reader is referencing that thread. In this case, we want the hapless reader to continue scanning the remainder of the thread group.

5. The thread-group leader might do exit(), destroying the old
list without forming a new one. In this case, we want any readers to stop scanning.

6. Some other thread might do exit(), destroying the old list
without forming a new one. In this case, we also want any readers to stop scanning.

Anything else I might be missing?

Quoted 5 lines · Paul E. McKenney

Quoted 2 lines · Oleg Nesterov

It is very possible that I missed something here, my only point is that I think it would be safer to assume nothing about the leaderness.

Quoted 4 lines

It is past time that I list out my assumptions more carefully. ;-)

First, what "bad things" can happen to a reader scanning a thread group?

(I assume you mean the lockless case)

Currently, the only bad thing is that while_each_thread(g) can loop forever if we race with exec(), or exit() if g is not leader.

And, to simplify, let's consider the same example again

	t = g;
	do {
		printk("pid %d\n", t->pid);
	} while_each_thread(g, t);

Quoted 3 lines

1. The thread-group leader might do exec(), destroying the old
list and forming a new one. In this case, we want any readers to stop scanning.

I'd say, it is not that we want to stop scanning, it is OK to stop scanning after we printed g->pid

Quoted 3 lines

2. Some other thread might do exec(), destroying the old list and
forming a new one. In this case, we also want any readers to stop scanning.

The same.

If the code above runs under for_each_process(g) or it did "g = find_task_by_pid(tgid)", we will see either new or old leader and print its pid at least.

Quoted 2 lines

3. The thread-group leader might do pthread_exit(), removing itself
from the thread group

No. It can exit, but it won't be removed from thread group. It will be zombie untill all sub-threads disappear.

Quoted 5 lines

4. Some other thread might do pthread_exit(), removing itself
from the thread group, and again might do so while the hapless reader is referencing that thread. In this case, we want the hapless reader to continue scanning the remainder of the thread group.

Yes.

But, if that thread was used as a starting point g, then

before the patch: loop forever
after the patch: break

Quoted 7 lines

5. The thread-group leader might do exit(), destroying the old
list without forming a new one. In this case, we want any readers to stop scanning.

6. Some other thread might do exit(), destroying the old list
without forming a new one. In this case, we also want any readers to stop scanning.

Yes. But again, it is fine to print more pids as far as we know it is safe to iterate over the exiting thread group. However, next_thread_careful() can stop earlier compared to next_thread(). Either way, we can miss none/some/most/all threads if we race with exit_group().

Quoted 1 line

Anything else I might be missing?

I think this is all.

Quoted 10 lines · Oleg Nesterov

Quoted 6 lines · Paul E. McKenney

4. Some other thread might do pthread_exit(), removing itself
from the thread group, and again might do so while the hapless reader is referencing that thread. In this case, we want the hapless reader to continue scanning the remainder of the thread group.

Yes.

Quoted 4 lines

But, if that thread was used as a starting point g, then

before the patch: loop forever
after the patch: break

So it is OK to skip some of the other threads in this case, even though they were present throughout the whole procedure?

Quoted 12 lines · Paul E. McKenney

Quoted 10 lines · Oleg Nesterov

Quoted 6 lines · Paul E. McKenney

4. Some other thread might do pthread_exit(), removing itself
from the thread group, and again might do so while the hapless reader is referencing that thread. In this case, we want the hapless reader to continue scanning the remainder of the thread group.

Yes.

Quoted 7 lines

Quoted 4 lines

But, if that thread was used as a starting point g, then

before the patch: loop forever
after the patch: break

So it is OK to skip some of the other threads in this case, even though they were present throughout the whole procedure?

I think, yes. We can miss them in any case, they can go away before while_each_thread(g, t) starts the scan.

If g == group_leader (old or new), then we should notice this thread at least.

Otherwise we can miss them all, with or without next_thread_careful().

Quoted 5 lines · Oleg Nesterov

Quoted 3 lines · Paul E. McKenney

So it is OK to skip some of the other threads in this case, even though they were present throughout the whole procedure?

Quoted 7 lines

I think, yes. We can miss them in any case, they can go away before while_each_thread(g, t) starts the scan.

If g == group_leader (old or new), then we should notice this thread at least.

Otherwise we can miss them all, with or without next_thread_careful().

Just to be sure that we are actually talking about the same scenario...

Suppose that a task group is lead by 2908 and has member 2909, 2910, 2911, and 2912. Suppose that 2910 does pthread_exit() just as some other task is "ls"ing the relevant /proc entry. Is it really OK for "ls" to show 2909 but not 2911 and 2912, even though 2911 and 2912 were alive and kicking the entire time?

Quoted 15 lines · Paul E. McKenney

Quoted 5 lines · Oleg Nesterov

Quoted 3 lines · Paul E. McKenney

So it is OK to skip some of the other threads in this case, even though they were present throughout the whole procedure?

Quoted 7 lines

I think, yes. We can miss them in any case, they can go away before while_each_thread(g, t) starts the scan.

If g == group_leader (old or new), then we should notice this thread at least.

Otherwise we can miss them all, with or without next_thread_careful().

Quoted 7 lines

Just to be sure that we are actually talking about the same scenario...

Suppose that a task group is lead by 2908 and has member 2909, 2910, 2911, and 2912. Suppose that 2910 does pthread_exit() just as some other task is "ls"ing the relevant /proc entry. Is it really OK for "ls" to show 2909 but not 2911 and 2912, even though 2911 and 2912 were alive and kicking the entire time?

Confused.

Let's return to

	do
		printk("%d\n", t->pid);
	while_each_thread(g, t);

for the moment.

In that case, if g != 2910 (the exiting thread) we will print all pids, except we can miss 2910. With or without next_thread_careful().

Only if we start at g == 2910, then

current code: print 2910, then spin forever printing
other pids

next_thread_careful: stop printing when we notice that 2910
was unhashed.

So, yes, in this case we can miss all other threads.

As for "ls"ing the relevant /proc entry. proc_task_readdir() is complicated, it can drop rcu lock, sleep, etc. But basically it mimics while_each_thread() logic. Let's assume that proc_task_fill_cache() never fails.

proc_task_readdir() always starts at the group_leader, 2908. So, with or without next_thread_careful() we can only miss the exiting 2910.

But (again, unless I missed something) the current code can race with exec, and s/next_thread/next_thread_careful/ in first_tid() can fix the race. (just in case, we can fix it differently).

But, of course, if you do "ls /proc/2910/task" instead of "ls /proc/2908/task" you can miss _all_ threads if 2910 exits before proc_task_readdir() finds its leader, 2908. Again, this is with or without next_thread_careful().

Paul, please let me know if I misunderstood your concerns, or if I missed something.

Quoted 3 lines · Oleg Nesterov

Paul, please let me know if I misunderstood your concerns, or if I missed something.

Thank you very much for laying this out completely! I was having a hard time believing that it was OK to miss threads in the "ls /proc/2910/task" case. But of course similar issues can arise when running "ls" on a directory with lots of files that are coming and going quickly in the meantime, I guess.

And if proc_task_fill_cache() fails, we can miss
tasks as well, correct?

Given all this, I believe that your fix really does work.

(add cc's)

Hmm. Once I sent this patch, I suddenly realized with horror that while_each_thread() is NOT safe under rcu_read_lock(). Both do_each_thread/while_each_thread or do/while_each_thread() can race with exec().

Yes, it is safe to do next_thread() or next_task(). But:

       #define while_each_thread(g, t) \
               while ((t = next_thread(t)) != g)

suppose that t is not the group leader, and it does de_thread() and then release_task(g). After that next_thread(t) returns t, not g, and the loop will never stop.

I _really_ hope I missed something, will recheck tomorrow with the fresh head. Still I'd like to share my concerns...

If I am right, probably we can fix this, something like

       #define while_each_thread(g, t) \
               while ((t = next_thread(t)) != g && pid_alive(g))

[we can't do while (!thread_group_leadr(t = next_thread(t)))]. but this needs barrires, and we should validate the callers anyway.

Or, perhaps,

       #define XXX(t)  ({
               struct task_struct *__prev = t;
               t = next_thread(t);
               t != g && t != __prev;
       })

       #define while_each_thread(g, t) \
               while (XXX(t))

Please tell me I am wrong!

Oleg.

Quoted 63 lines · Oleg Nesterov

check_hung_uninterruptible_tasks()->rcu_lock_break() introduced by "softlockup: check all tasks in hung_task" commit ce9dbe24 looks absolutely wrong.

      - rcu_lock_break() does put_task_struct(). If the task has exited it is not safe to even read its ->state, nothing protects this task_struct.

      - The TASK_DEAD checks are wrong too. Contrary to the comment, we can't use it to check if the task was unhashed. It can be unhashed without TASK_DEAD, or it can be valid with TASK_DEAD.

        For example, an autoreaping task can do release_task(current) long before it sets TASK_DEAD in do_exit().

        Or, a zombie task can have ->state == TASK_DEAD but release_task() was not called, and in this case we must not break the loop.

Change this code to check pid_alive() instead, and do this before we drop the reference to the task_struct.

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

---

 kernel/hung_task.c |   11 +++++++----

 1 file changed, 7 insertions(+), 4 deletions(-)

kernel/hung_task.c 2010-06-18 20:06:11.000000000 +0200 renamed from kernel/hung_task.c~CHT_FIX_RCU_LOCK_BREAK  2009-12-18 19:05:38.000000000 +0100 +7 -4

@@ -113,15 +113,20 @@ static void check_hung_task(struct task_
   * For preemptible RCU it is sufficient to call rcu_read_unlock in order
   * exit the grace period. For classic RCU, a reschedule is required.
   */
-static void rcu_lock_break(struct task_struct *g, struct task_struct *t)
+static bool rcu_lock_break(struct task_struct *g, struct task_struct *t)
  {
+     bool can_cont;
+
       get_task_struct(g);
       get_task_struct(t);
       rcu_read_unlock();
       cond_resched();
       rcu_read_lock();
+     can_cont = pid_alive(g) && pid_alive(t);
       put_task_struct(t);
       put_task_struct(g);
+
+     return can_cont;
  }
 
  /*

@@ -148,9 +153,7 @@ static void check_hung_uninterruptible_t
                       goto unlock;
               if (!--batch_count) {
                       batch_count = HUNG_TASK_BATCHING;
-                     rcu_lock_break(g, t);
-                     /* Exit if t or g was unhashed during refresh. */
-                     if (t->state == TASK_DEAD || g->state == TASK_DEAD)
+                     if (!rcu_lock_break(g, t))
                               goto unlock;
               }
               /* use "==" to skip the TASK_KILLABLE tasks waiting on NFS */

Quoted 16 lines · Oleg Nesterov

(add cc's)

Hmm. Once I sent this patch, I suddenly realized with horror that while_each_thread() is NOT safe under rcu_read_lock(). Both do_each_thread/while_each_thread or do/while_each_thread() can race with exec().

Yes, it is safe to do next_thread() or next_task(). But:

       #define while_each_thread(g, t) \
               while ((t = next_thread(t)) != g)

suppose that t is not the group leader, and it does de_thread() and then release_task(g). After that next_thread(t) returns t, not g, and the loop will never stop.

Quoted 3 lines

I _really_ hope I missed something, will recheck tomorrow with the fresh head. Still I'd like to share my concerns...

Yep. You're right. Not sure what I was thinking. This is only case where do_each_thread
is protected by an rcu_read_lock. All others, correctly use read_lock.

Quoted 5 lines

If I am right, probably we can fix this, something like

       #define while_each_thread(g, t) \
               while ((t = next_thread(t)) != g && pid_alive(g))

This seems like a reasonable approach. Maybe call it:

while_each_thread_maybe_rcu() :)

This makes hung_task a little less useful for failure fencing since this (and rcu_lock_break)
increases the potential for never examining all threads but its still a nice lightweight diagnostic
for finding bugs.

Quoted 17 lines · Mandeep Baines

Quoted 16 lines · Oleg Nesterov

(add cc's)

Hmm. Once I sent this patch, I suddenly realized with horror that while_each_thread() is NOT safe under rcu_read_lock(). Both do_each_thread/while_each_thread or do/while_each_thread() can race with exec().

Yes, it is safe to do next_thread() or next_task(). But:

       #define while_each_thread(g, t) \
               while ((t = next_thread(t)) != g)

suppose that t is not the group leader, and it does de_thread() and then release_task(g). After that next_thread(t) returns t, not g, and the loop will never stop.

Quoted 7 lines

Quoted 3 lines

I _really_ hope I missed something, will recheck tomorrow with the fresh head. Still I'd like to share my concerns...

Yep. You're right. Not sure what I was thinking. This is only case where do_each_thread
is protected by an rcu_read_lock. All others, correctly use read_lock.

cgroup does too.
taskstats also uses rcu with while_each_threads, and may be some others.

It's not your fault, theses iterators are supposed to be rcu safe, we are just encountering a bad race :)

Quoted 8 lines

Quoted 7 lines

If I am right, probably we can fix this, something like

       #define while_each_thread(g, t) \
               while ((t = next_thread(t)) != g && pid_alive(g))

Quoted 3 lines

This seems like a reasonable approach. Maybe call it:

while_each_thread_maybe_rcu() :)

Hmm, no while_each_thread must really be rcu_safe.

Quoted 8 lines

This makes hung_task a little less useful for failure fencing since this (and rcu_lock_break)
increases the potential for never examining all threads but its still a nice lightweight diagnostic
for finding bugs.

In fact may be we could just drop the rcu break, people who really care about latencies can use the preemptable version.

I know the worry is more about delaying too much the grace period if we walk a too long task list, but I don't think it's really a problem.

Quoted 4 lines · Oleg Nesterov

I _really_ hope I missed something, will recheck tomorrow with the fresh head. Still I'd like to share my concerns...

Yep. You're right. Not sure what I was thinking. This is only case where do_each_thread
is protected by an rcu_read_lock. All others, correctly use read_lock.

check_hung_uninterruptible_tasks()->rcu_lock_break() introduced by "softlockup: check all tasks in hung_task" commit ce9dbe24 looks absolutely wrong.

- rcu_lock_break() does put_task_struct(). If the task has exited it is not safe to even read its ->state, nothing protects this task_struct.

- The TASK_DEAD checks are wrong too. Contrary to the comment, we can't use it to check if the task was unhashed. It can be unhashed without TASK_DEAD, or it can be valid with TASK_DEAD.

For example, an autoreaping task can do release_task(current) long before it sets TASK_DEAD in do_exit().

Or, a zombie task can have ->state == TASK_DEAD but release_task() was not called, and in this case we must not break the loop.

Change this code to check pid_alive() instead, and do this before we drop the reference to the task_struct.

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

check_hung_uninterruptible_tasks()->rcu_lock_break() introduced by "softlockup: check all tasks in hung_task" commit ce9dbe24 looks absolutely wrong.

- rcu_lock_break() does put_task_struct(). If the task has exited it is not safe to even read its ->state, nothing protects this task_struct.

- The TASK_DEAD checks are wrong too. Contrary to the comment, we can't use it to check if the task was unhashed. It can be unhashed without TASK_DEAD, or it can be valid with TASK_DEAD.

For example, an autoreaping task can do release_task(current) long before it sets TASK_DEAD in do_exit().

Or, a zombie task can have ->state == TASK_DEAD but release_task() was not called, and in this case we must not break the loop.

Change this code to check pid_alive() instead, and do this before we drop the reference to the task_struct.

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

---