[patch] Race condition in usermodehelper.

[patch] Race condition in usermodehelper.

[Martin Schwidefsky]

From: Martin Schwidefsky <schwidefsky@de.ibm.com>

[patch] Race condition in usermodehelper.

There is a race between call_usermodehelper_keys, __call_usermodehelper
and wait_for_helper. It should only happen if preemption is enabled or
on a virtualized system.

If the cpu is preempted or put to sleep by the hypervisor in
__call_usermodehelper between the creation of the wait_for_helper
thread and the second check on sub_info->wait, the whole execution
of wait_for_helper including the complete call and the continuation
after the wait_for_completion in call_usermodehelper_keys can have
happened before __call_usermodehelper checks sub_info->wait for the
second time. Since sub_info can already have been clobbered,
sub_info->wait could be zero and complete is called a second time
with an invalid argument. This has happened on s390. It took me only
three days to find out ..

Thanks to Arnd Bergmann for his help to spot this bug.

Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
---

 kernel/kmod.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff -urpN linux-2.6/kernel/kmod.c linux-2.6-patched/kernel/kmod.c
--- linux-2.6/kernel/kmod.c	2006-09-15 12:17:52.000000000 +0200
+++ linux-2.6-patched/kernel/kmod.c	2006-09-15 12:18:00.000000000 +0200
@@ -196,12 +196,13 @@ static int wait_for_helper(void *data)
 static void __call_usermodehelper(void *data)
 {
 	struct subprocess_info *sub_info = data;
+	int wait = sub_info->wait;
 	pid_t pid;
 
 	/* CLONE_VFORK: wait until the usermode helper has execve'd
 	 * successfully We need the data structures to stay around
 	 * until that is done.  */
-	if (sub_info->wait)
+	if (wait)
 		pid = kernel_thread(wait_for_helper, sub_info,
 				    CLONE_FS | CLONE_FILES | SIGCHLD);
 	else
@@ -211,7 +212,7 @@ static void __call_usermodehelper(void *
 	if (pid < 0) {
 		sub_info->retval = pid;
 		complete(sub_info->complete);
-	} else if (!sub_info->wait)
+	} else if (!wait)
 		complete(sub_info->complete);
 }
 

Re: [patch] Race condition in usermodehelper.

[Andrew Morton]

On Fri, 15 Sep 2006 12:46:54 +0200
Martin Schwidefsky <schwidefsky@de.ibm.com> wrote:

> From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> 
> [patch] Race condition in usermodehelper.
> 
> There is a race between call_usermodehelper_keys, __call_usermodehelper
> and wait_for_helper. It should only happen if preemption is enabled or
> on a virtualized system.
> 
> If the cpu is preempted or put to sleep by the hypervisor in
> __call_usermodehelper between the creation of the wait_for_helper
> thread and the second check on sub_info->wait, the whole execution
> of wait_for_helper including the complete call and the continuation
> after the wait_for_completion in call_usermodehelper_keys can have
> happened before __call_usermodehelper checks sub_info->wait for the
> second time. Since sub_info can already have been clobbered,
> sub_info->wait could be zero and complete is called a second time
> with an invalid argument. This has happened on s390. It took me only
> three days to find out ..

You mean three days work?

If so, I owe you a big apology, because an identical patch has been in -mm
for over a month.  I guess I didn't appreciate its significance.

Shall expedite.

Re: [patch] Race condition in usermodehelper.

[Martin Schwidefsky]

On Fri, 2006-09-15 at 09:26 -0700, Andrew Morton wrote:
> > [patch] Race condition in usermodehelper.
>
> You mean three days work?

Unfortunately yes. You really have to hit the machine hard to provoke
this oops. All I had to work with was a dump that showed me the content
of the memory after it crashed.

> If so, I owe you a big apology, because an identical patch has been in -mm
> for over a month.  I guess I didn't appreciate its significance.

Well, I could have looked in -mm after the first suspicion that there is
something wrong with the kernel module loader. It would have saved me 2
of the 3 days.. will remember for the next debug session.

> Shall expedite.

Please do.

-- 
blue skies,
  Martin.

Martin Schwidefsky
Linux for zSeries Development & Services
IBM Deutschland Entwicklung GmbH

"Reality continues to ruin my life." - Calvin.

Re: [patch] Race condition in usermodehelper.

[Andrew Morton]

On Fri, 15 Sep 2006 19:08:56 +0200
Martin Schwidefsky <schwidefsky@de.ibm.com> wrote:

> On Fri, 2006-09-15 at 09:26 -0700, Andrew Morton wrote:
> > > [patch] Race condition in usermodehelper.
> >
> > You mean three days work?
> 
> Unfortunately yes. You really have to hit the machine hard to provoke
> this oops. All I had to work with was a dump that showed me the content
> of the memory after it crashed.

Sigh.  Sorry.

> > If so, I owe you a big apology, because an identical patch has been in -mm
> > for over a month.  I guess I didn't appreciate its significance.
> 
> Well, I could have looked in -mm after the first suspicion that there is
> something wrong with the kernel module loader. It would have saved me 2
> of the 3 days.. will remember for the next debug session.

No, you have absolutely no reason to expect that an oops fix is languishing
in -mm when we're at -rc6.  I reviewed the patch, agreed with it, queued it
in the wrong place in the series file and promptly forgot about it.  That's
literally three seconds inattention here costing three days over there.  I hate
me.

At least I got to do it to someone else for once.  But three days!!

Re: [patch] Race condition in usermodehelper.

[Adrian Bunk]

Thanks, applied to 2.6.16 (with a note that Kenneth Lee also sent the 
same patch independently).

cu
Adrian

On Fri, Sep 15, 2006 at 12:46:54PM +0200, Martin Schwidefsky wrote:
> From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> 
> [patch] Race condition in usermodehelper.
> 
> There is a race between call_usermodehelper_keys, __call_usermodehelper
> and wait_for_helper. It should only happen if preemption is enabled or
> on a virtualized system.
> 
> If the cpu is preempted or put to sleep by the hypervisor in
> __call_usermodehelper between the creation of the wait_for_helper
> thread and the second check on sub_info->wait, the whole execution
> of wait_for_helper including the complete call and the continuation
> after the wait_for_completion in call_usermodehelper_keys can have
> happened before __call_usermodehelper checks sub_info->wait for the
> second time. Since sub_info can already have been clobbered,
> sub_info->wait could be zero and complete is called a second time
> with an invalid argument. This has happened on s390. It took me only
> three days to find out ..
> 
> Thanks to Arnd Bergmann for his help to spot this bug.
> 
> Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
> ---
> 
>  kernel/kmod.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff -urpN linux-2.6/kernel/kmod.c linux-2.6-patched/kernel/kmod.c
> --- linux-2.6/kernel/kmod.c	2006-09-15 12:17:52.000000000 +0200
> +++ linux-2.6-patched/kernel/kmod.c	2006-09-15 12:18:00.000000000 +0200
> @@ -196,12 +196,13 @@ static int wait_for_helper(void *data)
>  static void __call_usermodehelper(void *data)
>  {
>  	struct subprocess_info *sub_info = data;
> +	int wait = sub_info->wait;
>  	pid_t pid;
>  
>  	/* CLONE_VFORK: wait until the usermode helper has execve'd
>  	 * successfully We need the data structures to stay around
>  	 * until that is done.  */
> -	if (sub_info->wait)
> +	if (wait)
>  		pid = kernel_thread(wait_for_helper, sub_info,
>  				    CLONE_FS | CLONE_FILES | SIGCHLD);
>  	else
> @@ -211,7 +212,7 @@ static void __call_usermodehelper(void *
>  	if (pid < 0) {
>  		sub_info->retval = pid;
>  		complete(sub_info->complete);
> -	} else if (!sub_info->wait)
> +	} else if (!wait)
>  		complete(sub_info->complete);
>  }
>