cgroup: fix a race condition in manipulating tsk->cg_list

PATCH In review Li Zefan 6 participants 24 messages Apr 17, 2008 9 messages not in the archive

Share & tools

Andrew Morton

2008-04-17 04:11 UTC #1 lore raw

Quoted 27 lines · On Thu, 17 Apr 2008 11:37:15 +0800 Li Zefan…

When I ran a test program to fork mass processes and at the same time 'cat /cgroup/tasks', I got the following oops:

------------[ cut here ]------------
kernel BUG at lib/list_debug.c:72!
invalid opcode: 0000 [#1] SMP
Pid: 4178, comm: a.out Not tainted (2.6.25-rc9 #72)
...
Call Trace:
 [<c044a5f9>] ? cgroup_exit+0x55/0x94
 [<c0427acf>] ? do_exit+0x217/0x5ba
 [<c0427ed7>] ? do_group_exit+0.65/0x7c
 [<c0427efd>] ? sys_exit_group+0xf/0x11
 [<c0404842>] ? syscall_call+0x7/0xb
 [<c05e0000>] ? init_cyrix+0x2fa/0x479
...
EIP: [<c04df671>] list_del+0x35/0x53 SS:ESP 0068:ebc7df4
---[ end trace caffb7332252612b ]---

Fixing recursive fault but reboot is needed!

After digging into the code and debugging, I finlly found out a race situation:

				do_exit()
				  ->cgroup_exit()
				    ->if (!list_empty(&tsk->cg_list))
				        list_del(&tsk->cg_list);

Quoted 5 lines

cgroup_iter_start()
->cgroup_enable_task_cg_list()
    ->list_add(&tsk->cg_list, ..);
In this case the list won't be deleted though the process has exited.

I don't fully understand the race. Both paths hold css_set_lock.

Can you describe it in more detail please?

Quoted 23 lines

We got two bug reports in the past, which seem to be the same bug as this one:
	http://lkml.org/lkml/2008/3/5/332
	http://lkml.org/lkml/2007/10/17/224
Actually sometimes I got oops on list_del, sometimes oops on list_add. And I can change my test program a bit to trigger other oops.
The patch has been tested both on x86_32 and x86_64.
Signed-off-by Li Zefan <[email protected]>
---
 kernel/cgroup.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)
kernel/cgroup.c +0 -0
index 2727f92..6d8de05 100644
@@ -1722,7 +1722,12 @@ void cgroup_enable_task_cg_lists(void)
 	use_task_css_set_links = 1;
 	do_each_thread(g, p) {
 		task_lock(p);

kernel/cgroup.c void cgroup_enable_task_cg_lists(void) · line 1733

Quoted 10 lines

kernel/cgroup.c +6 -1

-		if (list_empty(&p->cg_list))
+		/*
+		 * We should check if the process is exiting, otherwise
+		 * it will race with cgroup_exit() in that the list
+		 * entry won't be deleted though the process has exited.
+		 */
+		if (!(p->flags & PF_EXITING) && list_empty(&p->cg_list))
 			list_add(&p->cg_list, &p->cgroups->tasks);
 		task_unlock(p);
 	} while_each_thread(g, p);

Don't think I understand the fix either :(

afacit the task at *p could set PF_EXITING immediately after this code has tested PF_EXITING and then the task at *p could proceed until we hit the same race (whatever that is).

Perhaps taking p->sighand->siglock would fix that up, but that's just a guess at this stage.

A message that is not in the archive (sent off-list or not yet received): 20080416211144.a38f6fc0.akpm-de/[email protected]

Paul Menage

2008-04-17 04:17 UTC #3 lore raw

Quoted 4 lines · Andrew Morton

I don't fully understand the race. Both paths hold css_set_lock.
Can you describe it in more detail please?

Task A starts exiting, passes the check for unlinking current->cg_list.

Before it completely exits task B does the very first cgroup_iter_begin() call (via reading a cgroups tasks file) which links all tasks in to their css_set objects via tsk->cg_list.

Then task A finishes exiting and is freed, but doesn't unlink from the cg_list.

Quoted 4 lines

afacit the task at *p could set PF_EXITING immediately after this code has tested PF_EXITING and then the task at *p could proceed until we hit the same race (whatever that is).

The important fact there is that the task sets PF_EXITING *before* it checks whether it needs to unlink from current->cg_list.

Paul

Paul Menage

2008-04-17 04:17 UTC #4 lore raw

In reply to Andrew Morton

Quoted 4 lines · Andrew Morton

I don't fully understand the race. Both paths hold css_set_lock.
Can you describe it in more detail please?

Task A starts exiting, passes the check for unlinking current->cg_list.

Before it completely exits task B does the very first cgroup_iter_begin() call (via reading a cgroups tasks file) which links all tasks in to their css_set objects via tsk->cg_list.

Then task A finishes exiting and is freed, but doesn't unlink from the cg_list.

Quoted 4 lines

afacit the task at *p could set PF_EXITING immediately after this code has tested PF_EXITING and then the task at *p could proceed until we hit the same race (whatever that is).

The important fact there is that the task sets PF_EXITING *before* it checks whether it needs to unlink from current->cg_list.

Paul

A message that is not in the archive (sent off-list or not yet received): 6599ad830804162117w14364b7cg20d3694ffdfeb867-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Andrew Morton

2008-04-17 04:59 UTC #6 lore raw

Quoted 8 lines · On Wed, 16 Apr 2008 21:17:34 -0700 "Paul Menage"…

Quoted 4 lines · Andrew Morton
I don't fully understand the race. Both paths hold css_set_lock.
Can you describe it in more detail please?
Task A starts exiting, passes the check for unlinking current->cg_list.

So cgroup_exit() sees !list_empty(tsk->cg_list)

And the list_del() sets tsk->cg_list to LIST_POISON[12], which still means !list_empty(). Or we remove that debugging code and avoid writing to tsk->cg_list, and it _still_ is !list_empty().

Quoted 3 lines

Before it completely exits task B does the very first cgroup_iter_begin() call (via reading a cgroups tasks file) which links all tasks in to their css_set objects via tsk->cg_list.

But it won't link this task, because it's !list_empty().

Quoted 11 lines

Then task A finishes exiting and is freed, but doesn't unlink from the cg_list.
Quoted 4 lines
afacit the task at *p could set PF_EXITING immediately after this code has tested PF_EXITING and then the task at *p could proceed until we hit the same race (whatever that is).
The important fact there is that the task sets PF_EXITING *before* it checks whether it needs to unlink from current->cg_list.
Paul

Andrew Morton

2008-04-17 05:00 UTC #7 lore raw

In reply to Paul Menage

Quoted 8 lines · On Wed, 16 Apr 2008 21:17:34 -0700 "Paul Menage" <[email protected]> wrote:

Quoted 4 lines · Andrew Morton
I don't fully understand the race. Both paths hold css_set_lock.
Can you describe it in more detail please?
Task A starts exiting, passes the check for unlinking current->cg_list.

So cgroup_exit() sees !list_empty(tsk->cg_list)

And the list_del() sets tsk->cg_list to LIST_POISON[12], which still means !list_empty(). Or we remove that debugging code and avoid writing to tsk->cg_list, and it _still_ is !list_empty().

Quoted 3 lines

Before it completely exits task B does the very first cgroup_iter_begin() call (via reading a cgroups tasks file) which links all tasks in to their css_set objects via tsk->cg_list.

But it won't link this task, because it's !list_empty().

Quoted 11 lines

Then task A finishes exiting and is freed, but doesn't unlink from the cg_list.
Quoted 4 lines
afacit the task at *p could set PF_EXITING immediately after this code has tested PF_EXITING and then the task at *p could proceed until we hit the same race (whatever that is).
The important fact there is that the task sets PF_EXITING *before* it checks whether it needs to unlink from current->cg_list.
Paul

A message that is not in the archive (sent off-list or not yet received): 20080416215907.63d71409.akpm-de/[email protected]

Li Zefan

2008-04-17 05:10 UTC #9 lore raw

Quoted 11 lines · Andrew Morton

Quoted 6 lines · On Wed, 16 Apr 2008 21:17:34 -0700 "Paul Menage"…
Quoted 3 lines · Andrew Morton
I don't fully understand the race. Both paths hold css_set_lock.
Can you describe it in more detail please?
Task A starts exiting, passes the check for unlinking current->cg_list.
So cgroup_exit() sees !list_empty(tsk->cg_list)

cgroup_exit() sees list_empty(tsk->cg_list), then cgroup_enable_task_cg_list() links the task to the list, and then the task exited, so the list entry won't get deleted.

Quoted 21 lines

And the list_del() sets tsk->cg_list to LIST_POISON[12], which still means !list_empty(). Or we remove that debugging code and avoid writing to tsk->cg_list, and it _still_ is !list_empty().
Quoted 3 lines
Before it completely exits task B does the very first cgroup_iter_begin() call (via reading a cgroups tasks file) which links all tasks in to their css_set objects via tsk->cg_list.
But it won't link this task, because it's !list_empty().
Quoted 9 lines
Then task A finishes exiting and is freed, but doesn't unlink from the cg_list.
Quoted 3 lines
afacit the task at *p could set PF_EXITING immediately after this code has tested PF_EXITING and then the task at *p could proceed until we hit the same race (whatever that is).
The important fact there is that the task sets PF_EXITING *before* it checks whether it needs to unlink from current->cg_list.
Paul

Paul Menage

2008-04-17 05:20 UTC #10 lore raw

In reply to a message not in the archive

Quoted 4 lines · Andrew Morton

Quoted 2 lines
Task A starts exiting, passes the check for unlinking current->cg_list.
So cgroup_exit() sees !list_empty(tsk->cg_list)

We don't actually set up the links running through tsk->cg_list to the css_set objects until the first time someone calls cgroup_iter_begin()
- so anyone who never actually uses cgroups doesn't pay the list management overhead. So in this case, the list is empty.

Paul

Li Zefan

author

2008-04-17 05:12 UTC #11 lore raw

In reply to Andrew Morton

Quoted 11 lines · Andrew Morton

Quoted 6 lines · On Wed, 16 Apr 2008 21:17:34 -0700 "Paul Menage" <[email protected]> wrote:
Quoted 3 lines · Andrew Morton
I don't fully understand the race. Both paths hold css_set_lock.
Can you describe it in more detail please?
Task A starts exiting, passes the check for unlinking current->cg_list.
So cgroup_exit() sees !list_empty(tsk->cg_list)

cgroup_exit() sees list_empty(tsk->cg_list), then cgroup_enable_task_cg_list() links the task to the list, and then the task exited, so the list entry won't get deleted.

Quoted 21 lines

And the list_del() sets tsk->cg_list to LIST_POISON[12], which still means !list_empty(). Or we remove that debugging code and avoid writing to tsk->cg_list, and it _still_ is !list_empty().
Quoted 3 lines
Before it completely exits task B does the very first cgroup_iter_begin() call (via reading a cgroups tasks file) which links all tasks in to their css_set objects via tsk->cg_list.
But it won't link this task, because it's !list_empty().
Quoted 9 lines
Then task A finishes exiting and is freed, but doesn't unlink from the cg_list.
Quoted 3 lines
afacit the task at *p could set PF_EXITING immediately after this code has tested PF_EXITING and then the task at *p could proceed until we hit the same race (whatever that is).
The important fact there is that the task sets PF_EXITING *before* it checks whether it needs to unlink from current->cg_list.
Paul

A message that is not in the archive (sent off-list or not yet received): [email protected]

Andrew Morton

2008-04-17 05:16 UTC #13 lore raw

Quoted 8 lines · On Thu, 17 Apr 2008 13:10:33 +0800 Li Zefan…

Quoted 7 lines · Andrew Morton
Quoted 5 lines · On Wed, 16 Apr 2008 21:17:34 -0700 "Paul Menage"…
Quoted 3 lines · Andrew Morton
I don't fully understand the race. Both paths hold css_set_lock.
Can you describe it in more detail please?

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
Task A starts exiting, passes the check for unlinking current->cg_list.
So cgroup_exit() sees !list_empty(tsk->cg_list)
cgroup_exit() sees list_empty(tsk->cg_list), then cgroup_enable_task_cg_list() links the task to the list, and then the task exited, so the list entry won't get deleted.

OK.

Andrew Morton

2008-04-17 05:16 UTC #14 lore raw

In reply to Li Zefan

Quoted 8 lines · On Thu, 17 Apr 2008 13:10:33 +0800 Li Zefan <[email protected]> wrote:

Quoted 7 lines · Andrew Morton
Quoted 5 lines · On Wed, 16 Apr 2008 21:17:34 -0700 "Paul Menage" <[email protected]> wrote:
Quoted 3 lines · Andrew Morton
I don't fully understand the race. Both paths hold css_set_lock.
Can you describe it in more detail please?

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
Task A starts exiting, passes the check for unlinking current->cg_list.
So cgroup_exit() sees !list_empty(tsk->cg_list)
cgroup_exit() sees list_empty(tsk->cg_list), then cgroup_enable_task_cg_list() links the task to the list, and then the task exited, so the list entry won't get deleted.

OK.

Paul Menage

2008-04-17 05:20 UTC #15 lore raw

In reply to Andrew Morton

Quoted 4 lines · Andrew Morton

Quoted 2 lines
Task A starts exiting, passes the check for unlinking current->cg_list.
So cgroup_exit() sees !list_empty(tsk->cg_list)

Paul

A message that is not in the archive (sent off-list or not yet received): [email protected]

Paul Menage

2008-04-17 04:18 UTC #17 lore raw

Quoted 29 lines · Li Zefan

When I ran a test program to fork mass processes and at the same time
'cat /cgroup/tasks', I got the following oops:

 ------------[ cut here ]------------
 kernel BUG at lib/list_debug.c:72!
 invalid opcode: 0000 [#1] SMP
 Pid: 4178, comm: a.out Not tainted (2.6.25-rc9 #72)
 ...
 Call Trace:
  [<c044a5f9>] ? cgroup_exit+0x55/0x94
  [<c0427acf>] ? do_exit+0x217/0x5ba
  [<c0427ed7>] ? do_group_exit+0.65/0x7c
  [<c0427efd>] ? sys_exit_group+0xf/0x11
  [<c0404842>] ? syscall_call+0x7/0xb
  [<c05e0000>] ? init_cyrix+0x2fa/0x479
 ...
 EIP: [<c04df671>] list_del+0x35/0x53 SS:ESP 0068:ebc7df4
 ---[ end trace caffb7332252612b ]---
 Fixing recursive fault but reboot is needed!

After digging into the code and debugging, I finlly found out a race situation:

                                do_exit()
                                  ->cgroup_exit()
                                    ->if (!list_empty(&tsk->cg_list))
                                        list_del(&tsk->cg_list);

cgroup_iter_start()
->cgroup_enable_task_cg_list()

Quoted 8 lines

    ->list_add(&tsk->cg_list, ..);
In this case the list won't be deleted though the process has exited.
We got two bug reports in the past, which seem to be the same bug as this one:
        http://lkml.org/lkml/2008/3/5/332
        http://lkml.org/lkml/2007/10/17/224

Yes, that looks like it could be the same one - great. But this corruption can only be triggered the first time you cat a tasks file after a reboot, right? That would partly explain why it was hard to reproduce (at least, I had trouble).

My only thought about the downside of this is that an exiting task that gets stuck somewhere between setting PF_EXITING and calling cgroup_exit() won't show up in its cgroup's tasks file, since we'll enable cgroup links but skip it. I guess that's not a big deal.

Maybe it would be better to not do a cgroup_exit() until we're unhashed, so that cgroup_enable_task_cg_list() can't find the exiting task?

Paul

Paul Menage

2008-04-17 04:19 UTC #18 lore raw

In reply to Li Zefan

Quoted 29 lines · Li Zefan

When I ran a test program to fork mass processes and at the same time
'cat /cgroup/tasks', I got the following oops:

 ------------[ cut here ]------------
 kernel BUG at lib/list_debug.c:72!
 invalid opcode: 0000 [#1] SMP
 Pid: 4178, comm: a.out Not tainted (2.6.25-rc9 #72)
 ...
 Call Trace:
  [<c044a5f9>] ? cgroup_exit+0x55/0x94
  [<c0427acf>] ? do_exit+0x217/0x5ba
  [<c0427ed7>] ? do_group_exit+0.65/0x7c
  [<c0427efd>] ? sys_exit_group+0xf/0x11
  [<c0404842>] ? syscall_call+0x7/0xb
  [<c05e0000>] ? init_cyrix+0x2fa/0x479
 ...
 EIP: [<c04df671>] list_del+0x35/0x53 SS:ESP 0068:ebc7df4
 ---[ end trace caffb7332252612b ]---
 Fixing recursive fault but reboot is needed!

After digging into the code and debugging, I finlly found out a race situation:

                                do_exit()
                                  ->cgroup_exit()
                                    ->if (!list_empty(&tsk->cg_list))
                                        list_del(&tsk->cg_list);

cgroup_iter_start()
->cgroup_enable_task_cg_list()

Quoted 8 lines

    ->list_add(&tsk->cg_list, ..);
In this case the list won't be deleted though the process has exited.
We got two bug reports in the past, which seem to be the same bug as this one:
        http://lkml.org/lkml/2008/3/5/332
        http://lkml.org/lkml/2007/10/17/224

Maybe it would be better to not do a cgroup_exit() until we're unhashed, so that cgroup_enable_task_cg_list() can't find the exiting task?

Paul

A message that is not in the archive (sent off-list or not yet received): 6599ad830804162118g6b24d8ebq26b0d72133b0e19e-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Paul Menage

2008-04-17 04:28 UTC #20 lore raw

Quoted 6 lines · Paul Menage

My only thought about the downside of this is that an exiting task that gets stuck somewhere between setting PF_EXITING and calling cgroup_exit() won't show up in its cgroup's tasks file, since we'll enable cgroup links but skip it. I guess that's not a big deal.

How about this as an alternative approach? We can take advantage of the indirection in tsk->cgroups to create an additional distinguished css_set that indicates the task has passed the point of checking tsk->cg_list:

- create a new css_set, called exit_css_set; it has the same cgroup pointer set as init_css_set.

- in cgroup_exit(), set current->cgroups to &exit_css_set rather than &init_css_set

- in cgroup_enable_task_cg_list(), ignore any task where p->cgroups == &exit_css_set

That way we're synchronizing directly with the task_lock()-protected section in cgroup_exit(), rather than with the setting of PF_EXITING at the beginning of do_exit().

Paul

Li Zefan

2008-04-17 05:04 UTC #21 lore raw

In reply to a message not in the archive

Quoted 39 lines · Paul Menage

Quoted 29 lines · Li Zefan

When I ran a test program to fork mass processes and at the same time
'cat /cgroup/tasks', I got the following oops:

 ------------[ cut here ]------------
 kernel BUG at lib/list_debug.c:72!
 invalid opcode: 0000 [#1] SMP
 Pid: 4178, comm: a.out Not tainted (2.6.25-rc9 #72)
 ...
 Call Trace:
  [<c044a5f9>] ? cgroup_exit+0x55/0x94
  [<c0427acf>] ? do_exit+0x217/0x5ba
  [<c0427ed7>] ? do_group_exit+0.65/0x7c
  [<c0427efd>] ? sys_exit_group+0xf/0x11
  [<c0404842>] ? syscall_call+0x7/0xb
  [<c05e0000>] ? init_cyrix+0x2fa/0x479
 ...
 EIP: [<c04df671>] list_del+0x35/0x53 SS:ESP 0068:ebc7df4
 ---[ end trace caffb7332252612b ]---
 Fixing recursive fault but reboot is needed!

After digging into the code and debugging, I finlly found out a race situation:

                                do_exit()
                                  ->cgroup_exit()
                                    ->if (!list_empty(&tsk->cg_list))
                                        list_del(&tsk->cg_list);

cgroup_iter_start()
->cgroup_enable_task_cg_list()

Quoted 8 lines

    ->list_add(&tsk->cg_list, ..);
In this case the list won't be deleted though the process has exited.
We got two bug reports in the past, which seem to be the same bug as this one:
        http://lkml.org/lkml/2008/3/5/332
        http://lkml.org/lkml/2007/10/17/224

Quoted 5 lines

Yes, that looks like it could be the same one - great. But this corruption can only be triggered the first time you cat a tasks file after a reboot, right? That would partly explain why it was hard to reproduce (at least, I had trouble).

Right. I was lucky to trigger this and thus knew how to reproduce.

Quoted 5 lines

My only thought about the downside of this is that an exiting task that gets stuck somewhere between setting PF_EXITING and calling cgroup_exit() won't show up in its cgroup's tasks file, since we'll enable cgroup links but skip it. I guess that's not a big deal.

Agree. I think it won't be a problem.

Quoted 6 lines

Maybe it would be better to not do a cgroup_exit() until we're unhashed, so that cgroup_enable_task_cg_list() can't find the exiting task?
Paul

Paul Menage

2008-04-17 04:28 UTC #22 lore raw

In reply to Paul Menage

Quoted 6 lines · Paul Menage

My only thought about the downside of this is that an exiting task that gets stuck somewhere between setting PF_EXITING and calling cgroup_exit() won't show up in its cgroup's tasks file, since we'll enable cgroup links but skip it. I guess that's not a big deal.

- create a new css_set, called exit_css_set; it has the same cgroup pointer set as init_css_set.

- in cgroup_exit(), set current->cgroups to &exit_css_set rather than &init_css_set

- in cgroup_enable_task_cg_list(), ignore any task where p->cgroups == &exit_css_set

That way we're synchronizing directly with the task_lock()-protected section in cgroup_exit(), rather than with the setting of PF_EXITING at the beginning of do_exit().

Paul

Li Zefan

author

2008-04-17 05:06 UTC #23 lore raw

In reply to Paul Menage

Quoted 39 lines · Paul Menage

Quoted 29 lines · Li Zefan

When I ran a test program to fork mass processes and at the same time
'cat /cgroup/tasks', I got the following oops:

 ------------[ cut here ]------------
 kernel BUG at lib/list_debug.c:72!
 invalid opcode: 0000 [#1] SMP
 Pid: 4178, comm: a.out Not tainted (2.6.25-rc9 #72)
 ...
 Call Trace:
  [<c044a5f9>] ? cgroup_exit+0x55/0x94
  [<c0427acf>] ? do_exit+0x217/0x5ba
  [<c0427ed7>] ? do_group_exit+0.65/0x7c
  [<c0427efd>] ? sys_exit_group+0xf/0x11
  [<c0404842>] ? syscall_call+0x7/0xb
  [<c05e0000>] ? init_cyrix+0x2fa/0x479
 ...
 EIP: [<c04df671>] list_del+0x35/0x53 SS:ESP 0068:ebc7df4
 ---[ end trace caffb7332252612b ]---
 Fixing recursive fault but reboot is needed!

After digging into the code and debugging, I finlly found out a race situation:

                                do_exit()
                                  ->cgroup_exit()
                                    ->if (!list_empty(&tsk->cg_list))
                                        list_del(&tsk->cg_list);

cgroup_iter_start()
->cgroup_enable_task_cg_list()

Quoted 8 lines

    ->list_add(&tsk->cg_list, ..);
In this case the list won't be deleted though the process has exited.
We got two bug reports in the past, which seem to be the same bug as this one:
        http://lkml.org/lkml/2008/3/5/332
        http://lkml.org/lkml/2007/10/17/224

Quoted 5 lines

Yes, that looks like it could be the same one - great. But this corruption can only be triggered the first time you cat a tasks file after a reboot, right? That would partly explain why it was hard to reproduce (at least, I had trouble).

Right. I was lucky to trigger this and thus knew how to reproduce.

Quoted 5 lines

My only thought about the downside of this is that an exiting task that gets stuck somewhere between setting PF_EXITING and calling cgroup_exit() won't show up in its cgroup's tasks file, since we'll enable cgroup links but skip it. I guess that's not a big deal.

Agree. I think it won't be a problem.

Quoted 6 lines

Maybe it would be better to not do a cgroup_exit() until we're unhashed, so that cgroup_enable_task_cg_list() can't find the exiting task?
Paul

A message that is not in the archive (sent off-list or not yet received): [email protected]

Andrew Morton

2008-04-17 05:16 UTC #25 lore raw

Quoted 9 lines · On Thu, 17 Apr 2008 13:04:47 +0800 Li Zefan…

Quoted 6 lines
Yes, that looks like it could be the same one - great. But this corruption can only be triggered the first time you cat a tasks file after a reboot, right? That would partly explain why it was hard to reproduce (at least, I had trouble).
Right. I was lucky to trigger this and thus knew how to reproduce.

Quoted 11 lines

Quoted 5 lines
My only thought about the downside of this is that an exiting task that gets stuck somewhere between setting PF_EXITING and calling cgroup_exit() won't show up in its cgroup's tasks file, since we'll enable cgroup links but skip it. I guess that's not a big deal.
Agree. I think it won't be a problem.
Quoted 3 lines
Maybe it would be better to not do a cgroup_exit() until we're unhashed, so that cgroup_enable_task_cg_list() can't find the exiting task?

So we won't be doing what Paul suggested?

Andrew Morton

2008-04-17 05:17 UTC #26 lore raw

In reply to Li Zefan

Quoted 9 lines · On Thu, 17 Apr 2008 13:04:47 +0800 Li Zefan <[email protected]> wrote:

Quoted 6 lines
Yes, that looks like it could be the same one - great. But this corruption can only be triggered the first time you cat a tasks file after a reboot, right? That would partly explain why it was hard to reproduce (at least, I had trouble).
Right. I was lucky to trigger this and thus knew how to reproduce.

Quoted 11 lines

Quoted 5 lines
My only thought about the downside of this is that an exiting task that gets stuck somewhere between setting PF_EXITING and calling cgroup_exit() won't show up in its cgroup's tasks file, since we'll enable cgroup links but skip it. I guess that's not a big deal.
Agree. I think it won't be a problem.
Quoted 3 lines
Maybe it would be better to not do a cgroup_exit() until we're unhashed, so that cgroup_enable_task_cg_list() can't find the exiting task?

So we won't be doing what Paul suggested?

A message that is not in the archive (sent off-list or not yet received): 20080416221655.c73d219f.akpm-de/[email protected]

Paul Menage

2008-04-17 05:24 UTC #28 lore raw

Quoted 6 lines · Andrew Morton

Quoted 3 lines
Quoted 3 lines
Maybe it would be better to not do a cgroup_exit() until we're unhashed, so that cgroup_enable_task_cg_list() can't find the exiting task?
So we won't be doing what Paul suggested?

It's not as high a priority as Li's bug fix (which may be a good candidate for 2.6.25.1) but for the future I think I'll implement this distinguished css_set pointer for tasks that have finished cgroup_exit(), since I think it will make the similar synchronization in attach_task() cleaner, as well as cgroup_enable_task_cg_list().

Paul

Paul Menage

2008-04-17 05:24 UTC #29 lore raw

In reply to Andrew Morton

Quoted 6 lines · Andrew Morton

Quoted 3 lines
Quoted 3 lines
Maybe it would be better to not do a cgroup_exit() until we're unhashed, so that cgroup_enable_task_cg_list() can't find the exiting task?
So we won't be doing what Paul suggested?

Paul

A message that is not in the archive (sent off-list or not yet received): 6599ad830804162224s42ba221vea981fe34b30636a-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Li Zefan

2008-04-17 05:27 UTC #31 lore raw

Quoted 6 lines · Paul Menage

Quoted 4 lines · Andrew Morton
Quoted 3 lines
Quoted 3 lines
Maybe it would be better to not do a cgroup_exit() until we're unhashed, so that cgroup_enable_task_cg_list() can't find the exiting task?

Quoted 9 lines

Quoted 2 lines
So we won't be doing what Paul suggested?
It's not as high a priority as Li's bug fix (which may be a good candidate for 2.6.25.1) but for the future I think I'll implement this distinguished css_set pointer for tasks that have finished cgroup_exit(), since I think it will make the similar synchronization in attach_task() cleaner, as well as cgroup_enable_task_cg_list().

Yes, this approach sounds good to me. :)

Quoted 2 lines

Paul

Li Zefan

author

2008-04-17 05:28 UTC #32 lore raw

In reply to Paul Menage

Quoted 6 lines · Paul Menage

Quoted 4 lines · Andrew Morton
Quoted 3 lines
Quoted 3 lines
Maybe it would be better to not do a cgroup_exit() until we're unhashed, so that cgroup_enable_task_cg_list() can't find the exiting task?

Quoted 9 lines

Quoted 2 lines
So we won't be doing what Paul suggested?
It's not as high a priority as Li's bug fix (which may be a good candidate for 2.6.25.1) but for the future I think I'll implement this distinguished css_set pointer for tasks that have finished cgroup_exit(), since I think it will make the similar synchronization in attach_task() cleaner, as well as cgroup_enable_task_cg_list().

Yes, this approach sounds good to me. :)

Quoted 2 lines

Paul