Revert "kmod: handle UMH_WAIT_PROC from system unbound workqueue"

This reverts commit bb304a5c6fc63d8506cd9741a3a5f35b73605625.

Because this patch leads to kthread zombies.

call_usermodehelper_exec_sync() does fork() + wait() with "unignored" SIGCHLD. What we have missed is that this worker thread can have other children previously forked by call_usermodehelper_exec_work() without UMH_WAIT_PROC. If such a child exits in between it becomes a zombie and nobody can reap it (unless/until this worker thread exits too).

Hello,

I noticed by accident the kworker zombies on my testing machine. Can't reproduce (although I think it won't be hard to make a test-case), but I think the reason is clear, see the changelog.

We could fix this by using kthread_create() if !UMH_WAIT_PROC, but imo it would be better to revert this change at least for now.

If we really want to avoid the extra kernel_thread(), we can make another patch which also avoids sys_wait4() and the games with SIGCHLD; we can rely on wait_chldexit.

Quoted 4 lines · Oleg Nesterov

This reverts commit bb304a5c6fc63d8506cd9741a3a5f35b73605625.

Because this patch leads to kthread zombies.

Quoted 5 lines

call_usermodehelper_exec_sync() does fork() + wait() with "unignored" SIGCHLD. What we have missed is that this worker thread can have other children previously forked by call_usermodehelper_exec_work() without UMH_WAIT_PROC. If such a child exits in between it becomes a zombie and nobody can reap it (unless/until this worker thread exits too).

I missed that indeed.

But then when we create the async thread with UMH_NO_WAIT, who reaps it? It's created by the workqueue which never exits.

And on others cases, who buries the sync thread?

Quoted 6 lines · Frederic Weisbecker

Quoted 4 lines · Oleg Nesterov

This reverts commit bb304a5c6fc63d8506cd9741a3a5f35b73605625.

Because this patch leads to kthread zombies.

Quoted 7 lines

Quoted 5 lines

call_usermodehelper_exec_sync() does fork() + wait() with "unignored" SIGCHLD. What we have missed is that this worker thread can have other children previously forked by call_usermodehelper_exec_work() without UMH_WAIT_PROC. If such a child exits in between it becomes a zombie and nobody can reap it (unless/until this worker thread exits too).

I missed that indeed.

Heh me too ;)

Quoted 3 lines

But then when we create the async thread with UMH_NO_WAIT, who reaps it? It's created by the workqueue which never exits.

It is auto-reaped because SIGCHILD is ignored. And this is why bb304a5c6fc6 is wrong; it can die while UMH_WAIT_PROC case waits for the new child.

Quoted 2 lines

And on others cases, who buries the sync thread?

The same.

Please see V2 I sent. I'll try to send more cleanups soon to make this all more explicit.

Oleg.

call_usermodehelper_exec_sync() does fork() + wait() with "unignored" SIGCHLD. What we have missed is that this worker thread can have other children previously forked by call_usermodehelper_exec_work() without UMH_WAIT_PROC. If such a child exits in between it becomes a zombie and nobody can reap it (unless/until this worker thread exits too).

Change the !UMH_WAIT_PROC case to use CLONE_PARENT.

Note: this is only first step. All PF_KTHREAD tasks, even created by kernel_thread() should have ->parent == kthreadd by default.

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

---
 kernel/kmod.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

kernel/kmod.c +0 -0

index da98d05..e7185a2 100644

kernel/kmod.c +1 -1

 		pid = kernel_thread(call_usermodehelper_exec_async, sub_info,
-				    SIGCHLD);
+				    CLONE_PARENT | SIGCHLD);
 		if (pid < 0) {
 			sub_info->retval = pid;
 			umh_complete(sub_info);

Signature · 1 line

2.4.3

Quoted 5 lines · Oleg Nesterov

call_usermodehelper_exec_sync() does fork() + wait() with "unignored" SIGCHLD. What we have missed is that this worker thread can have other children previously forked by call_usermodehelper_exec_work() without UMH_WAIT_PROC. If such a child exits in between it becomes a zombie and nobody can reap it (unless/until this worker thread exits too).

I think we should elaborate a tiny bit the last sentence here:

"When the parent masks SIGCHLD, a child autoreaps itself, this is what we expect from !UMH_WAIT_PROC children.

kernel/kmod.c static void call_usermodehelper_exec_work(struct work_struct *work) · line 334

Quoted 10 lines

kernel/kmod.c +5 -1

@@ -327,9 +327,13 @@ static void call_usermodehelper_exec_work(struct work_struct *work)
 		call_usermodehelper_exec_sync(sub_info);
 	} else {
 		pid_t pid;
-
+		/*
+		 * Use CLONE_PARENT to reparent it to kthreadd; we do not
+		 * want to pollute current->children, in particular because
+		 * call_usermodehelper_exec_sync() assumes it is empty.
+		 */

IMHO, that too should get some more details. Maybe:

+ /*
+ * Use CLONE_PARENT to reparent it to kthreadd. We need a parent
+ * that always ignore SIGCHLD such that the child always autoreaps
+ * as expected.
+ */

Quoted 9 lines · Frederic Weisbecker

Quoted 5 lines · Oleg Nesterov

call_usermodehelper_exec_sync() does fork() + wait() with "unignored" SIGCHLD. What we have missed is that this worker thread can have other children previously forked by call_usermodehelper_exec_work() without UMH_WAIT_PROC. If such a child exits in between it becomes a zombie and nobody can reap it (unless/until this worker thread exits too).

I think we should elaborate a tiny bit the last sentence here:

OK, I'll try to update the changelog and send v2...

Quoted 2 lines

"When the parent masks SIGCHLD, a child autoreaps itself, this is what we expect from !UMH_WAIT_PROC children.

                      ^^^^^^^^^^^^^^^^^^^^^^^

Not really. This is what we _usually_ expect from kernel_thread().

Quoted 14 lines

kernel/kmod.c static void call_usermodehelper_exec_work(struct work_struct *work) · line 334

Quoted 11 lines

kernel/kmod.c +5 -1

@@ -327,9 +327,13 @@ static void call_usermodehelper_exec_work(struct work_struct *work)
 		call_usermodehelper_exec_sync(sub_info);
 	} else {
 		pid_t pid;
-
+		/*
+		 * Use CLONE_PARENT to reparent it to kthreadd; we do not
+		 * want to pollute current->children, in particular because
+		 * call_usermodehelper_exec_sync() assumes it is empty.
+		 */

IMHO, that too should get some more details. Maybe:

Quoted 5 lines

+ /*
+ * Use CLONE_PARENT to reparent it to kthreadd. We need a parent
+ * that always ignore SIGCHLD such that the child always autoreaps
+ * as expected.
+ */

Well, OK...

But I would like to keep "we do not want to pollute current->children" because this the goal of the next cleanups.

Plus I don't really like "parent that always ignore SIGCHLD". To remind, we can also remove kernel_sigaction() and sys_wait4() from call_usermodehelper_exec_sync().

Plus I have other changes in mind,
kernel_thread() should not rely on SIGCHLD at all. The auto-reapable kernel threads should run with ->exit_signal == 0.

Finally, this comment should go into kernel_thread() eventually.

Quoted 3 lines · Oleg Nesterov

But I would like to keep "we do not want to pollute current->children" because this the goal of the next cleanups.

I don't mind it, it's just that it's not obvious _why_ we don't want to pollute it.