Commit 95e0d86b authored by Sebastian Andrzej Siewior's avatar Sebastian Andrzej Siewior Committed by Linus Torvalds

Revert "kmod: fix race in usermodehelper code"

This reverts commit c02e3f36 ("kmod: fix race in usermodehelper code")

The patch is wrong.  UMH_WAIT_EXEC is called with VFORK what ensures
that the child finishes prior returing back to the parent.  No race.

In fact, the patch makes it even worse because it does the thing it
claims not do:

 - It calls ->complete() on UMH_WAIT_EXEC

 - the complete() callback may de-allocated subinfo as seen in the
   following call chain:

    [<c009f904>] (__link_path_walk+0x20/0xeb4) from [<c00a094c>] (path_walk+0x48/0x94)
    [<c00a094c>] (path_walk+0x48/0x94) from [<c00a0a34>] (do_path_lookup+0x24/0x4c)
    [<c00a0a34>] (do_path_lookup+0x24/0x4c) from [<c00a158c>] (do_filp_open+0xa4/0x83c)
    [<c00a158c>] (do_filp_open+0xa4/0x83c) from [<c009ba90>] (open_exec+0x24/0xe0)
    [<c009ba90>] (open_exec+0x24/0xe0) from [<c009bfa8>] (do_execve+0x7c/0x2e4)
    [<c009bfa8>] (do_execve+0x7c/0x2e4) from [<c0026a80>] (kernel_execve+0x34/0x80)
    [<c0026a80>] (kernel_execve+0x34/0x80) from [<c004b514>] (____call_usermodehelper+0x130/0x148)
    [<c004b514>] (____call_usermodehelper+0x130/0x148) from [<c0024858>] (kernel_thread_exit+0x0/0x8)

   and the path pointer was NULL.  Good that ARM's kernel_execve()
   doesn't check the pointer for NULL or else I wouldn't notice it.

The only race there might be is with UMH_NO_WAIT but it is too late for
me to investigate it now.  UMH_WAIT_PROC could probably also use VFORK
and we could save one exec.  So the only race I see is with UMH_NO_WAIT
and recent scheduler changes where the child does not always run first
might have trigger here something but as I said, it is late....
Signed-off-by: default avatarSebastian Andrzej Siewior <sebastian@breakpoint.cc>
Acked-by: default avatarNeil Horman <nhorman@tuxdriver.com>
Signed-off-by: default avatarLinus Torvalds <torvalds@linux-foundation.org>
parent 0dd52d0d
...@@ -143,7 +143,6 @@ struct subprocess_info { ...@@ -143,7 +143,6 @@ struct subprocess_info {
static int ____call_usermodehelper(void *data) static int ____call_usermodehelper(void *data)
{ {
struct subprocess_info *sub_info = data; struct subprocess_info *sub_info = data;
enum umh_wait wait = sub_info->wait;
int retval; int retval;
BUG_ON(atomic_read(&sub_info->cred->usage) != 1); BUG_ON(atomic_read(&sub_info->cred->usage) != 1);
...@@ -185,14 +184,10 @@ static int ____call_usermodehelper(void *data) ...@@ -185,14 +184,10 @@ static int ____call_usermodehelper(void *data)
*/ */
set_user_nice(current, 0); 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); retval = kernel_execve(sub_info->path, sub_info->argv, sub_info->envp);
/* Exec failed? */ /* Exec failed? */
if (wait != UMH_WAIT_EXEC) sub_info->retval = retval;
sub_info->retval = retval;
do_exit(0); do_exit(0);
} }
...@@ -271,14 +266,16 @@ static void __call_usermodehelper(struct work_struct *work) ...@@ -271,14 +266,16 @@ static void __call_usermodehelper(struct work_struct *work)
switch (wait) { switch (wait) {
case UMH_NO_WAIT: case UMH_NO_WAIT:
case UMH_WAIT_EXEC:
break; break;
case UMH_WAIT_PROC: case UMH_WAIT_PROC:
if (pid > 0) if (pid > 0)
break; break;
sub_info->retval = pid; sub_info->retval = pid;
break; /* FALLTHROUGH */
case UMH_WAIT_EXEC:
complete(sub_info->complete);
} }
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment