[PATCH 0/1] Revert "kmod: handle UMH_WAIT_PROC from system unbound workqueue"

[PATCH 0/1] Revert "kmod: handle UMH_WAIT_PROC from system unbound workqueue"

[Oleg Nesterov]

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.

Oleg.

[PATCH 1/1] Revert "kmod: handle UMH_WAIT_PROC from system unbound workqueue"

[Oleg Nesterov]

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).

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/kmod.c | 44 ++++++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/kernel/kmod.c b/kernel/kmod.c
index da98d05..d38b2da 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -265,9 +265,15 @@ out:
 	do_exit(0);
 }
 
-/* Handles UMH_WAIT_PROC.  */
-static void call_usermodehelper_exec_sync(struct subprocess_info *sub_info)
+/*
+ * Handles UMH_WAIT_PROC. Our parent (unbound workqueue) might not be able to
+ * run enough instances to handle usermodehelper completions without blocking
+ * some other pending requests. That's why we use a kernel thread dedicated for
+ * that purpose.
+ */
+static int call_usermodehelper_exec_sync(void *data)
 {
+	struct subprocess_info *sub_info = data;
 	pid_t pid;
 
 	/* If SIGCLD is ignored sys_wait4 won't populate the status. */
@@ -281,9 +287,9 @@ static void call_usermodehelper_exec_sync(struct subprocess_info *sub_info)
 		 * Normally it is bogus to call wait4() from in-kernel because
 		 * wait4() wants to write the exit code to a userspace address.
 		 * But call_usermodehelper_exec_sync() always runs as kernel
-		 * thread (workqueue) and put_user() to a kernel address works
-		 * OK for kernel threads, due to their having an mm_segment_t
-		 * which spans the entire address space.
+		 * thread and put_user() to a kernel address works OK for kernel
+		 * threads, due to their having an mm_segment_t which spans the
+		 * entire address space.
 		 *
 		 * Thus the __user pointer cast is valid here.
 		 */
@@ -298,21 +304,19 @@ static void call_usermodehelper_exec_sync(struct subprocess_info *sub_info)
 			sub_info->retval = ret;
 	}
 
-	/* Restore default kernel sig handler */
-	kernel_sigaction(SIGCHLD, SIG_IGN);
-
 	umh_complete(sub_info);
+	do_exit(0);
 }
 
 /*
- * We need to create the usermodehelper kernel thread from a task that is affine
+ * This function doesn't strictly needs to be called asynchronously. But we
+ * need to create the usermodehelper kernel threads from a task that is affine
  * to an optimized set of CPUs (or nohz housekeeping ones) such that they
  * inherit a widest affinity irrespective of call_usermodehelper() callers with
  * possibly reduced affinity (eg: per-cpu workqueues). We don't want
  * usermodehelper targets to contend a busy CPU.
  *
- * Unbound workqueues provide such wide affinity and allow to block on
- * UMH_WAIT_PROC requests without blocking pending request (up to some limit).
+ * Unbound workqueues provide such wide affinity.
  *
  * Besides, workqueues provide the privilege level that caller might not have
  * to perform the usermodehelper request.
@@ -322,18 +326,18 @@ static void call_usermodehelper_exec_work(struct work_struct *work)
 {
 	struct subprocess_info *sub_info =
 		container_of(work, struct subprocess_info, work);
+	pid_t pid;
 
-	if (sub_info->wait & UMH_WAIT_PROC) {
-		call_usermodehelper_exec_sync(sub_info);
-	} else {
-		pid_t pid;
-
+	if (sub_info->wait & UMH_WAIT_PROC)
+		pid = kernel_thread(call_usermodehelper_exec_sync, sub_info,
+				    CLONE_FS | CLONE_FILES | SIGCHLD);
+	else
 		pid = kernel_thread(call_usermodehelper_exec_async, sub_info,
 				    SIGCHLD);
-		if (pid < 0) {
-			sub_info->retval = pid;
-			umh_complete(sub_info);
-		}
+
+	if (pid < 0) {
+		sub_info->retval = pid;
+		umh_complete(sub_info);
 	}
 }
 
-- 
2.4.3

Re: [PATCH 1/1] Revert "kmod: handle UMH_WAIT_PROC from system unbound workqueue"

[Frederic Weisbecker]

On Wed, Oct 14, 2015 at 08:52:09PM +0200, Oleg Nesterov wrote:
> 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).

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?

Re: [PATCH 1/1] Revert "kmod: handle UMH_WAIT_PROC from system unbound workqueue"

[Oleg Nesterov]

On 10/15, Frederic Weisbecker wrote:
>
> On Wed, Oct 14, 2015 at 08:52:09PM +0200, Oleg Nesterov wrote:
> > 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).
>
> I missed that indeed.

Heh me too ;)

> 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.

> 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.

Re: [PATCH 1/1] Revert "kmod: handle UMH_WAIT_PROC from system unbound workqueue"

[Frederic Weisbecker]

On Thu, Oct 15, 2015 at 05:18:19PM +0200, Oleg Nesterov wrote:
> On 10/15, Frederic Weisbecker wrote:
> >
> > On Wed, Oct 14, 2015 at 08:52:09PM +0200, Oleg Nesterov wrote:
> > > 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).
> >
> > I missed that indeed.
> 
> Heh me too ;)
> 
> > 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.

Oooh, that's subtle!

> 
> > 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.

Ok.

> Oleg.
> 

[PATCH 0/1] kmod: don't run async usermode helper as a child of kworker thread

[Oleg Nesterov]

Andrew, please drop

	revert-kmod-handle-umh_wait_proc-from-system-unbound-workqueue.patch

I sent yesterday. On a second thought we have a better solution.

On 10/14, Oleg Nesterov wrote:
>
> 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.

I changed my mind. I was worried about other workqueue callbacks
which could abuse kernel_thread() and populate kworker->children
even if we change call_usermodehelper_exec_work() to not do this.

But according to git-grep nobody does this. And this is good!
Because we can do more cleanups (will try to send "soon") to
ensure that all kthreads have parent == kthreadd.

And since the worker thread is already its child, we do not need
kthread_create(), we can just use CLONE_PARENT (which should be
later used by kernel_thread() by default).

> 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.

Yes, this probably makes sense too, but we can do this regardless.

Oleg.

Re: [PATCH 0/1] kmod: don't run async usermode helper as a child of kworker thread

[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).

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 <oleg@redhat.com>
---
 kernel/kmod.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/kmod.c b/kernel/kmod.c
index da98d05..e7185a2 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -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.
+		 */
 		pid = kernel_thread(call_usermodehelper_exec_async, sub_info,
-				    SIGCHLD);
+				    CLONE_PARENT | SIGCHLD);
 		if (pid < 0) {
 			sub_info->retval = pid;
 			umh_complete(sub_info);
-- 
2.4.3

Re: [PATCH 0/1] kmod: don't run async usermode helper as a child of kworker thread

[Frederic Weisbecker]

On Thu, Oct 15, 2015 at 04:37:57PM +0200, Oleg Nesterov wrote:
> 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. Now if such a child exits during
this unlucky window where the parent worker enabled SIGCHLD to wait for a sibling,
the autoreap will fail and the child then becomes a zombie because  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 <oleg@redhat.com>
> ---
>  kernel/kmod.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/kmod.c b/kernel/kmod.c
> index da98d05..e7185a2 100644
> --- a/kernel/kmod.c
> +++ b/kernel/kmod.c
> @@ -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.
 +		 */

Thanks!

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

Re: [PATCH 0/1] kmod: don't run async usermode helper as a child of kworker thread

[Oleg Nesterov]

On 10/15, Frederic Weisbecker wrote:
>
> On Thu, Oct 15, 2015 at 04:37:57PM +0200, Oleg Nesterov wrote:
> > 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...

> "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().

> > @@ -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.
>  +		 */

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.

Oleg.

Re: [PATCH 0/1] kmod: don't run async usermode helper as a child of kworker thread

[Frederic Weisbecker]

On Thu, Oct 15, 2015 at 06:32:17PM +0200, Oleg Nesterov wrote:
> On 10/15, Frederic Weisbecker wrote:
> >
> > On Thu, Oct 15, 2015 at 04:37:57PM +0200, Oleg Nesterov wrote:
> > > 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...
> 
> > "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().

Sure. But kmod does special things with signals.

> 
> > > @@ -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.
> >  +		 */
> 
> Well, OK...
> 
> 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. I think that's what is missing in the comment. At least I
couldn't deduce it by myself without you explaining.

> 
> 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().

Yeah that would be great. Something that lets us wait for a task completion
without bothering with signals.

> 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.

Ok.

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

Agreed, as long as reviewers really understand what we are doing in usermodehelper.
It took me several days to understand why we were doing things the way we did before
updating all the comments recently. The flags, the workqueues, the kernel threads...
Not much is intuitive there. Of course usermodehelper isn't doing intuitive things, hence why.

Re: [PATCH 0/1] kmod: don't run async usermode helper as a child of kworker thread

[Oleg Nesterov]

On 10/15, Frederic Weisbecker wrote:
>
> On Thu, Oct 15, 2015 at 06:32:17PM +0200, Oleg Nesterov wrote:
> >
> > 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.

Well, this connects to the "Note:" part of the changelog, I don't
this the comment can really explain this.

But note that this fix is not enough in the long term _unless_ we
ensure that another workueue callback can not call kernel_thread()
without CLONE_PARENT. And this is what I am going to do.

Nevermind, this is (almost) off-topic right now. Please review V2
I'll send in a minute, I tried to update the comment/changelog.

Oleg.

[PATCH v2 1/1] kmod: don't run async usermode helper as a child of kworker thread

[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
because auto-reaping only works if SIGCHLD is ignored, 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 <oleg@redhat.com>
---
 kernel/kmod.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/kmod.c b/kernel/kmod.c
index da98d05..0277d12 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -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, and we need a parent
+		 * that always ignores SIGCHLD to ensure auto-reaping.
+		 */
 		pid = kernel_thread(call_usermodehelper_exec_async, sub_info,
-				    SIGCHLD);
+				    CLONE_PARENT | SIGCHLD);
 		if (pid < 0) {
 			sub_info->retval = pid;
 			umh_complete(sub_info);
-- 
2.4.3