* [PATCH][Resend v2] Fix infinite loop in search_binary_handler()
@ 2011-07-04 11:30 Richard Weinberger
2011-07-04 11:51 ` Tetsuo Handa
0 siblings, 1 reply; 18+ messages in thread
From: Richard Weinberger @ 2011-07-04 11:30 UTC (permalink / raw)
To: viro; +Cc: akpm, linux-kernel, linux-fsdevel, 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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()
2011-07-04 11:30 [PATCH][Resend v2] Fix infinite loop in search_binary_handler() Richard Weinberger
@ 2011-07-04 11:51 ` Tetsuo Handa
2011-07-04 11:57 ` Richard Weinberger
0 siblings, 1 reply; 18+ messages in thread
From: Tetsuo Handa @ 2011-07-04 11:51 UTC (permalink / raw)
To: richard; +Cc: viro, akpm, linux-kernel, linux-fsdevel
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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()
2011-07-04 11:51 ` Tetsuo Handa
@ 2011-07-04 11:57 ` Richard Weinberger
2011-07-04 12:10 ` Tetsuo Handa
0 siblings, 1 reply; 18+ messages in thread
From: Richard Weinberger @ 2011-07-04 11:57 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: viro, akpm, linux-kernel, linux-fsdevel
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()
2011-07-04 11:57 ` Richard Weinberger
@ 2011-07-04 12:10 ` Tetsuo Handa
2011-07-04 12:20 ` Richard Weinberger
0 siblings, 1 reply; 18+ messages in thread
From: Tetsuo Handa @ 2011-07-04 12:10 UTC (permalink / raw)
To: richard; +Cc: viro, akpm, linux-kernel, linux-fsdevel
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?
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()
2011-07-04 12:10 ` Tetsuo Handa
@ 2011-07-04 12:20 ` Richard Weinberger
2011-07-04 14:42 ` Tetsuo Handa
0 siblings, 1 reply; 18+ messages in thread
From: Richard Weinberger @ 2011-07-04 12:20 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: viro, akpm, linux-kernel, linux-fsdevel
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()
2011-07-04 12:20 ` Richard Weinberger
@ 2011-07-04 14:42 ` Tetsuo Handa
2011-07-04 14:59 ` Richard Weinberger
0 siblings, 1 reply; 18+ messages in thread
From: Tetsuo Handa @ 2011-07-04 14:42 UTC (permalink / raw)
To: richard; +Cc: viro, akpm, linux-kernel, linux-fsdevel
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.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()
2011-07-04 14:42 ` Tetsuo Handa
@ 2011-07-04 14:59 ` Richard Weinberger
2011-07-04 15:07 ` Richard Weinberger
0 siblings, 1 reply; 18+ messages in thread
From: Richard Weinberger @ 2011-07-04 14:59 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: viro, akpm, linux-kernel, linux-fsdevel
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()
2011-07-04 14:59 ` Richard Weinberger
@ 2011-07-04 15:07 ` Richard Weinberger
2011-07-04 22:03 ` Tetsuo Handa
0 siblings, 1 reply; 18+ messages in thread
From: Richard Weinberger @ 2011-07-04 15:07 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: viro, akpm, linux-kernel, linux-fsdevel
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()
2011-07-04 15:07 ` Richard Weinberger
@ 2011-07-04 22:03 ` Tetsuo Handa
2011-07-04 22:17 ` Richard Weinberger
0 siblings, 1 reply; 18+ messages in thread
From: Tetsuo Handa @ 2011-07-04 22:03 UTC (permalink / raw)
To: richard; +Cc: viro, akpm, linux-kernel, linux-fsdevel
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()
2011-07-04 22:03 ` Tetsuo Handa
@ 2011-07-04 22:17 ` Richard Weinberger
2011-07-05 1:24 ` Tetsuo Handa
0 siblings, 1 reply; 18+ messages in thread
From: Richard Weinberger @ 2011-07-04 22:17 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: viro, akpm, linux-kernel, linux-fsdevel
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()
2011-07-04 22:17 ` Richard Weinberger
@ 2011-07-05 1:24 ` Tetsuo Handa
2011-07-05 9:55 ` Richard Weinberger
0 siblings, 1 reply; 18+ messages in thread
From: Tetsuo Handa @ 2011-07-05 1:24 UTC (permalink / raw)
To: richard; +Cc: viro, akpm, linux-kernel, linux-fsdevel
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;
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()
2011-07-05 1:24 ` Tetsuo Handa
@ 2011-07-05 9:55 ` Richard Weinberger
2011-07-05 12:02 ` Tetsuo Handa
0 siblings, 1 reply; 18+ messages in thread
From: Richard Weinberger @ 2011-07-05 9:55 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: viro, akpm, linux-kernel, linux-fsdevel
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()
2011-07-05 9:55 ` Richard Weinberger
@ 2011-07-05 12:02 ` Tetsuo Handa
2011-07-05 12:21 ` Richard Weinberger
0 siblings, 1 reply; 18+ messages in thread
From: Tetsuo Handa @ 2011-07-05 12:02 UTC (permalink / raw)
To: richard; +Cc: viro, akpm, linux-kernel, linux-fsdevel
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?
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()
2011-07-05 12:02 ` Tetsuo Handa
@ 2011-07-05 12:21 ` Richard Weinberger
2011-07-06 11:28 ` Tetsuo Handa
0 siblings, 1 reply; 18+ messages in thread
From: Richard Weinberger @ 2011-07-05 12:21 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: viro, akpm, linux-kernel, linux-fsdevel
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()
2011-07-05 12:21 ` Richard Weinberger
@ 2011-07-06 11:28 ` Tetsuo Handa
2011-07-06 11:36 ` Richard Weinberger
2011-07-06 20:21 ` Andrew Morton
0 siblings, 2 replies; 18+ messages in thread
From: Tetsuo Handa @ 2011-07-06 11:28 UTC (permalink / raw)
To: richard; +Cc: viro, akpm, linux-kernel, linux-fsdevel
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
}
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()
2011-07-06 11:28 ` Tetsuo Handa
@ 2011-07-06 11:36 ` Richard Weinberger
2011-07-06 20:21 ` Andrew Morton
1 sibling, 0 replies; 18+ messages in thread
From: Richard Weinberger @ 2011-07-06 11:36 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: viro, akpm, linux-kernel, linux-fsdevel
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()
2011-07-06 11:28 ` Tetsuo Handa
2011-07-06 11:36 ` Richard Weinberger
@ 2011-07-06 20:21 ` Andrew Morton
2011-07-07 4:04 ` Tetsuo Handa
1 sibling, 1 reply; 18+ messages in thread
From: Andrew Morton @ 2011-07-06 20:21 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: richard, viro, linux-kernel, linux-fsdevel
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.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH][Resend v2] Fix infinite loop in search_binary_handler()
2011-07-06 20:21 ` Andrew Morton
@ 2011-07-07 4:04 ` Tetsuo Handa
0 siblings, 0 replies; 18+ messages in thread
From: Tetsuo Handa @ 2011-07-07 4:04 UTC (permalink / raw)
To: akpm; +Cc: richard, viro, linux-kernel, linux-fsdevel
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;
}
^ permalink raw reply related [flat|nested] 18+ messages in thread
end of thread, other threads:[~2011-07-07 4:06 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-07-04 11:30 [PATCH][Resend v2] Fix infinite loop in search_binary_handler() Richard Weinberger
2011-07-04 11:51 ` Tetsuo Handa
2011-07-04 11:57 ` Richard Weinberger
2011-07-04 12:10 ` Tetsuo Handa
2011-07-04 12:20 ` Richard Weinberger
2011-07-04 14:42 ` Tetsuo Handa
2011-07-04 14:59 ` Richard Weinberger
2011-07-04 15:07 ` Richard Weinberger
2011-07-04 22:03 ` Tetsuo Handa
2011-07-04 22:17 ` Richard Weinberger
2011-07-05 1:24 ` Tetsuo Handa
2011-07-05 9:55 ` Richard Weinberger
2011-07-05 12:02 ` Tetsuo Handa
2011-07-05 12:21 ` Richard Weinberger
2011-07-06 11:28 ` Tetsuo Handa
2011-07-06 11:36 ` Richard Weinberger
2011-07-06 20:21 ` Andrew Morton
2011-07-07 4:04 ` Tetsuo Handa
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.