kernel param and KBUILD_MODNAME name-munging mess

Booting kernel 2.5.59 with the "-s" kernel boot parameter doesn't get you into single-user mode like it should.

One part of the problem is Rusty's new module option and kernel boot parameter parsing code, which changes '-' to '_' in every string. This change is not reverted when the string is found NOT to be a kernel option, with the result that "_s" is passed to init instead of "-s".

Why is this s/-/_/ stuff done at all?
I suppose it's because the string is compared with MODULE_PARAM_PREFIX, which is __stringify(KBUILD_MODNAME) ".", and KBUILD_MODNAME is the module name after s/-/_/.

And why does KBUILD_MODNAME change the name like this? A grep for KBUILD_MODNAME shows that it's only used in #ifdef and __stringify(). __stringify() macro-expands its argument before turning it to a string literal. If this is intensional, then its a bug, since it breaks module parameters to any module whose (munged) name also is a #define. (I was bitten by that myself recently when adding a module param to ide-scsi.c.)

Would anything break if we made scripts/Makefile.lib set KBUILD_MODNAME to the original module name in "", drop the __stringify() around uses of KBUILD_MODNAME, and remove the s/-/_/ from kernel/param.c ?

/Mikael

This is what Rusty had to say regd s/-/_/ for KBUILD_MODNAME.

--Vamsi.

--quote--
From: Rusty Russell <[email protected]>
To: Zwane Mwaikambo <[email protected]>
Cc: [email protected], lkml <[email protected]>
Subject: Re: [BUG] module-init-tools 0.9.3, rmmod modules with '-'
Date: Tue, 17 Dec 2002 11:17:05 +1100

In message <[email protected]> y ou write:

Yes, the filenames are unchanged. But if you modprobe snd-mixer-oss, you'll see snd_mixer_oss in /proc/modules. But rmmod "snd-mixer-oss" works as expected. Basically, the kernel and tools see them as
equivalent: anything else is a bug, please report.

BTW, this was done for (1) simplicity, (2) so KBUILD_MODNAME can be used to construct identifiers, and (3) so parameters when the module is built-in have a consistent name.

Hope that clarifies!

Try using "s" instead. This works since ever. I didn't even know, that the other option exists, too.

Regards

Ingo Oeser

Science is what we can tell a computer. Art is everything else. --- D.E.Knuth

That's a workaround for this particular case, but the name-munging is still wrong and broken.

With "foo-bar=fie-fum" passed to the kernel, "foo_bar=fie-fum" is what's put into init's environment. (I checked.)

Mikael Pettersson, Wed, Jan 22, 2003 11:20:01 +0100:

 $ foo-bar=fie-fum
  bash: foo-bar=fie-fum: command not found

 $ foo_bar=fie-fum

I just mean to say, it is not exactly usual names one get to see in an ordinary environment.

Still, i think you are right, it is somewhat unexpected.

-alex

I agree that the current behavior is unexpected and probably should be fixed. There's basically two ways:
o Pass KBUILD_MODNAME as a string without munging
o Change the setup code to not alter the command line (either use a special version of strcmp which knows about "-,_", or work on a temporary copy)

KBUILD_BASENAME needs to be an un-stringified symbol following certain conventions to make it possible to use it e.g. in include/linux/spinlock.h, that's why '-' and ',' are escaped to '_'.

However, for all I can tell, this is not true for KBUILD_MODNAME, since that seems to be only used for constructing an actual string, which of course can contain all kinds of characters. So I think using the first approach would be somewhat nicer, as it gets rid of the unintuitive "ide-cd" -> "ide_cd" munging on the kernel command line.

Just needs to be done, of course ;)

--Kai

Hi,

It might be useful later to build a list of builtin modules. Currently we do nothing if a builtin module fails to initialize, but we should disable dependent modules and remove the exported symbols. Maybe it's better to provide the module name both as label and string.

bye, Roman

In message <[email protected]> you write:

Absolutely agreed. Patch re-xmitted below.

Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Name: Avoid mangling - in parameters
Author: Rusty Russell
Status: Trivial (tested in userspace framework)

D: Mikael Pettersson points out that "-s" gets mangled to "_s" on the
D: kernel command line, even though it turns out not to be a
D: parameter.

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.59/kernel/params.c working-2.5.59-underscore/kernel/params.c
@@ -27,6 +27,22 @@
 #define DEBUGP(fmt, a...)
 #endif
 
+static inline int dash2underscore(char c)
+{
+	if (c == '-')
+		return '_';
+	return c;
+}
+
+static inline int parameq(const char *input, const char *paramname)
+{
+	unsigned int i;
+	for (i = 0; dash2underscore(input[i]) == paramname[i]; i++)
+		if (input[i] == '\0')
+			return 1;
+	return 0;
+}
+
 static int parse_one(char *param,
 		     char *val,
 		     struct kernel_param *params, 

@@ -37,7 +53,7 @@ static int parse_one(char *param,
 
 	/* Find parameter */
 	for (i = 0; i < num_params; i++) {
-		if (strcmp(param, params[i].name) == 0) {
+		if (parameq(param, params[i].name)) {
 			DEBUGP("They are equal!  Calling %p\n",
 			       params[i].set);
 			return params[i].set(val, &params[i]);

@@ -69,8 +85,6 @@ static char *next_arg(char *args, char *
 		if (equals == 0) {
 			if (args[i] == '=')
 				equals = i;
-			else if (args[i] == '-')
-				args[i] = '_';
 		}
 		if (args[i] == '"')
 			in_quote = !in_quote;

In message <[email protected]> you write:

That's a bug. Good catch. Fix below.

Basically because making - and _ equal is the only sane way to unify parameter names without pissing off users, and life for coders is much nicer when KBUILD_MODNAME is a unique valid C prefix for that module.

Thanks for the report!
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Name: Avoid mangling - in parameters
Author: Rusty Russell
Status: Trivial (tested in userspace framework)

D: Mikael Pettersson points out that "-s" gets mangled to "_s" on the
D: kernel command line, even though it turns out not to be a
D: parameter.

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.59/kernel/params.c working-2.5.59-underscore/kernel/params.c
@@ -27,6 +27,22 @@
 #define DEBUGP(fmt, a...)
 #endif
 
+static inline int dash2underscore(char c)
+{
+	if (c == '-')
+		return '_';
+	return c;
+}
+
+static inline int parameq(const char *input, const char *paramname)
+{
+	unsigned int i;
+	for (i = 0; dash2underscore(input[i]) == paramname[i]; i++)
+		if (input[i] == '\0')
+			return 1;
+	return 0;
+}
+
 static int parse_one(char *param,
 		     char *val,
 		     struct kernel_param *params, 

@@ -37,7 +53,7 @@ static int parse_one(char *param,
 
 	/* Find parameter */
 	for (i = 0; i < num_params; i++) {
-		if (strcmp(param, params[i].name) == 0) {
+		if (parameq(param, params[i].name)) {
 			DEBUGP("They are equal!  Calling %p\n",
 			       params[i].set);
 			return params[i].set(val, &params[i]);

@@ -69,8 +85,6 @@ static char *next_arg(char *args, char *
 		if (equals == 0) {
 			if (args[i] == '=')
 				equals = i;
-			else if (args[i] == '-')
-				args[i] = '_';
 		}
 		if (args[i] == '"')
 			in_quote = !in_quote;