Reduce stack usage in module.c

Attempt to reduce stack usage in module.c (linux-2.6.12-rc1-mm3). Specifically from checkstack.pl

Before patch
------------------
who_is_doing_it: 512 obsolete_params: 160

After patch
----------------
who_is_doing_it: none

obsolete_params: 12

Also while at it, fix following in who_is_doing_it(...)
- use only as much memory is needed
- do not write past array index for the boundary case

module.c renamed from module.c.orig +21 -7

-	for (i = 0; i < num; i++) {
-		char sym_name[128 + sizeof(MODULE_SYMBOL_PREFIX)];
+	if (num) {
+		sym_name_len = 128 + sizeof (MODULE_SYMBOL_PREFIX);
+		sym_name = kmalloc(sym_name_len, GFP_KERNEL);
+		if (!sym_name) {
+			ret = -ENOMEM;
+			goto free_kp;
+		}
+	}
 
-		snprintf(sym_name, sizeof(sym_name), "%s%s",
+	for (i = 0; i < num; i++) {
+		
+		snprintf(sym_name, sym_name_len, "%s%s",
 			 MODULE_SYMBOL_PREFIX, obsparm[i].name);
 
 		kp[i].name = obsparm[i].name;

@@ -791,13 +801,15 @@
 			printk("%s: falsely claims to have parameter %s\n",
 			       name, obsparm[i].name);
 			ret = -EINVAL;
-			goto out;
+			goto free_sym;
 		}
 		kp[i].arg = &obsparm[i];
 	}
 
 	ret = parse_args(name, args, kp, num, NULL);
- out:
+ free_sym:
+	kfree(sym_name);
+ free_kp:
 	kfree(kp);
 	return ret;
 }

@@ -1399,12 +1411,16 @@
 static void who_is_doing_it(void)
 {
 	/* Print out all the args. */
-	char args[512];
+	char *args;
 	unsigned long i, len = current->mm->arg_end - current->mm->arg_start;
 
 	if (len > 512)
 		len = 512;
 
+	args = kmalloc(len + 1, GFP_KERNEL);
+	if (!args)
+		return;
+
 	len -= copy_from_user(args, (void *)current->mm->arg_start, len);
 
 	for (i = 0; i < len; i++) {

@@ -1413,6 +1429,7 @@
 	}
 	args[i] = 0;
 	printk("ARGS: %s\n", args);
+	kfree(args);
 }
 
 /* Allocate and load the module: note that size of section 0 is always
-

Quoted 8 lines · Yum Rayan

Attempt to reduce stack usage in module.c (linux-2.6.12-rc1-mm3). Specifically from checkstack.pl

Before patch
------------------
who_is_doing_it: 512
obsolete_params: 160

Quoted 3 lines

After patch
----------------
who_is_doing_it: none

So all function local variables are in registers?

Quoted 3 lines

Also while at it, fix following in who_is_doing_it(...)
- use only as much memory is needed
- do not write past array index for the boundary case

I don't see a boundary case problem with the current code, hence I don't see why the kmalloc(len + 1, GFP_KERNEL) is needed...

File names start one level deeper than wanted. They should begin with linux/ or a/ or ./ e.g.
There are plenty of docs on this, please let me know if you need references to them.

Review line 777

Quoted 11 lines

(quoted diff) +2 -0

@@ -769,15 +769,25 @@
      struct kernel_param *kp;
      unsigned int i;
      int ret;
+     char *sym_name = NULL;
+     unsigned int sym_name_len = 0;
 
      kp = kmalloc(sizeof(kp[0]) * num, GFP_KERNEL);
      if (!kp)
              return -ENOMEM;

Style thing, I guess, but since the case of num == 0 doesn't do anything here, I would just begin the function with:

       if (!num)
               return;

or goto out;
to maintain one return point.

and then eliminate the kmalloc()s, if (num), kfree()s, and parse_args().

Quoted 5 lines · Randy.Dunlap

Quoted 2 lines · Yum Rayan

Attempt to reduce stack usage in module.c (linux-2.6.12-rc1-mm3). Specifically from checkstack.pl

Quoted 7 lines

Quoted 3 lines

Also while at it, fix following in who_is_doing_it(...)
- use only as much memory is needed
- do not write past array index for the boundary case

I don't see a boundary case problem with the current code, hence I don't see why the kmalloc(len + 1, GFP_KERNEL) is needed...

Let's consider the original code and len = 513

   1399 static void who_is_doing_it(void)
   1400 {
   1401         /* Print out all the args. */
   1402         char args[512];
   1403         unsigned long i, len = current->mm->arg_end -
current->mm->arg_start;
   1404
   1405         if (len > 512)
   1406                 len = 512;
   1407
   1408         len -= copy_from_user(args, (void
*)current->mm->arg_start, len);
   1409
   1410         for (i = 0; i < len; i++) {
   1411                 if (args[i] == '\0')
   1412                         args[i] = ' ';
   1413         }
   1414         args[i] = 0;
   1415         printk("ARGS: %s\n", args);
   1416 }

After lines 1410 thru 1413, "i" wil be 512. So line 1414 will be "args[512] = 0". But args is 512 byte array with last legally accessible element at 511?

Quoted 4 lines

File names start one level deeper than wanted. They should begin with linux/ or a/ or ./ e.g.
There are plenty of docs on this, please let me know if you need references to them.

Point noted. Will post patch to linux/Documentation/SubmittingPatches, hopefully making it more clear. Reworked patch at end of email.

Quoted 32 lines · Yum Rayan

Quoted 7 lines · Randy.Dunlap

Quoted 1 line · Yum Rayan

- do not write past array index for the boundary case

I don't see a boundary case problem with the current code, hence I don't see why the kmalloc(len + 1, GFP_KERNEL) is needed...

   1399 static void who_is_doing_it(void)
   1400 {
   1401         /* Print out all the args. */
   1402         char args[512];
   1403         unsigned long i, len = current->mm->arg_end -
current->mm->arg_start;
   1404
   1405         if (len > 512)
   1406                 len = 512;
   1407
   1408         len -= copy_from_user(args, (void
*)current->mm->arg_start, len);
   1409
   1410         for (i = 0; i < len; i++) {
   1411                 if (args[i] == '\0')
   1412                         args[i] = ' ';
   1413         }
   1414         args[i] = 0;
   1415         printk("ARGS: %s\n", args);
   1416 }

Quoted 5 lines

Let's consider the original code and len = 513

After lines 1410 thru 1413, "i" wil be 512. So line 1414 will be "args[512] = 0". But args is 512 byte array with last legally accessible element at 511?

Yes, it's so obvious (now). :)