[PATCH] kmod: Fix race in usermodehelper code

[PATCH] kmod: Fix race in usermodehelper code

[Neil Horman]

User Mode Helper: Fix race in UMH_WAIT_EXEC case

The user mode helper code has a race in it.  call_usermodehelper_exec takes an
allocated subprocess_info structure, which it passes to a workqueue, and then
passes it to a kernel thread which it creates, after which it calls complete to
signal to the caller of call_usremodehelper_exec that it can free the
subprocess_info struct.  But since we use that structure in the created thread,
we can't call complete from __call_usermodehelper, which is where we create the
kernel_thread.  We need to call complete from within the kernel thread and then
not use subprocess_info afterward in the case of UMH_WAIT_EXEC.  Tested
successfully by me.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

diff --git a/kernel/kmod.c b/kernel/kmod.c
index 7e95bed..afff87a 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -136,6 +136,7 @@ struct subprocess_info {
 static int ____call_usermodehelper(void *data)
 {
 	struct subprocess_info *sub_info = data;
+	enum umh_wait wait = sub_info->wait;
 	int retval;
 
 	BUG_ON(atomic_read(&sub_info->cred->usage) != 1);
@@ -177,10 +178,14 @@ static int ____call_usermodehelper(void *data)
 	 */
 	set_user_nice(current, 0);
 
+	if (wait == UMH_WAIT_EXEC)
+		complete(sub_info->complete);
+
 	retval = kernel_execve(sub_info->path, sub_info->argv, sub_info->envp);
 
 	/* Exec failed? */
-	sub_info->retval = retval;
+	if (wait != UMH_WAIT_EXEC)
+		sub_info->retval = retval;
 	do_exit(0);
 }
 
@@ -259,16 +264,14 @@ static void __call_usermodehelper(struct work_struct *work)
 
 	switch (wait) {
 	case UMH_NO_WAIT:
+	case UMH_WAIT_EXEC:
 		break;
 
 	case UMH_WAIT_PROC:
 		if (pid > 0)
 			break;
 		sub_info->retval = pid;
-		/* FALLTHROUGH */
-
-	case UMH_WAIT_EXEC:
-		complete(sub_info->complete);
+		break;
 	}
 }
 

Re: [PATCH] kmod: Fix race in usermodehelper code

[Andrew Morton]

On Tue, 30 Jun 2009 22:48:03 -0400
Neil Horman <nhorman@tuxdriver.com> wrote:

> User Mode Helper: Fix race in UMH_WAIT_EXEC case
> 
> The user mode helper code has a race in it.  call_usermodehelper_exec takes an
> allocated subprocess_info structure, which it passes to a workqueue, and then
> passes it to a kernel thread which it creates, after which it calls complete to
> signal to the caller of call_usremodehelper_exec that it can free the
> subprocess_info struct.  But since we use that structure in the created thread,
> we can't call complete from __call_usermodehelper, which is where we create the
> kernel_thread.  We need to call complete from within the kernel thread and then
> not use subprocess_info afterward in the case of UMH_WAIT_EXEC.  Tested
> successfully by me.
> 

<stares at the code for ten minutes>

Geeze that's getting complex, isn't it?  The patch looks OK.  I think.

I wonder why people aren't reporting this.  Perhaps CLONE_VFORK plus
child-runs-first?  (_does_ the chld run first?  We seem to have changed
it a couple of times)

Re: [PATCH] kmod: Fix race in usermodehelper code

[Neil Horman]

On Wed, Jul 01, 2009 at 01:56:31PM -0700, Andrew Morton wrote:
> On Tue, 30 Jun 2009 22:48:03 -0400
> Neil Horman <nhorman@tuxdriver.com> wrote:
> 
> > User Mode Helper: Fix race in UMH_WAIT_EXEC case
> > 
> > The user mode helper code has a race in it.  call_usermodehelper_exec takes an
> > allocated subprocess_info structure, which it passes to a workqueue, and then
> > passes it to a kernel thread which it creates, after which it calls complete to
> > signal to the caller of call_usremodehelper_exec that it can free the
> > subprocess_info struct.  But since we use that structure in the created thread,
> > we can't call complete from __call_usermodehelper, which is where we create the
> > kernel_thread.  We need to call complete from within the kernel thread and then
> > not use subprocess_info afterward in the case of UMH_WAIT_EXEC.  Tested
> > successfully by me.
> > 
> 
> <stares at the code for ten minutes>
> 
> Geeze that's getting complex, isn't it?  The patch looks OK.  I think.
> 
It really is.  I had initially considered solving the problem by eliminating the
completion queue, and substituting a refcounting mechanism, but as I wrote it, I
found that the memory allocation and freeing for subprocess_info was going to
need a bunch of augmentation too.  Talk about convoluted.... I decided a simple
fixing of the bug was probably for the best.

> I wonder why people aren't reporting this.  Perhaps CLONE_VFORK plus
> child-runs-first?  (_does_ the chld run first?  We seem to have changed
> it a couple of times)
> 
I think the child currently does run first, but as you note, its not always been
that way, and it might not be that way in the future, especially if the child
queues to a different cpu for whatever reason.  

Neil