Bug#524940: module-init-tools: modprobe starts fork-bombing on

Bug#524940: module-init-tools: modprobe starts fork-bombing on

[Marco d'Itri]

[-- Attachment #1: Type: text/plain, Size: 16 bytes --]

-- 
ciao,
Marco

[-- Attachment #2: Type: message/rfc822, Size: 11854 bytes --]

[-- Attachment #2.1.1.1: Type: Text/Plain, Size: 1446 bytes --]

tags 524940 - help
tags 524940 patch
thanks

Hello,

On trečiadienis 05 Rugpjūtis 2009 03:05:06 Marco d'Itri wrote:
> On Aug 05, Eli Mackenzie <eli.mackenzie@gmail.com> wrote:
> > With this bug marked "important", apt-listbugs didn't notify me that
> > there was anything wrong with this package. A failure to boot seems to
> > fit the "makes unrelated software on the system (or the whole system)
> > break" description of "critical" more than "important".
> 
> It only affects a small number of users.
> BTW, the upstream maintainer does not appear to have time to investigate
> this. Feel free to send patches.

The bug can only be reproduced with kernel 2.6.19 or earlier (i.e. the kernel 
in etch). If /sys/module/*/initstate (added in 2.6.20) is not present, current 
modprobe assumes that the module is not loaded. This triggers recursive "fork 
bomb" as modprobe keeps trying to load snd-pcm (i.e. keeps executing that 
custom install command of snd-pcm) again and again. More detailed info about 
this can be found in the patch headers.

To reproduce, either install oss-compat under <= 2.6.19 kernel or reboot to <= 
2.6.19 with oss-compat installed (init=/bin/bash may be helpful to get system 
bootable in this case).

Actually, you need 0002 patch to fix this bug, 0001 is just a safety 
precaution. Please submit both these patches to upstream. Thanks.

-- 
Modestas Vainius <modestas@vainius.eu>

[-- Attachment #2.1.1.2: 0002-Get-module-initstate-from-proc-modules-if-it-is-not-.patch --]
[-- Type: text/x-patch, Size: 3485 bytes --]

From 567f889a3c8d50234abef72b10e3af9312aa808f Mon Sep 17 00:00:00 2001
From: Modestas Vainius <modestas@vainius.eu>
Date: Sun, 20 Sep 2009 14:41:48 +0300
Subject: [PATCH 2/2] Get module initstate from /proc/modules if it is not supported via sysfs.

Get module state from /proc/modules if it is not supported via sysfs.
/sys/module/*/initstate is only available since Linux 2.6.20. Current code
assumes that module is not in the kernel if initstate attribute is absent.
Among other potential problems, this may trigger a "fork bomb" if certain
custom "install" sequences are used for the module and/or its dependencies.
 .
The patch adds a fallback to /proc/modules in order to get the module state if
/sys/module/*/initstate attribute is missing. The code is based on the
module_in_kernel() function from v3.4).

"Fork bombing" side effect of the bug is described in [1]. What's more,
handling of module_in_kernel() failures could still be improved in order avoid
similar problems when /sys is not mounted. Currently, if module_in_kernel()
fails with -1, modprobe assumes the module is not present in the kernel.

1. http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=524940

Signed-off-by: Modestas Vainius <modestas@vainius.eu>
---
 modprobe.c |   52 +++++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/modprobe.c b/modprobe.c
index ec46e0b..06825bd 100644
--- a/modprobe.c
+++ b/modprobe.c
@@ -537,6 +537,42 @@ static int read_attribute(const char *filename, char *buf, size_t buflen)
 	return (s == NULL) ? -1 : 1;
 }
 
+/* Since /sys/module/%s/initstate is only available since 2.6.20,
+   fallback to /proc/modules to get module state on earlier kernels.
+   0 means module is not live, 1 means yes, -1 means unknown.
+ */
+static int proc_is_module_live(const char *modname)
+{
+	FILE *proc_modules;
+	char *line;
+	int i, ret;
+
+	/* Might not be mounted yet.  Don't fail. */
+	proc_modules = fopen("/proc/modules", "r");
+	if (!proc_modules)
+		return -1;
+
+	while ((line = getline_wrapped(proc_modules, NULL)) != NULL) {
+		char *entry = strtok(line, " \n");
+
+		if (entry && streq(entry, modname)) {
+			/* If it exists, initstate is the fifth entry. */
+            for (i = 1; i < 5; i++) {
+				if (!(entry = strtok(NULL, " \n")))
+					break;
+			}
+			ret = !(entry && (streq(entry, "Loading") || streq(entry, "Unloading")));
+
+			free(line);
+			fclose(proc_modules);
+			return ret;
+		}
+		free(line);
+	}
+	fclose(proc_modules);
+	return 0;
+}
+
 /* Is module in /sys/module?  If so, fill in usecount if not NULL. 
    0 means no, 1 means yes, -1 means unknown.
  */
@@ -562,10 +598,20 @@ static int module_in_kernel(const char *modname, unsigned int *usecount)
 
 	/* Wait for the existing module to either go live or disappear. */
 	nofail_asprintf(&name, "/sys/module/%s/initstate", modname);
+	ret = 1;
 	while (1) {
-		ret = read_attribute(name, attr, ATTR_LEN);
-		if (ret != 1 || streq(attr, "live\n"))
-			break;
+		/* If ret == 0 upon second and subsequent cycles,
+		   /sys/module/%s/initstate is not supported. Do not try again. */
+		if (ret != 0)
+			ret = read_attribute(name, attr, ATTR_LEN);
+		if (ret == 1) {
+			if (streq(attr, "live\n"))
+				break;
+		} else {
+			ret = proc_is_module_live(modname);
+			if (ret != 0)
+				break;
+		}
 
 		usleep(100000);
 	}
-- 
1.6.4.3


[-- Attachment #2.1.1.3: 0001-Ignore-custom-install-commands-if-failed-to-find-whe.patch --]
[-- Type: text/x-patch, Size: 2073 bytes --]

From 47a0c951d41f586f4e8dfeef3e770449a0fb2570 Mon Sep 17 00:00:00 2001
From: Modestas Vainius <modestas@vainius.eu>
Date: Sun, 20 Sep 2009 16:15:44 +0300
Subject: [PATCH 1/2] Ignore custom install commands if failed to find whether module is loaded.

This should protect from disastrous consequences like in [1] if for some
reason module_in_kernel() status cannot be determined.

1. http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=524940

Signed-off-by: Modestas Vainius <modestas@vainius.eu>
---
 modprobe.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/modprobe.c b/modprobe.c
index 21a3111..ec46e0b 100644
--- a/modprobe.c
+++ b/modprobe.c
@@ -1095,6 +1095,7 @@ static int insmod(struct list_head *list,
 	const char *command;
 	struct module *mod = list_entry(list->next, struct module, list);
 	int rc = 0;
+	int isloaded = 0;
 
 	/* Take us off the list. */
 	list_del(&mod->list);
@@ -1122,7 +1123,7 @@ static int insmod(struct list_head *list,
 
 	/* Don't do ANYTHING if already in kernel. */
 	if (!(flags & mit_ignore_loaded)
-	    && module_in_kernel(newname ?: mod->modname, NULL) == 1) {
+	    && (isloaded = module_in_kernel(newname ?: mod->modname, NULL) == 1)) {
 		if (flags & mit_first_time)
 			error("Module %s already in kernel.\n",
 			      newname ?: mod->modname);
@@ -1131,10 +1132,15 @@ static int insmod(struct list_head *list,
 
 	command = find_command(mod->modname, commands);
 	if (command && !(flags & mit_ignore_commands)) {
-		close_file(fd);
-		do_command(mod->modname, command, flags & mit_dry_run, error,
-			   "install", cmdline_opts);
-		goto out_optstring;
+		if (isloaded == -1) {
+			error("Unable to find if module %s is loaded. Assuming --ignore-install.\n",
+			      newname ?: mod->modname);
+		} else {
+			close_file(fd);
+			do_command(mod->modname, command, flags & mit_dry_run, error,
+				   "install", cmdline_opts);
+			goto out_optstring;
+		}
 	}
 
 	module = grab_elf_file_fd(mod->filename, fd);
-- 
1.6.4.3


[-- Attachment #2.1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: Bug#524940: module-init-tools: modprobe starts fork-bombing on

[Alan Jenkins]

CC: linux-modules

You haven't added your Signed-off-by; I believe it is customary for
one to do so when forwarding patches on behalf of another.

I will try to grok the code later.  In general, I think the first
"safety precaution" patch is a really good idea.  The second patch has
me confused right now; it would be nice if the code explained _why_ it
calls read_attribute() twice before falling back to /proc/modules.

Also we should try to ressurect the old test cases covering /proc/modules.

Thanks
Alan

<less useful verbiage follows>
...
> What's more,
> handling of module_in_kernel() failures could still be improved in order avoid
> similar problems when /sys is not mounted. Currently, if module_in_kernel()
> fails with -1, modprobe assumes the module is not present in the kernel.

Heh.  ISTR a comment along the lines of 'if we're not sure, it's safe
to assume the module is not present'.

You're coy about which 'custom "install" sequences' trigger this fork
bomb...  ah, I see it

oss-compat.conf:
install snd-pcm modprobe --ignore-install snd-pcm $CMDLINE_OPTS && {
modprobe --quiet snd-pcm-oss ; : ; }

mount --bind /mnt/empty /sys
modprobe snd-pcm
... (spins creating little modprobes)

and the cycle comes about because snd-pcm-oss depends on snd-pcm.

Adding "--ignore-install" to the second command wouldn't help, because
that option does not affect modules loaded to satisfy dependencies.

Andreas Robinson started some work to replace "install" commands like
this with a "soft dependency" directive, which included detection of
infinite recursion.  I moaned about his initial proof-of-concept and
nothing further has been posted.  I don't want to nag; I'll just say
that would be a really nice way to make sure these forkbombs can't
happen (provided the config files are updated to take advantage of
it).

Re: Bug#524940: module-init-tools: modprobe starts fork-bombing on

[Andreas Robinson]

On Wed, Sep 23, 2009 at 11:26 PM, Alan Jenkins
<sourcejedi.lkml@googlemail.com> wrote:

> Andreas Robinson started some work to replace "install" commands like
> this with a "soft dependency" directive, which included detection of
> infinite recursion.  I moaned about his initial proof-of-concept and
> nothing further has been posted.  I don't want to nag; I'll just say
> that would be a really nice way to make sure these forkbombs can't
> happen (provided the config files are updated to take advantage of
> it).
>

I haven't abandoned it. Your objections made it a more challenging
project though, so I felt it was safer to take a break from it before
things got really messy. :-)

I don't mind getting back to it though, now that it's a more urgent
problem. I should be able to work on it again tonight.

Cheers,
Andreas

Re: Bug#524940: module-init-tools: modprobe starts fork-bombing on

[Alan Jenkins]

On 9/24/09, Andreas Robinson <andr345@gmail.com> wrote:
> On Wed, Sep 23, 2009 at 11:26 PM, Alan Jenkins
> <sourcejedi.lkml@googlemail.com> wrote:
>
>> Andreas Robinson started some work to replace "install" commands like
>> this with a "soft dependency" directive, which included detection of
>> infinite recursion.  I moaned about his initial proof-of-concept and
>> nothing further has been posted.  I don't want to nag; I'll just say
>> that would be a really nice way to make sure these forkbombs can't
>> happen (provided the config files are updated to take advantage of
>> it).
>>
>
> I haven't abandoned it. Your objections made it a more challenging
> project though, so I felt it was safer to take a break from it before
> things got really messy. :-)
>
> I don't mind getting back to it though, now that it's a more urgent
> problem. I should be able to work on it again tonight.

I don't think it is urgent.  Hopefully we can use the two patches
Marco forwarded to the linux-hotplug ML [1].

Basically, I wanted to inform (or remind?) Marco that we have another
idea which could help in the long run.  The changelog in the second
patch here suggested that there was still room for improvement; I
thought I should mention existing ideas in the same area.  This bug
suggests another practical reason to work on them, but not a pressing
one.

Best wishes... and whatever you do tonight, remember to sleep :).
Alan

[1] <http://www.spinics.net/lists/hotplug/msg02735.html>