Re: [PATCH v3] kmod: Avoid deadlock by recursive kmod call.

[Andrew Morton <akpm@linux-foundation.org>]

On Fri, 6 Jan 2012 21:39:17 +0900
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:

> Tetsuo Handa wrote:
> > An example operation for triggering this deadlock:
> > 
> >   # : > /tmp/dummy
> >   # chmod 755 /tmp/dummy
> >   # echo /tmp/dummy > /proc/sys/kernel/hotplug
> >   # modprobe whatever
> 
> I wrote two patches. Please review.
> 
> ----- Patch A start -----
> [PATCH v3] kmod: Avoid deadlock by recursive kmod call.
> 
> The system deadlocks when call_usermodehelper(UMH_WAIT_EXEC) request triggered
> call_usermodehelper(UMH_WAIT_PROC) request.
> 
> This is because "khelper thread is waiting at wait_for_completion() in
> do_fork() since the worker thread was created with CLONE_VFORK flag" and
> "the worker thread cannot call complete() because do_execve() is blocked at
> UMH_WAIT_PROC request (e.g. request_module() from search_binary_handler())" and
> "the khelper thread cannot start processing UMH_WAIT_PROC request because
> the khelper thread is waiting at wait_for_completion() in do_fork()".
> 
> In order to avoid deadlock, do not try to call wait_for_completion() in
> call_usermodehelper_exec() if the worker thread was created by khelper thread
> with CLONE_VFORK flag.

I suppose we should fix this, although it's obscure.

The changelog doesn't tell people how the bug might occur, which is
bad.  I suggest adding this text to it:

: This bug was observed when using a corrupted /sbin/hotplug binary.  The
: corrupted binary caused a call to request_module("binfmt-0000"). 
: search_binary_handler() uses UMH_WAIT_EXEC.
: 
: An example operation for triggering this deadlock:
: 
:    # : > /tmp/dummy
:    # chmod 755 /tmp/dummy
:    # echo /tmp/dummy > /proc/sys/kernel/hotplug
:    # modprobe whatever

which might be inaccurate/inadequate.  I didn't try very hard.  Please
provide a complete description and analysis of the bug.

I prefer patch A - all that poking around at stack slots is hacky.

> --- linux-3.2.orig/fs/exec.c
> +++ linux-3.2/fs/exec.c
> @@ -1406,6 +1406,7 @@ int search_binary_handler(struct linux_b
>  					fput(bprm->file);
>  				bprm->file = NULL;
>  				current->did_exec = 1;
> +				current->kmod_thread = 0;
>  				proc_exec_connector(current);
>  				return retval;
>  			}

Special-casing this assignment down in this particular client of kmod
looks bad.  We need to find somewhere else to do this.  Perferably
within the kmod code itself, or possibly over in exec.c or fork.c.