[PATCH][Resend v2] Fix infinite loop in search_binary_handler()

[PATCH][Resend v2] Fix infinite loop in search_binary_handler()

[Richard Weinberger]

When search_binary_handler() is called to find a handler
for /sbin/modprobe it will end up in an infinite loop because
it executes request_module() to load a binfmt module with
/sbin/modprobe...

Running a x86_64 kernel without ia32 emulation and a x86 user land
triggers this issue.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/exec.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 97e0d52..7271b22 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1411,7 +1411,10 @@ int search_binary_handler(struct linux_binprm *bprm,struct pt_regs *regs)
 			    printable(bprm->buf[2]) &&
 			    printable(bprm->buf[3]))
 				break; /* -ENOEXEC */
-			request_module("binfmt-%04x", *(unsigned short *)(&bprm->buf[2]));
+
+			/* Avoid an infinite loop */
+			if (strcmp(modprobe_path, bprm->filename))
+				request_module("binfmt-%04x", *(unsigned short *)(&bprm->buf[2]));
 #endif
 		}
 	}
-- 
1.7.5.4

Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()

[Tetsuo Handa]

Richard Weinberger wrote:
> Running a x86_64 kernel without ia32 emulation and a x86 user land
> triggers this issue.

Executing /sbin/modprobe for x86_32 on an x86_64 kernel without x86_32 support?

Anyway, request_module() calls __request_module() but
__request_module() stops at MAX_KMOD_CONCURRENT levels of nesting.
So, I think "infinite loop" cannot happen.

Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()

[Richard Weinberger]

Am Montag 04 Juli 2011, 13:51:55 schrieb Tetsuo Handa:
> Richard Weinberger wrote:
> > Running a x86_64 kernel without ia32 emulation and a x86 user land
> > triggers this issue.
> 
> Executing /sbin/modprobe for x86_32 on an x86_64 kernel without x86_32
> support?

Yep.

> Anyway, request_module() calls __request_module() but
> __request_module() stops at MAX_KMOD_CONCURRENT levels of nesting.
> So, I think "infinite loop" cannot happen.

Booting a x86_64 UML kernel with x86_32 user land triggers this issue.
I always wondered why the UML kernel hangs an consumes 100% CPU.

Thanks,
//richard

Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()

[Tetsuo Handa]

Richard Weinberger wrote:
> Am Montag 04 Juli 2011, 13:51:55 schrieb Tetsuo Handa:
> > Richard Weinberger wrote:
> > > Running a x86_64 kernel without ia32 emulation and a x86 user land
> > > triggers this issue.
> > 
> > Executing /sbin/modprobe for x86_32 on an x86_64 kernel without x86_32
> > support?
> 
> Yep.
> 
> > Anyway, request_module() calls __request_module() but
> > __request_module() stops at MAX_KMOD_CONCURRENT levels of nesting.
> > So, I think "infinite loop" cannot happen.
> 
> Booting a x86_64 UML kernel with x86_32 user land triggers this issue.
> I always wondered why the UML kernel hangs an consumes 100% CPU.

That's strange... Would you show us printk() output like

	printk(KERN_INFO "Calling request_module()\n");
	request_module("binfmt-%04x", *(unsigned short *)(&bprm->buf[2]));
	printk(KERN_INFO "Returned from request_module()\n");

for demonstrating that __request_module() cannot stop at
MAX_KMOD_CONCURRENT levels of nesting?

Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()

[Richard Weinberger]

Am Montag 04 Juli 2011, 14:10:43 schrieb Tetsuo Handa:
> Richard Weinberger wrote:
> > Am Montag 04 Juli 2011, 13:51:55 schrieb Tetsuo Handa:
> > > Richard Weinberger wrote:
> > > > Running a x86_64 kernel without ia32 emulation and a x86 user land
> > > > triggers this issue.
> > > 
> > > Executing /sbin/modprobe for x86_32 on an x86_64 kernel without x86_32
> > > support?
> > 
> > Yep.
> > 
> > > Anyway, request_module() calls __request_module() but
> > > __request_module() stops at MAX_KMOD_CONCURRENT levels of nesting.
> > > So, I think "infinite loop" cannot happen.
> > 
> > Booting a x86_64 UML kernel with x86_32 user land triggers this issue.
> > I always wondered why the UML kernel hangs an consumes 100% CPU.
> 
> That's strange... Would you show us printk() output like
> 
> 	printk(KERN_INFO "Calling request_module()\n");
> 	request_module("binfmt-%04x", *(unsigned short *)(&bprm->buf[2]));
> 	printk(KERN_INFO "Returned from request_module()\n");
> 
> for demonstrating that __request_module() cannot stop at
> MAX_KMOD_CONCURRENT levels of nesting?

There you go!
http://userweb.kernel.org/~rw/boot.log

I did not count all messages, but they are more than 50. :-)

Thanks,
//richard

Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()

[Tetsuo Handa]

Richard Weinberger wrote:
> > That's strange... Would you show us printk() output like
> > 
> > 	printk(KERN_INFO "Calling request_module()\n");
> > 	request_module("binfmt-%04x", *(unsigned short *)(&bprm->buf[2]));
> > 	printk(KERN_INFO "Returned from request_module()\n");
> > 
> > for demonstrating that __request_module() cannot stop at
> > MAX_KMOD_CONCURRENT levels of nesting?
> 
> There you go!
> http://userweb.kernel.org/~rw/boot.log
> 
> I did not count all messages, but they are more than 50. :-)

Thank you.

$ grep -F 'Calling request_module()' boot.log | wc -l
25819
$ grep -F 'Returned from request_module()' boot.log | wc -l
25770

So, __request_module() is stopping at MAX_KMOD_CONCURRENT levels
of nesting. Eventually the process that triggered the first
request_module() will return from search_binary_handler().
I don't think this is an infinite loop inside search_binary_handler().

But it would look like an infinite loop bug if the caller of execve()
repeats forever. Printing additional information like

  printk(KERN_INFO "Calling request_module() %s %d %s %d %d\n", current->comm,
         current->pid, bprm->filename, bprm->argc, bprm->envc);

would help.

Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()

[Richard Weinberger]

Am Montag 04 Juli 2011, 16:42:09 schrieb Tetsuo Handa:
> Richard Weinberger wrote:
> > > That's strange... Would you show us printk() output like
> > > 
> > > 	printk(KERN_INFO "Calling request_module()\n");
> > > 	request_module("binfmt-%04x", *(unsigned short *)(&bprm->buf[2]));
> > > 	printk(KERN_INFO "Returned from request_module()\n");
> > > 
> > > for demonstrating that __request_module() cannot stop at
> > > MAX_KMOD_CONCURRENT levels of nesting?
> > 
> > There you go!
> > http://userweb.kernel.org/~rw/boot.log
> > 
> > I did not count all messages, but they are more than 50. :-)
> 
> Thank you.
> 
> $ grep -F 'Calling request_module()' boot.log | wc -l
> 25819
> $ grep -F 'Returned from request_module()' boot.log | wc -l
> 25770

Ahh, the dela is 49. Got it!

> So, __request_module() is stopping at MAX_KMOD_CONCURRENT levels
> of nesting. Eventually the process that triggered the first
> request_module() will return from search_binary_handler().
> I don't think this is an infinite loop inside search_binary_handler().
> 
> But it would look like an infinite loop bug if the caller of execve()
> repeats forever. Printing additional information like
> 
>   printk(KERN_INFO "Calling request_module() %s %d %s %d %d\n",
> current->comm, current->pid, bprm->filename, bprm->argc, bprm->envc);
> 
> would help.

Here the second boot log:
http://userweb.kernel.org/~rw/boot2.log

The interesting part is:
---cut---
VFS: Mounted root (ext2 filesystem) readonly on device 98:0.
Calling request_module() swapper 1 /sbin/init 1 3
Calling request_module() kworker/u:0 211 /sbin/modprobe 4 3
...
Calling request_module() kworker/u:0 8741 /sbin/modprobe 4 3
---cut---

After the last "Calling request_module..." message no more messages appear and 
the kernel seems to loop for ever.

Maybe it takes just very long until all calls to /sbin/modprobe terminate?

Thanks,
//richard

Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()

[Richard Weinberger]

Am Montag 04 Juli 2011, 16:59:32 schrieb Richard Weinberger:
> Here the second boot log:
> http://userweb.kernel.org/~rw/boot2.log
> 
> The interesting part is:
> ---cut---
> VFS: Mounted root (ext2 filesystem) readonly on device 98:0.
> Calling request_module() swapper 1 /sbin/init 1 3
> Calling request_module() kworker/u:0 211 /sbin/modprobe 4 3
> ...
> Calling request_module() kworker/u:0 8741 /sbin/modprobe 4 3
> ---cut---
> 
> After the last "Calling request_module..." message no more messages appear
> and the kernel seems to loop for ever.
> 

Please ignore this.
The "Calling request_module..." messages continue for ever.
In the first case my terminal locked up...
current->pid also wraps around.

Thanks,
//richard

Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()

[Tetsuo Handa]

Richard Weinberger wrote:
> Am Montag 04 Juli 2011, 16:59:32 schrieb Richard Weinberger:
> > Here the second boot log:
> > http://userweb.kernel.org/~rw/boot2.log
> > 
> > The interesting part is:
> > ---cut---
> > VFS: Mounted root (ext2 filesystem) readonly on device 98:0.
> > Calling request_module() swapper 1 /sbin/init 1 3
> > Calling request_module() kworker/u:0 211 /sbin/modprobe 4 3
> > ...
> > Calling request_module() kworker/u:0 8741 /sbin/modprobe 4 3
> > ---cut---
> > 
> > After the last "Calling request_module..." message no more messages appear
> > and the kernel seems to loop for ever.
> > 
> 
> Please ignore this.
> The "Calling request_module..." messages continue for ever.
> In the first case my terminal locked up...
> current->pid also wraps around.
> 
Please note that the each PID is printed for twice.
This means that each thread within the loop tries to create a thread.
Therefore, it will repeat for (1 << MAX_KMOD_CONCURRENT) times.
It is large enough to wrap the PID.
Well, try with smaller MAX_KMOD_CONCURRENT (e.g. 3).

Calling request_module() kworker/u:0 299 /sbin/modprobe 4 3
Calling request_module() kworker/u:0 301 /sbin/modprobe 4 3 **** 301 1st loop
Calling request_module() kworker/u:0 303 /sbin/modprobe 4 3 **** 303 1st loop
Calling request_module() kworker/u:0 305 /sbin/modprobe 4 3 **** 305 1st loop
Calling request_module() kworker/u:0 307 /sbin/modprobe 4 3 **** 307 1st loop
Calling request_module() kworker/u:0 309 /sbin/modprobe 4 3 **** 309 1st loop
request_module: runaway loop modprobe binfmt-464c
Calling request_module() kworker/u:0 309 /sbin/modprobe 4 3 **** 309 2nd loop
request_module: runaway loop modprobe binfmt-464c
Calling request_module() kworker/u:0 307 /sbin/modprobe 4 3 **** 307 2nd loop
Calling request_module() kworker/u:0 311 /sbin/modprobe 4 3 **** 311 1st loop
request_module: runaway loop modprobe binfmt-464c
Calling request_module() kworker/u:0 311 /sbin/modprobe 4 3 **** 311 2nd loop
request_module: runaway loop modprobe binfmt-464c
Calling request_module() kworker/u:0 305 /sbin/modprobe 4 3 **** 305 2nd loop
Calling request_module() kworker/u:0 313 /sbin/modprobe 4 3 **** 313 1st loop
Calling request_module() kworker/u:0 315 /sbin/modprobe 4 3 **** 315 1st loop
request_module: runaway loop modprobe binfmt-464c
Calling request_module() kworker/u:0 315 /sbin/modprobe 4 3 **** 315 2nd loop
Calling request_module() kworker/u:0 313 /sbin/modprobe 4 3 **** 313 2nd loop
Calling request_module() kworker/u:0 317 /sbin/modprobe 4 3 **** 317 1st loop
Calling request_module() kworker/u:0 317 /sbin/modprobe 4 3 **** 317 2nd loop
Calling request_module() kworker/u:0 303 /sbin/modprobe 4 3 **** 303 2nd loop
Calling request_module() kworker/u:0 319 /sbin/modprobe 4 3
Calling request_module() kworker/u:0 321 /sbin/modprobe 4 3
Calling request_module() kworker/u:0 323 /sbin/modprobe 4 3
Calling request_module() kworker/u:0 323 /sbin/modprobe 4 3
Calling request_module() kworker/u:0 321 /sbin/modprobe 4 3
Calling request_module() kworker/u:0 325 /sbin/modprobe 4 3
Calling request_module() kworker/u:0 325 /sbin/modprobe 4 3
Calling request_module() kworker/u:0 319 /sbin/modprobe 4 3
Calling request_module() kworker/u:0 327 /sbin/modprobe 4 3
Calling request_module() kworker/u:0 329 /sbin/modprobe 4 3
Calling request_module() kworker/u:0 329 /sbin/modprobe 4 3
Calling request_module() kworker/u:0 327 /sbin/modprobe 4 3
Calling request_module() kworker/u:0 331 /sbin/modprobe 4 3
Calling request_module() kworker/u:0 331 /sbin/modprobe 4 3
Calling request_module() kworker/u:0 301 /sbin/modprobe 4 3 **** 301 2nd loop
Calling request_module() kworker/u:0 333 /sbin/modprobe 4 3

Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()

[Richard Weinberger]

Am Dienstag 05 Juli 2011, 00:03:47 schrieb Tetsuo Handa:
> Richard Weinberger wrote:
> > Am Montag 04 Juli 2011, 16:59:32 schrieb Richard Weinberger:
> > > Here the second boot log:
> > > http://userweb.kernel.org/~rw/boot2.log
> > > 
> > > The interesting part is:
> > > ---cut---
> > > VFS: Mounted root (ext2 filesystem) readonly on device 98:0.
> > > Calling request_module() swapper 1 /sbin/init 1 3
> > > Calling request_module() kworker/u:0 211 /sbin/modprobe 4 3
> > > ...
> > > Calling request_module() kworker/u:0 8741 /sbin/modprobe 4 3
> > > ---cut---
> > > 
> > > After the last "Calling request_module..." message no more messages
> > > appear and the kernel seems to loop for ever.
> > 
> > Please ignore this.
> > The "Calling request_module..." messages continue for ever.
> > In the first case my terminal locked up...
> > current->pid also wraps around.
> 
> Please note that the each PID is printed for twice.
> This means that each thread within the loop tries to create a thread.
> Therefore, it will repeat for (1 << MAX_KMOD_CONCURRENT) times.
> It is large enough to wrap the PID.
> Well, try with smaller MAX_KMOD_CONCURRENT (e.g. 3).
> 

With MAX_KMOD_CONCURRENT=3 it takes only a few seconds until 
the modprobe storm ends.

How shall we proceed?
Applying my ad-hoc patch 
or lowering MAX_KMOD_CONCURRENT?

Thanks,
//richard

Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()

[Tetsuo Handa]

Richard Weinberger wrote:
> With MAX_KMOD_CONCURRENT=3 it takes only a few seconds until 
> the modprobe storm ends.

OK. Many years ago, I got a few reports that the kernel panics after printing

  request_module: runaway loop modprobe binfmt-464c

line. This was because they installed by error a binary x86_32 kernel rpm on
x86_64 userland tools. So, this error is not specific to UML.

> How shall we proceed?
> Applying my ad-hoc patch 
> or lowering MAX_KMOD_CONCURRENT?

What about disallowing request_module() for ____call_usermodehelper() threads?

diff --git a/include/linux/sched.h b/include/linux/sched.h
index a837b20..70b52de 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1289,6 +1289,7 @@ struct task_struct {
 	unsigned did_exec:1;
 	unsigned in_execve:1;	/* Tell the LSMs that the process is doing an
 				 * execve */
+	unsigned no_request_module:1; /* Disallow request_module() calls. */
 	unsigned in_iowait:1;
 
 
diff --git a/kernel/kmod.c b/kernel/kmod.c
index 47613df..32ad87c 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -88,6 +88,8 @@ int __request_module(bool wait, const char *fmt, ...)
 #define MAX_KMOD_CONCURRENT 50	/* Completely arbitrary value - KAO */
 	static int kmod_loop_msg;
 
+	if (current->no_request_module)
+		return -ENOSYS;
 	va_start(args, fmt);
 	ret = vsnprintf(module_name, MODULE_NAME_LEN, fmt, args);
 	va_end(args);
@@ -177,6 +179,7 @@ static int ____call_usermodehelper(void *data)
 
 	commit_creds(new);
 
+	current->no_request_module = 1;
 	retval = kernel_execve(sub_info->path,
 			       (const char *const *)sub_info->argv,
 			       (const char *const *)sub_info->envp);
diff --git a/fs/exec.c b/fs/exec.c
index 6075a1e..b949387 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1387,6 +1387,7 @@ int search_binary_handler(struct linux_binprm *bprm,struct pt_regs *regs)
 				if (bprm->file)
 					fput(bprm->file);
 				bprm->file = NULL;
+				current->no_request_module = 0;
 				current->did_exec = 1;
 				proc_exec_connector(current);
 				return retval;

Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()

[Richard Weinberger]

Am Dienstag 05 Juli 2011, 03:24:14 schrieb Tetsuo Handa:
> Richard Weinberger wrote:
> > With MAX_KMOD_CONCURRENT=3 it takes only a few seconds until
> > the modprobe storm ends.
> 
> OK. Many years ago, I got a few reports that the kernel panics after
> printing
> 
>   request_module: runaway loop modprobe binfmt-464c
> 
> line. This was because they installed by error a binary x86_32 kernel rpm
> on x86_64 userland tools. So, this error is not specific to UML.
> 
> > How shall we proceed?
> > Applying my ad-hoc patch
> > or lowering MAX_KMOD_CONCURRENT?
> 
> What about disallowing request_module() for ____call_usermodehelper()
> threads?

This patch helps.
But IMHO adding a new attribute to task_struct is a bit overkill.
Why is your variant better than my strcmp() in fs/exec.c?

Thanks,
//richard

Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()

[Tetsuo Handa]

Richard Weinberger wrote:
> But IMHO adding a new attribute to task_struct is a bit overkill.
> Why is your variant better than my strcmp() in fs/exec.c?

Say, there are multiple /sbin/modprobe

/sbin/modprobe
/var/chroot/sbin/modprobe

and only /var/chroot/sbin/modprobe needs request_module() whereas
/sbin/modprobe does not need request_module(). Why do we need to make
execl("/sbin/modprobe", "--help", NULL) from chroot("/var/chroot/") fail
by denying request_module() that does not cause recursion?

Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()

[Richard Weinberger]

Am Dienstag 05 Juli 2011, 14:02:34 schrieb Tetsuo Handa:
> Richard Weinberger wrote:
> > But IMHO adding a new attribute to task_struct is a bit overkill.
> > Why is your variant better than my strcmp() in fs/exec.c?
> 
> Say, there are multiple /sbin/modprobe
> 
> /sbin/modprobe
> /var/chroot/sbin/modprobe
> 
> and only /var/chroot/sbin/modprobe needs request_module() whereas
> /sbin/modprobe does not need request_module(). Why do we need to make
> execl("/sbin/modprobe", "--help", NULL) from chroot("/var/chroot/") fail
> by denying request_module() that does not cause recursion?

*headdesk*, bprm->filename will contain "/sbin/modprobe" and the strcmp()
will falsely match.
Thanks for the pointer!

//richard

Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()

[Tetsuo Handa]

Richard Weinberger wrote:
> But IMHO adding a new attribute to task_struct is a bit overkill.
Indeed. I came up with a simpler solution. ;-)
----------------------------------------
[PATCH] exec: Do not call request_module() twice from search_binary_handler().

Currently, search_binary_handler() tries to load binary loader module using
request_module() if a loader for the requested program is not yet loaded. But
second attempt of request_module() does not affect the result of
search_binary_handler().

If request_module() triggered recursion, calling request_module() twice causes
2 to the power of MAX_KMOD_CONCURRENT (= 50) repetitions. It is not an infinite
loop but is sufficient for users to consider as a hang up.

Therefore, this patch changes not to call request_module() twice, making
1 to the power of MAX_KMOD_CONCURRENT repetitions in case of recursion.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
diff --git a/fs/exec.c b/fs/exec.c
index 6075a1e..044c13f 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1411,6 +1411,8 @@ int search_binary_handler(struct linux_binprm *bprm,struct pt_regs *regs)
 			    printable(bprm->buf[2]) &&
 			    printable(bprm->buf[3]))
 				break; /* -ENOEXEC */
+			if (try)
+				break; /* -ENOEXEC */
 			request_module("binfmt-%04x", *(unsigned short *)(&bprm->buf[2]));
 #endif
 		}

Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()

[Richard Weinberger]

Am Mittwoch 06 Juli 2011, 13:28:18 schrieb Tetsuo Handa:
> Richard Weinberger wrote:
> > But IMHO adding a new attribute to task_struct is a bit overkill.
> 
> Indeed. I came up with a simpler solution. ;-)
> ----------------------------------------
> [PATCH] exec: Do not call request_module() twice from
> search_binary_handler().
> 
> Currently, search_binary_handler() tries to load binary loader module using
> request_module() if a loader for the requested program is not yet loaded.
> But second attempt of request_module() does not affect the result of
> search_binary_handler().
> 
> If request_module() triggered recursion, calling request_module() twice
> causes 2 to the power of MAX_KMOD_CONCURRENT (= 50) repetitions. It is not
> an infinite loop but is sufficient for users to consider as a hang up.
> 
> Therefore, this patch changes not to call request_module() twice, making
> 1 to the power of MAX_KMOD_CONCURRENT repetitions in case of recursion.
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> diff --git a/fs/exec.c b/fs/exec.c
> index 6075a1e..044c13f 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1411,6 +1411,8 @@ int search_binary_handler(struct linux_binprm
> *bprm,struct pt_regs *regs) printable(bprm->buf[2]) &&
>  			    printable(bprm->buf[3]))
>  				break; /* -ENOEXEC */
> +			if (try)
> +				break; /* -ENOEXEC */
>  			request_module("binfmt-%04x", *(unsigned short *)(&bprm->buf[2]));
>  #endif
>  		}

Nice solution. :-)

Tested-by: Richard Weinberger <richard@nod.at>

Thanks,
//richard

Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()

[Andrew Morton]

On Wed, 6 Jul 2011 20:28:18 +0900
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:

> Richard Weinberger wrote:
> > But IMHO adding a new attribute to task_struct is a bit overkill.
> Indeed. I came up with a simpler solution. ;-)
> ----------------------------------------
> [PATCH] exec: Do not call request_module() twice from search_binary_handler().
> 
> Currently, search_binary_handler() tries to load binary loader module using
> request_module() if a loader for the requested program is not yet loaded. But
> second attempt of request_module() does not affect the result of
> search_binary_handler().
> 
> If request_module() triggered recursion, calling request_module() twice causes
> 2 to the power of MAX_KMOD_CONCURRENT (= 50) repetitions. It is not an infinite
> loop but is sufficient for users to consider as a hang up.
> 
> Therefore, this patch changes not to call request_module() twice, making
> 1 to the power of MAX_KMOD_CONCURRENT repetitions in case of recursion.
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> diff --git a/fs/exec.c b/fs/exec.c
> index 6075a1e..044c13f 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1411,6 +1411,8 @@ int search_binary_handler(struct linux_binprm *bprm,struct pt_regs *regs)
>  			    printable(bprm->buf[2]) &&
>  			    printable(bprm->buf[3]))
>  				break; /* -ENOEXEC */
> +			if (try)
> +				break; /* -ENOEXEC */
>  			request_module("binfmt-%04x", *(unsigned short *)(&bprm->buf[2]));
>  #endif
>  		}

What is that "for (try=0; try<2" loop actually there for?  To retry
after the request_module(), it appears.  Which is pointless if
!CONFIG_MODULES.

Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()

[Tetsuo Handa]

Andrew Morton wrote:
> What is that "for (try=0; try<2" loop actually there for?  To retry
> after the request_module(), it appears.  Which is pointless if
> !CONFIG_MODULES.

Indeed. This loop was introduced in Linux 1.3.57 (where CONFIG_KERNELD was
introduced). Comparing between 1.3.56 and 1.3.57, there is no reason to retry
if !CONFIG_KERNELD, for get_fs_type() in fs/super.c and dev_get() in
net/core/dev.c do not retry if !CONFIG_KERNELD.

struct file_system_type *get_fs_type(const char *name)
{
	struct file_system_type * fs = file_systems;
	
	if (!name)
		return fs;
	for (fs = file_systems; fs && strcmp(fs->name, name); fs = fs->next)
		;
#ifdef CONFIG_KERNELD
	if (!fs && (request_module(name) == 0)) {
		for (fs = file_systems; fs && strcmp(fs->name, name); fs = fs->next)
			;
	}
#endif

	return fs;
}

struct device *dev_get(const char *name)
{
	struct device *dev;

	for (dev = dev_base; dev != NULL; dev = dev->next) 
	{
		if (strcmp(dev->name, name) == 0)
			return(dev);
	}
#ifdef CONFIG_KERNELD
	if (request_module(name) == 0)
		for (dev = dev_base; dev != NULL; dev = dev->next) {
			if (strcmp(dev->name, name) == 0)
				return(dev);
		}
#endif
	return(NULL);
}

It is likely that we have been doing needless retry if !CONFIG_MODULES.
----------------------------------------
[PATCH] exec: Do not retry load_binary method if CONFIG_MODULES=n.

If CONFIG_MODULES=n, it makes no sense to retry the list of binary formats
handler because the list will not be modified by request_module().

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
diff --git a/fs/exec.c b/fs/exec.c
index 6075a1e..46c399f 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1401,9 +1401,9 @@ int search_binary_handler(struct linux_binprm *bprm,struct pt_regs *regs)
 			}
 		}
 		read_unlock(&binfmt_lock);
+#ifdef CONFIG_MODULES
 		if (retval != -ENOEXEC || bprm->mm == NULL) {
 			break;
-#ifdef CONFIG_MODULES
 		} else {
 #define printable(c) (((c)=='\t') || ((c)=='\n') || (0x20<=(c) && (c)<=0x7e))
 			if (printable(bprm->buf[0]) &&
@@ -1412,8 +1412,10 @@ int search_binary_handler(struct linux_binprm *bprm,struct pt_regs *regs)
 			    printable(bprm->buf[3]))
 				break; /* -ENOEXEC */
 			request_module("binfmt-%04x", *(unsigned short *)(&bprm->buf[2]));
-#endif
 		}
+#else
+		break;
+#endif
 	}
 	return retval;
 }