Re: + kmod-avoid-deadlock-by-recursive-kmod-call.patch added to -mm tree

Re: + kmod-avoid-deadlock-by-recursive-kmod-call.patch added to -mm tree

[Oleg Nesterov]

> @@ -449,6 +460,16 @@ int call_usermodehelper_exec(struct subp
>  		retval = -EBUSY;
>  		goto out;
>  	}
> +	/*
> +	 * Worker thread must not wait for khelper thread at below
> +	 * wait_for_completion() if the thread was created with CLONE_VFORK
> +	 * flag, for khelper thread is already waiting for the thread at
> +	 * wait_for_completion() in do_fork().
> +	 */
> +	if (wait != UMH_NO_WAIT && current == kmod_thread_locker) {
> +		retval = -EBUSY;
> +		goto out;
> +	}

So, this is because khelper_wq's max_active == 1.

Can't we simply kill khelper_wq and use system_unbound_wq instead?

Oleg.

Re: + kmod-avoid-deadlock-by-recursive-kmod-call.patch added to -mm tree

[Rusty Russell]

On Thu, 26 Jan 2012 18:56:12 +0100, Oleg Nesterov <oleg@redhat.com> wrote:
> > @@ -449,6 +460,16 @@ int call_usermodehelper_exec(struct subp
> >  		retval = -EBUSY;
> >  		goto out;
> >  	}
> > +	/*
> > +	 * Worker thread must not wait for khelper thread at below
> > +	 * wait_for_completion() if the thread was created with CLONE_VFORK
> > +	 * flag, for khelper thread is already waiting for the thread at
> > +	 * wait_for_completion() in do_fork().
> > +	 */
> > +	if (wait != UMH_NO_WAIT && current == kmod_thread_locker) {
> > +		retval = -EBUSY;
> > +		goto out;
> > +	}
> 
> So, this is because khelper_wq's max_active == 1.
> 
> Can't we simply kill khelper_wq and use system_unbound_wq instead?

I'd prefer that, because then we'd hit the existing "too many modprobes"
check.

Thanks,
Rusty.

Re: + kmod-avoid-deadlock-by-recursive-kmod-call.patch added to -mm tree

[Oleg Nesterov]

On 01/27, Rusty Russell wrote:
>
> On Thu, 26 Jan 2012 18:56:12 +0100, Oleg Nesterov <oleg@redhat.com> wrote:
> > > @@ -449,6 +460,16 @@ int call_usermodehelper_exec(struct subp
> > >  		retval = -EBUSY;
> > >  		goto out;
> > >  	}
> > > +	/*
> > > +	 * Worker thread must not wait for khelper thread at below
> > > +	 * wait_for_completion() if the thread was created with CLONE_VFORK
> > > +	 * flag, for khelper thread is already waiting for the thread at
> > > +	 * wait_for_completion() in do_fork().
> > > +	 */
> > > +	if (wait != UMH_NO_WAIT && current == kmod_thread_locker) {
> > > +		retval = -EBUSY;
> > > +		goto out;
> > > +	}
> >
> > So, this is because khelper_wq's max_active == 1.
> >
> > Can't we simply kill khelper_wq and use system_unbound_wq instead?
>
> I'd prefer that, because then we'd hit the existing "too many modprobes"
> check.

Hmm. Why? I mean, why do you think that s/khelper_wq/system_unbound_wq/
leads to recursive __request_module's ?

Note that that this patch (which adds kmod_thread_locker) can not limit
the recursive modprobe loop.


OK, yes, with system_unbound_wq we can hit this warning if we have
max_modprobes UMH_WAIT_EXEC's resulting in __request_module at the
same time, but probably this is good?

I guess I missed something, could you explain? Just curious.

Oleg.

Re: + kmod-avoid-deadlock-by-recursive-kmod-call.patch added to -mm tree

[Rusty Russell]

On Fri, 27 Jan 2012 15:32:34 +0100, Oleg Nesterov <oleg@redhat.com> wrote:
> On 01/27, Rusty Russell wrote:
> > On Thu, 26 Jan 2012 18:56:12 +0100, Oleg Nesterov <oleg@redhat.com> wrote:
> > > Can't we simply kill khelper_wq and use system_unbound_wq instead?
> >
> > I'd prefer that, because then we'd hit the existing "too many modprobes"
> > check.
> 
> Hmm. Why? I mean, why do you think that s/khelper_wq/system_unbound_wq/
> leads to recursive __request_module's ?
> 
> Note that that this patch (which adds kmod_thread_locker) can not limit
> the recursive modprobe loop.
> 
> 
> OK, yes, with system_unbound_wq we can hit this warning if we have
> max_modprobes UMH_WAIT_EXEC's resulting in __request_module at the
> same time, but probably this is good?

Yes, that's what I'm saying.

We already have a check against too many modprobes, it might be best to
use it.

Cheers,
Rusty.

Re: + kmod-avoid-deadlock-by-recursive-kmod-call.patch added to -mm tree

[Oleg Nesterov]

On 01/29, Rusty Russell wrote:
>
> On Fri, 27 Jan 2012 15:32:34 +0100, Oleg Nesterov <oleg@redhat.com> wrote:
> > On 01/27, Rusty Russell wrote:
> > > On Thu, 26 Jan 2012 18:56:12 +0100, Oleg Nesterov <oleg@redhat.com> wrote:
> > > > Can't we simply kill khelper_wq and use system_unbound_wq instead?
> > >
> > > I'd prefer that, because then we'd hit the existing "too many modprobes"
> > > check.
> >
> > Hmm. Why? I mean, why do you think that s/khelper_wq/system_unbound_wq/
> > leads to recursive __request_module's ?
> >
> > Note that that this patch (which adds kmod_thread_locker) can not limit
> > the recursive modprobe loop.
> >
> >
> > OK, yes, with system_unbound_wq we can hit this warning if we have
> > max_modprobes UMH_WAIT_EXEC's resulting in __request_module at the
> > same time, but probably this is good?
>
> Yes, that's what I'm saying.
>
> We already have a check against too many modprobes, it might be best to
> use it.

Confused... in this case I do not understand why do you dislike the
idea to kill khelper_wq.

Help!

Oleg.

Re: + kmod-avoid-deadlock-by-recursive-kmod-call.patch added to -mm tree

[Rusty Russell]

On Sun, 29 Jan 2012 17:31:41 +0100, Oleg Nesterov <oleg@redhat.com> wrote:
> Confused... in this case I do not understand why do you dislike the
> idea to kill khelper_wq.

Yes, you are confused.  I was agreeing with you:

 On Thu, 26 Jan 2012 18:56:12 +0100, Oleg Nesterov <oleg@redhat.com> wrote:
 > Can't we simply kill khelper_wq and use system_unbound_wq instead?

 I'd prefer that, because then we'd hit the existing "too many modprobes"
 check.

 Thanks,
 Rusty.

I thought "I'd prefer that" was pretty clear??

Thanks,
Rusty.

Re: + kmod-avoid-deadlock-by-recursive-kmod-call.patch added to -mm tree

[Tejun Heo]

On Mon, Jan 30, 2012 at 09:56:44AM +1030, Rusty Russell wrote:
> On Sun, 29 Jan 2012 17:31:41 +0100, Oleg Nesterov <oleg@redhat.com> wrote:
> > Confused... in this case I do not understand why do you dislike the
> > idea to kill khelper_wq.
> 
> Yes, you are confused.  I was agreeing with you:
> 
>  On Thu, 26 Jan 2012 18:56:12 +0100, Oleg Nesterov <oleg@redhat.com> wrote:
>  > Can't we simply kill khelper_wq and use system_unbound_wq instead?

BTW, why does it have to be unbound_wq?  Is it expected consume large
amount of CPU cycles?

Thanks.

-- 
tejun

Re: + kmod-avoid-deadlock-by-recursive-kmod-call.patch added to -mm tree

[Oleg Nesterov]

On 01/29, Tejun Heo wrote:
>
> BTW, why does it have to be unbound_wq?

Perhaps we can use another system_wq, but afaics WQ_UNBOUND makes sense
in this case. I mean, there is no reason to bind this work to any CPU.
See also below.

> Is it expected consume large
> amount of CPU cycles?

Currently __call_usermodehelper() does kernel_thread(), this is almost
all. But it can block waiting for kernel_execve().

Not sure this really makes sense, but if we kill khelper_wq perhaps we
can simplify this code a bit. We can change __call_usermodehelper()

		if (wait == UMH_WAIT_PROC)
	-		pid = kernel_thread(wait_for_helper, sub_info,
	-				    CLONE_FS | CLONE_FILES | SIGCHLD);
	+		wait_for_helper(...);
		else

IOW, the worker thread itself can do the UMH_WAIT_PROC work. This makes
this work really "long running", but then we can kill sub_info->complete
and use flush_work().

Oleg.

Re: + kmod-avoid-deadlock-by-recursive-kmod-call.patch added to -mm tree

[Tejun Heo]

Hello,

On Mon, Jan 30, 2012 at 02:03:35PM +0100, Oleg Nesterov wrote:
> Perhaps we can use another system_wq, but afaics WQ_UNBOUND makes sense
> in this case. I mean, there is no reason to bind this work to any CPU.
> See also below.

I've been trying to nudge people away from using special wqs or flags
unless really necessary.  Other than non-reentrancy and strict
ordering, all behaviors are mostly for optimization and using them
incorrectly / spuriously usually doesn't cause any visible failure,
making it very easy to get them wrong and if you have enough of wrong
/ unnecessary usages in tree, the whole thing gets really confusing
and difficult to update in the future.

> > Is it expected consume large
> > amount of CPU cycles?
> 
> Currently __call_usermodehelper() does kernel_thread(), this is almost
> all. But it can block waiting for kernel_execve().

Blocking is completely fine on any workqueue.  The only reason to
require the use of unbound_wq is if work items would burn a lot of CPU
cycles.  In such cases, we want to let the scheduler have full
jurisdiction instead of wq regulating concurrency.

> Not sure this really makes sense, but if we kill khelper_wq perhaps we
> can simplify this code a bit. We can change __call_usermodehelper()
> 
> 		if (wait == UMH_WAIT_PROC)
> 	-		pid = kernel_thread(wait_for_helper, sub_info,
> 	-				    CLONE_FS | CLONE_FILES | SIGCHLD);
> 	+		wait_for_helper(...);
> 		else
> 
> IOW, the worker thread itself can do the UMH_WAIT_PROC work. This makes
> this work really "long running", but then we can kill sub_info->complete
> and use flush_work().

Again, long-running in the sense that the work item spending a lot of
time sleeping should be fine on system_wq or any other wq with default
attributes.  AFAICS, the things to consider here are...

* If work items are expected to consume large amount of CPU cycles (as
  in crypto work items), consider using system_unbound_wq / WQ_UNBOUND.

* If per-domain concurrency limit is necessary (ie. the number of
  concurrent work items doing this particular task should be limited
  rather than consuming global system_wq limit), a dedicated workqueue
  would be better.

Thanks.

-- 
tejun

Re: + kmod-avoid-deadlock-by-recursive-kmod-call.patch added to -mm tree

[Oleg Nesterov]

Hi Tejun,

On 01/30, Tejun Heo wrote:
>
> On Mon, Jan 30, 2012 at 02:03:35PM +0100, Oleg Nesterov wrote:
> > Perhaps we can use another system_wq, but afaics WQ_UNBOUND makes sense
> > in this case. I mean, there is no reason to bind this work to any CPU.
> > See also below.
>
> I've been trying to nudge people away from using special wqs or flags
> unless really necessary.  Other than non-reentrancy and strict
> ordering, all behaviors are mostly for optimization and using them
> incorrectly / spuriously usually doesn't cause any visible failure,
> making it very easy to get them wrong and if you have enough of wrong
> / unnecessary usages in tree, the whole thing gets really confusing
> and difficult to update in the future.

You know, I am a bit suprized. To me, it is the !WQ_UNBOUND case is
"special". IOW, I think we need some reason to bind the work to the
specific CPU.

> > > Is it expected consume large
> > > amount of CPU cycles?
> >
> > Currently __call_usermodehelper() does kernel_thread(), this is almost
> > all. But it can block waiting for kernel_execve().
>
> Blocking is completely fine on any workqueue.

I understand. But, the blocked worker "consumes" nr_active/worker.

> The only reason to
> require the use of unbound_wq is if work items would burn a lot of CPU
> cycles.  In such cases, we want to let the scheduler have full
> jurisdiction instead of wq regulating concurrency.

I am starting to think I do not understand this code at all. OK,
perhaps unbound_wq should be used for cpu-intensive works only.

But why do you think that we should use a !WQ_UNBOUND workque
instead of khelper_wq? And why "a lot of CPU" is the only reason
for WQ_UNBOUND?

> * If work items are expected to consume large amount of CPU cycles (as
>   in crypto work items), consider using system_unbound_wq / WQ_UNBOUND.
>
> * If per-domain concurrency limit is necessary (ie. the number of
>   concurrent work items doing this particular task should be limited
>   rather than consuming global system_wq limit), a dedicated workqueue
>   would be better.

So I don't understand whether you like the idea to kill khelper_wq
and use some system_ wq or not (and fix the bug).

I do not really like the current patch. If nothing else, what if
UMH_WAIT_EXEC request actually needs another UMH_WAIT_EXEC/PROC
request to succeed?

Tetsuo, we spent a lot of time discussing other problems. What
do you think about s/khelper/system/ instead of this patch?

Oleg.

Re: + kmod-avoid-deadlock-by-recursive-kmod-call.patch added to -mm tree

[Tejun Heo]

Hello, Oleg.

On Fri, Feb 03, 2012 at 07:00:30PM +0100, Oleg Nesterov wrote:
> > I've been trying to nudge people away from using special wqs or flags
> > unless really necessary.  Other than non-reentrancy and strict
> > ordering, all behaviors are mostly for optimization and using them
> > incorrectly / spuriously usually doesn't cause any visible failure,
> > making it very easy to get them wrong and if you have enough of wrong
> > / unnecessary usages in tree, the whole thing gets really confusing
> > and difficult to update in the future.
> 
> You know, I am a bit suprized. To me, it is the !WQ_UNBOUND case is
> "special". IOW, I think we need some reason to bind the work to the
> specific CPU.

It's part historical due to the way workqueue was originally
implemented and part because work items are expected to consume little
CPU cycles and access data which the issuer accessed, but the question
really isn't about whether unbound would be marginally better or not.
We may change the default behavior in the future as implementation
evolves or hardware environment changes and having more things which
claim special treatment without clear reason makes things needlessly
complicated, so the question to ask is whether the default behavior
doesn't fit enough that the extra flag is really required.

> > Blocking is completely fine on any workqueue.
> 
> I understand. But, the blocked worker "consumes" nr_active/worker.

If it's expected to consume high number of nr_active and needs
limiting, creating a dedicated wq as an isolation domain would be a
good idea.  It's not like wqs are expensive or complex to use after
all.

> > The only reason to
> > require the use of unbound_wq is if work items would burn a lot of CPU
> > cycles.  In such cases, we want to let the scheduler have full
> > jurisdiction instead of wq regulating concurrency.
> 
> I am starting to think I do not understand this code at all. OK,
> perhaps unbound_wq should be used for cpu-intensive works only.
> 
> But why do you think that we should use a !WQ_UNBOUND workque
> instead of khelper_wq? And why "a lot of CPU" is the only reason
> for WQ_UNBOUND?

As I wrote, it's not about "should use bound", it's about having
enough justification for deviating from the default.

Thanks.

-- 
tejun

Re: + kmod-avoid-deadlock-by-recursive-kmod-call.patch added to-mm tree

[Tetsuo Handa]

Oleg Nesterov wrote:
> What do you think about s/khelper/system/ instead of this patch?

I'm not catching up with this topic. But I'm fine with any solution provided
that it can handle

  some kernel function calls request_module()

    request_module() triggers call_usermodehelper("/sbin/modprobe", UMH_WAIT_PROC)

      /sbin/modprobe loads a kernel module

        loading a kernel module triggers kobject_uevent_env()

          kobject_uevent_env() calls call_usermodehelper("/sbin/hotplug", UMH_WAIT_EXEC)

            do_execve("/sbin/hotplug") calls request_module("binfmt-0000")

              request_module("binfmt-0000") triggers call_usermodehelper("/sbin/modprobe", UMH_WAIT_PROC)

                /sbin/modprobe fails to load binfmt-0000 module

call trace on an UP machine.

kmod-avoid-deadlock-by-recursive-kmod-call.patch is for avoiding
call_usermodehelper("/sbin/modprobe", UMH_WAIT_PROC) called from
call_usermodehelper("/sbin/hotplug", UMH_WAIT_EXEC).

Re: + kmod-avoid-deadlock-by-recursive-kmod-call.patch added to-mm tree

[Oleg Nesterov]

On 02/04, Tetsuo Handa wrote:
>
> Oleg Nesterov wrote:
> > What do you think about s/khelper/system/ instead of this patch?
>
> I'm not catching up with this topic. But I'm fine with any solution provided
> that it can handle

I think it should...

Tetsuo, all, sorry I am busy today. I'll try to return tomorrow to
continue this (and off-list) discussion.

Oleg.

Re: + kmod-avoid-deadlock-by-recursive-kmod-call.patch added to -mm tree

[Oleg Nesterov]

On 01/30, Rusty Russell wrote:
>
> On Sun, 29 Jan 2012 17:31:41 +0100, Oleg Nesterov <oleg@redhat.com> wrote:
> > Confused... in this case I do not understand why do you dislike the
> > idea to kill khelper_wq.
>
> Yes, you are confused.  I was agreeing with you:

Ah...

> I thought "I'd prefer that" was pretty clear??

It was! Looking back, I do not understand how did I manage to misread
your email.

Thanks Rusty,

Oleg.

+ kmod-avoid-deadlock-by-recursive-kmod-call.patch added to -mm tree

[akpm]

The patch titled
     Subject: kmod: avoid deadlock from recursive kmod call
has been added to the -mm tree.  Its filename is
     kmod-avoid-deadlock-by-recursive-kmod-call.patch

Before you just go and hit "reply", please:
   a) Consider who else should be cc'ed
   b) Prefer to cc a suitable mailing list as well
   c) Ideally: find the original patch on the mailing list and do a
      reply-to-all to that, adding suitable additional cc's

*** Remember to use Documentation/SubmitChecklist when testing your code ***

The -mm tree is included into linux-next and is updated
there every 3-4 working days

------------------------------------------------------
From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Subject: kmod: avoid deadlock from 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.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Arjan van de Ven <arjan@linux.intel.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Tejun Heo <tj@kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 kernel/kmod.c |   25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff -puN kernel/kmod.c~kmod-avoid-deadlock-by-recursive-kmod-call kernel/kmod.c
--- a/kernel/kmod.c~kmod-avoid-deadlock-by-recursive-kmod-call
+++ a/kernel/kmod.c
@@ -44,6 +44,7 @@
 extern int max_threads;
 
 static struct workqueue_struct *khelper_wq;
+static const struct task_struct *kmod_thread_locker;
 
 #define CAP_BSET	(void *)1
 #define CAP_PI		(void *)2
@@ -191,6 +192,13 @@ fail:
 	do_exit(0);
 }
 
+static int call_helper(void *data)
+{
+	/* Worker thread started blocking khelper thread. */
+	kmod_thread_locker = current;
+	return ____call_usermodehelper(data);
+}
+
 void call_usermodehelper_freeinfo(struct subprocess_info *info)
 {
 	if (info->cleanup)
@@ -253,9 +261,12 @@ static void __call_usermodehelper(struct
 	if (wait == UMH_WAIT_PROC)
 		pid = kernel_thread(wait_for_helper, sub_info,
 				    CLONE_FS | CLONE_FILES | SIGCHLD);
-	else
-		pid = kernel_thread(____call_usermodehelper, sub_info,
+	else {
+		pid = kernel_thread(call_helper, sub_info,
 				    CLONE_VFORK | SIGCHLD);
+		/* Worker thread stopped blocking khelper thread. */
+		kmod_thread_locker = NULL;
+	}
 
 	switch (wait) {
 	case UMH_NO_WAIT:
@@ -449,6 +460,16 @@ int call_usermodehelper_exec(struct subp
 		retval = -EBUSY;
 		goto out;
 	}
+	/*
+	 * Worker thread must not wait for khelper thread at below
+	 * wait_for_completion() if the thread was created with CLONE_VFORK
+	 * flag, for khelper thread is already waiting for the thread at
+	 * wait_for_completion() in do_fork().
+	 */
+	if (wait != UMH_NO_WAIT && current == kmod_thread_locker) {
+		retval = -EBUSY;
+		goto out;
+	}
 
 	sub_info->complete = &done;
 	sub_info->wait = wait;
_
Subject: Subject: kmod: avoid deadlock from recursive kmod call

Patches currently in -mm which might be from penguin-kernel@i-love.sakura.ne.jp are

kmod-avoid-deadlock-by-recursive-kmod-call.patch
kmod-avoid-deadlock-by-recursive-kmod-call-fix.patch