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

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

On Tue, 24 Jan 2012 13:32:40 +0900
Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:

> Tetsuo Handa wrote:
> > Andrew Morton wrote:
> > > > --- 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.
> > > 
> > Well, kmod code is no longer called if do_execve() succeeded.
> > 
> > In "[PATCH v2] kmod: Avoid deadlock by recursive kmod call"
> > ( https://lkml.org/lkml/2012/1/4/111 ), I forgot to do
> > 
> > 	current->kmod_thread = 0;
> > 
> > when do_execve() succeeded. Not clearing this flag might overkill functionality
> > of /sbin/hotplug and its descendants. If we want to clear this flag, I think we
> > cannot help doing it in fork.c or exec.c (or do like patch B which does not
> > need this flag).

(This email is difficult to reply to :(.  I think I'll reorganise it for you)

> I found another approach which mixed patch A and patch B.
>
> ----------------------------------------
> [PATCH v4] kmod: Avoid deadlock by recursive kmod call.
> 
> The system deadlocks (at least since 2.6.10) when
> call_usermodehelper(UMH_WAIT_EXEC) request triggered
> call_usermodehelper(UMH_WAIT_PROC) request.
> 
> This is because "khelper thread is waiting for the worker thread 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" and "the khelper thread cannot
> start processing UMH_WAIT_PROC request because the khelper thread is waiting
> for the worker thread 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.
> 
> The easiest example to observe this deadlock is to use a corrupted
> /sbin/hotplug binary (like shown below).
> 
>   # : > /tmp/dummy
>   # chmod 755 /tmp/dummy
>   # echo /tmp/dummy > /proc/sys/kernel/hotplug
>   # modprobe whatever
> 
> call_usermodehelper("/tmp/dummy", UMH_WAIT_EXEC) is called from
> kobject_uevent_env() in lib/kobject_uevent.c upon loading/unloading a module.
> do_execve("/tmp/dummy") triggers a call to request_module("binfmt-0000") from
> search_binary_handler() which in turn calls call_usermodehelper(UMH_WAIT_PROC).
> 
> There are various hooks called during do_execve() operation (e.g.
> security_bprm_check(), audit_bprm(), "struct linux_binfmt"->load_binary()).
> If one of such hooks triggers UMH_WAIT_EXEC, this deadlock will happen even if
> /sbin/hotplug is not corrupted.


> Since __call_usermodehelper() is exclusively called (am I right?), we don't
> need to use per "struct task_struct" flag.

The locking for the new kmod_thread_locker global is quite unobvious. 
>From a quick look I can't say that I obviously agree with the above
claim.

Maybe it relies upon there only ever being a single khelper thread,
system wide?  If so then, err, maybe this is OK.  But I think the
analysis should be fully spelled out in the changelog and described in
the code in some manner which will prevent us from accidentally
breaking this exclusivity as the code evolves.

Does this look truthful and complete?

--- a/kernel/kmod.c~kmod-avoid-deadlock-by-recursive-kmod-call-fix
+++ a/kernel/kmod.c
@@ -44,6 +44,12 @@
 extern int max_threads;
 
 static struct workqueue_struct *khelper_wq;
+
+/*
+ * kmod_thread_locker is used for deadlock avoidance.  There is no explicit
+ * locking to protect this global - it is private to the singleton khelper
+ * thread and should only ever be modified by that thread.
+ */
 static const struct task_struct *kmod_thread_locker;
 
 #define CAP_BSET	(void *)1
_