Skip to contentSkip to conversation

Fix undefined/miscompiled construct in kernel parameters

PATCH In review Andi Kleen 4 participants 6 messages Jun 15, 2003 – Jun 16, 2003 Module support

lore
Share & tools
lore

Andi Kleen

author
#0 lore raw

PATCH 1/1 Fix undefined/miscompiled construct in kernel parameters

The parse_args call in init/main.c does pointer arithmetic between two different external symbols. This is undefined in C (only pointer arthmetic in the same symbol is defined) and gets miscompiled on AMD64 with gcc 3.2, resulting in a triple fault at boot when an driver with new style parameters is compiled in. In my case it was triggered by oprofile.

This patch works around this by passing the end pointer directly to the low level function and comparing it there. Strictly this is still undefined, but should be ok because gcc has to handle these pointers in another function correctly.

It is also possible to use an empty asm() with dummy input/out to work around this, but I didn't do that for now.

The construct may appear in other cases too, but I didn't see any miscompilation so far.

Please apply,

-Andi

include/linux/moduleparam.h +1 -1
diff -burpN -X ../KDIFX linux/include/linux/moduleparam.h linux-2.5.71-amd64/include/linux/moduleparam.h
@@ -67,7 +67,7 @@ struct kparam_string {
 extern int parse_args(const char *name,
 		      char *args,
 		      struct kernel_param *params,
-		      unsigned num,
+		      struct kernel_param *end,
 		      int (*unknown)(char *param, char *val));
 
 /* All the helper functions */
init/main.c +1 -1
diff -burpN -X ../KDIFX linux/init/main.c linux-2.5.71-amd64/init/main.c
@@ -404,7 +404,7 @@ asmlinkage void __init start_kernel(void
 	page_alloc_init();
 	printk("Kernel command line: %s\n", saved_command_line);
 	parse_args("Booting kernel", command_line, &__start___param,
-		   &__stop___param - &__start___param,
+		   &__stop___param,
 		   &unknown_bootoption);
 	trap_init();
 	rcu_init();
kernel/module.c +6 -6
diff -burpN -X ../KDIFX linux/kernel/module.c linux-2.5.71-amd64/kernel/module.c
@@ -929,7 +929,7 @@ static int obsolete_params(const char *n
 		kp[i].arg = &obsparm[i];
 	}
 
-	ret = parse_args(name, args, kp, num, NULL);
+	ret = parse_args(name, args, kp, &kp[num], NULL);
  out:
 	kfree(kp);
 	return ret;

@@ -1622,11 +1622,11 @@ static struct module *load_module(void _
 				      (char *)sechdrs[strindex].sh_addr);
 	} else {
 		/* Size of section 0 is 0, so this works well if no params */
-		err = parse_args(mod->name, mod->args,
-				 (struct kernel_param *)
-				 sechdrs[setupindex].sh_addr,
-				 sechdrs[setupindex].sh_size
-				 / sizeof(struct kernel_param),
+		struct kernel_param *parm = (struct kernel_param *)
+					 sechdrs[setupindex].sh_addr;	
+		err = parse_args(mod->name, mod->args, parm,
+				&parm[sechdrs[setupindex].sh_size
+				 / sizeof(struct kernel_param)],
 				 NULL);
 	}
 	if (err < 0)
kernel/params.c +4 -4
diff -burpN -X ../KDIFX linux/kernel/params.c linux-2.5.71-amd64/kernel/params.c
@@ -46,13 +46,13 @@ static inline int parameq(const char *in
 static int parse_one(char *param,
 		     char *val,
 		     struct kernel_param *params, 
-		     unsigned num_params,
+		     struct kernel_param *end,
 		     int (*handle_unknown)(char *param, char *val))
 {
 	unsigned int i;
 
 	/* Find parameter */
-	for (i = 0; i < num_params; i++) {
+	for (i = 0; &params[i] < end; i++) {
 		if (parameq(param, params[i].name)) {
 			DEBUGP("They are equal!  Calling %p\n",
 			       params[i].set);

@@ -109,7 +109,7 @@ static char *next_arg(char *args, char *
 int parse_args(const char *name,
 	       char *args,
 	       struct kernel_param *params,
-	       unsigned num,
+	       struct kernel_param *end,
 	       int (*unknown)(char *param, char *val))
 {
 	char *param, *val;

@@ -120,7 +120,7 @@ int parse_args(const char *name,
 		int ret;
 
 		args = next_arg(args, &param, &val);
-		ret = parse_one(param, val, params, num, unknown);
+		ret = parse_one(param, val, params, end, unknown);
 		switch (ret) {
 		case -ENOENT:
 			printk(KERN_ERR "%s: Unknown parameter `%s'\n",
#1 lore raw
Quoted 4 lines · Andi Kleen

The parse_args call in init/main.c does pointer arithmetic between two different external symbols. This is undefined in C (only pointer arthmetic in the same symbol is defined) and gets miscompiled on AMD64 with gcc 3.2,

That's silly. You're making the code less readable, with some silly parameters. Why does it get miscompiled on amd64, and let's just fix that one.

Linus

#2 lore raw

In message <[email protected]> you wri te:

Quoted 10 lines · Andi Kleen
Quoted 4 lines

The parse_args call in init/main.c does pointer arithmetic between two different external symbols. This is undefined in C (only pointer arthmetic in the same symbol is defined) and gets miscompiled on AMD64 with gcc 3.2,

That's silly. You're making the code less readable, with some silly parameters. Why does it get miscompiled on amd64, and let's just fix that one.

AFAICT, Roman's fix is correct; Richard admonished me in the past for such code, IIRC, but this one slipped through.

Since Andi reports that even that doesn't work for x86-64, I'd say apply this patch based on his: it's an arbitrary change anyway.

The compiler should be fixed, too, but if this is the only compiler bug which effects the kernel, we should consider ourselves lucky.

(Andi: your module.c patch was a little convoluted). Cheers,
Rusty.

include/linux/moduleparam.h +1 -1
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.71-bk1/include/linux/moduleparam.h working-2.5.71-bk1-param/include/linux/moduleparam.h
@@ -67,7 +67,7 @@ struct kparam_string {
 extern int parse_args(const char *name,
 		      char *args,
 		      struct kernel_param *params,
-		      unsigned num,
+		      struct kernel_param *end,
 		      int (*unknown)(char *param, char *val));
 
 /* All the helper functions */
init/main.c +1 -1
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.71-bk1/init/main.c working-2.5.71-bk1-param/init/main.c
@@ -404,7 +404,7 @@ asmlinkage void __init start_kernel(void
 	page_alloc_init();
 	printk("Kernel command line: %s\n", saved_command_line);
 	parse_args("Booting kernel", command_line, &__start___param,
-		   &__stop___param - &__start___param,
+		   &__stop___param,
 		   &unknown_bootoption);
 	trap_init();
 	rcu_init();
kernel/module.c +4 -5
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.71-bk1/kernel/module.c working-2.5.71-bk1-param/kernel/module.c
@@ -929,7 +929,7 @@ static int obsolete_params(const char *n
 		kp[i].arg = &obsparm[i];
 	}
 
-	ret = parse_args(name, args, kp, num, NULL);
+	ret = parse_args(name, args, kp, kp + num, NULL);
  out:
 	kfree(kp);
 	return ret;

@@ -1623,10 +1623,9 @@ static struct module *load_module(void _
 	} else {
 		/* Size of section 0 is 0, so this works well if no params */
 		err = parse_args(mod->name, mod->args,
-				 (struct kernel_param *)
-				 sechdrs[setupindex].sh_addr,
-				 sechdrs[setupindex].sh_size
-				 / sizeof(struct kernel_param),
+				 (void *)sechdrs[setupindex].sh_addr,
+				 (void *)sechdrs[setupindex].sh_addr
+				 + sechdrs[setupindex].sh_size,
 				 NULL);
 	}
 	if (err < 0)
kernel/params.c +4 -4
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.71-bk1/kernel/params.c working-2.5.71-bk1-param/kernel/params.c
@@ -46,13 +46,13 @@ static inline int parameq(const char *in
 static int parse_one(char *param,
 		     char *val,
 		     struct kernel_param *params, 
-		     unsigned num_params,
+		     struct kernel_param *end,
 		     int (*handle_unknown)(char *param, char *val))
 {
 	unsigned int i;
 
 	/* Find parameter */
-	for (i = 0; i < num_params; i++) {
+	for (i = 0; &params[i] < end; i++) {
 		if (parameq(param, params[i].name)) {
 			DEBUGP("They are equal!  Calling %p\n",
 			       params[i].set);

@@ -109,7 +109,7 @@ static char *next_arg(char *args, char *
 int parse_args(const char *name,
 	       char *args,
 	       struct kernel_param *params,
-	       unsigned num,
+	       struct kernel_param *end,
 	       int (*unknown)(char *param, char *val))
 {
 	char *param, *val;

@@ -120,7 +120,7 @@ int parse_args(const char *name,
 		int ret;
 
 		args = next_arg(args, &param, &val);
-		ret = parse_one(param, val, params, num, unknown);
+		ret = parse_one(param, val, params, end, unknown);
 		switch (ret) {
 		case -ENOENT:
 			printk(KERN_ERR "%s: Unknown parameter `%s'\n",

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

#3 lore raw
Quoted 2 lines · Rusty Russell

Since Andi reports that even that doesn't work for x86-64, I'd say apply this patch based on his: it's an arbitrary change anyway.

No, Andi located the *real* problem. The compiler was over-aligning these objects, which added padding, which broke the array semantics you were looking for. The solution is to add an attribute aligned; he's sent a patch to Linus already.

r~

#4 lore raw

In message <[email protected]> you write:

Quoted 8 lines · Rusty Russell
Quoted 2 lines

Since Andi reports that even that doesn't work for x86-64, I'd say apply this patch based on his: it's an arbitrary change anyway.

No, Andi located the *real* problem. The compiler was over-aligning these objects, which added padding, which broke the array semantics you were looking for. The solution is to add an attribute aligned; he's sent a patch to Linus already.

Thanks for the explanation. Sorry I missed it.

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

#5 lore raw
In reply to Rusty Russell
Quoted 3 lines · Rusty Russell

AFAICT, Roman's fix is correct; Richard admonished me in the past for such code, IIRC, but this one slipped through.

Roman's fix is fine, but the fact is, the original code was also fine. Yes, the C standard has all these rules about "within objects" for pointer differences, but the "objects" themselves can come from outside the compiler. As they did in this case.

(Yeah, I could see the compiler warning about cases it suspects might be separate objects, but the end result should still be the right one).

In general, I accept _local_ uglifications to work around compiler problems. But I do not accept non-local stuff like making for ugly calling conventions etc, which is why Andi's original patch was not acceptable to me.

It turns out that the real bug was somewhere in the tool chain, and the linker should either honor alignment requirements or warn about them when it cannot. I suspect in this case the alignment requirement wasn't properly passed down the chain somewhere, I dunno. The problem is fixed, but for future reference please keep this in mind when working around compiler problems.

If worst comes to worst, we'll have notes about certain compiler versions just not working. It's certainly happened before.

Linus