Fix undefined/miscompiled construct in kernel parameters
diff mbox series

Message ID 20030615125854.GA29458@averell
State New, archived
Headers show
Series
  • Fix undefined/miscompiled construct in kernel parameters
Related show

Commit Message

Andi Kleen June 15, 2003, 12:58 p.m. UTC
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



-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Comments

Linus Torvalds June 15, 2003, 4:52 p.m. UTC | #1
On Sun, 15 Jun 2003, Andi Kleen wrote:
> 
> 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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Richard Henderson June 16, 2003, 12:49 a.m. UTC | #2
On Mon, Jun 16, 2003 at 10:23:41AM +1000, Rusty Russell wrote:
> 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~
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patch
diff mbox series

diff -burpN -X ../KDIFX linux/include/linux/moduleparam.h linux-2.5.71-amd64/include/linux/moduleparam.h
--- linux/include/linux/moduleparam.h	2003-05-27 03:00:39.000000000 +0200
+++ linux-2.5.71-amd64/include/linux/moduleparam.h	2003-06-15 14:49:45.000000000 +0200
@@ -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 */
diff -burpN -X ../KDIFX linux/init/main.c linux-2.5.71-amd64/init/main.c
--- linux/init/main.c	2003-06-14 23:43:06.000000000 +0200
+++ linux-2.5.71-amd64/init/main.c	2003-06-15 13:16:41.000000000 +0200
@@ -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();
diff -burpN -X ../KDIFX linux/kernel/module.c linux-2.5.71-amd64/kernel/module.c
--- linux/kernel/module.c	2003-06-14 23:43:06.000000000 +0200
+++ linux-2.5.71-amd64/kernel/module.c	2003-06-15 13:18:49.000000000 +0200
@@ -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)
diff -burpN -X ../KDIFX linux/kernel/params.c linux-2.5.71-amd64/kernel/params.c
--- linux/kernel/params.c	2003-05-27 03:00:46.000000000 +0200
+++ linux-2.5.71-amd64/kernel/params.c	2003-06-15 13:39:16.000000000 +0200
@@ -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",