[PATCH] Reduce stack usage in module.c

[PATCH] Reduce stack usage in module.c

[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

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

Patch is against linux-2.6.12-rc1-mm3

Thanks,
Rayan

Signed-off-by: Yum Rayan <yum.rayan@gmail.com>

--- kernel/module.c.orig	2005-03-28 22:32:35.000000000 -0800
+++ kernel/module.c	2005-03-28 22:49:26.000000000 -0800
@@ -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;
 
-	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

Re: [PATCH] Reduce stack usage in module.c

[Randy.Dunlap]

Yum Rayan wrote:
> 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
So all function local variables are in registers?

> 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

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...

> Patch is against linux-2.6.12-rc1-mm3
> 
> Thanks,
> Rayan
> 
> Signed-off-by: Yum Rayan <yum.rayan@gmail.com>
> 
> --- kernel/module.c.orig	2005-03-28 22:32:35.000000000 -0800
> +++ kernel/module.c	2005-03-28 22:49:26.000000000 -0800
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.

> @@ -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().

> -	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
> -

-- 
~Randy

Re: [PATCH] Reduce stack usage in module.c

[Yum Rayan]

On Tue, 29 Mar 2005 09:43:12 -0800, Randy.Dunlap <rddunlap@osdl.org> wrote:
> Yum Rayan wrote:
> > 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
> So all function local variables are in registers?

Yes, all function local variables of the patched who_is_doing_it(...)
are in registers.

> > 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?

> 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.

> 
> > @@ -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().

Was attempting to preserve the call flow of the previous author. But
yes, this makes more sense. I changed code to return "0" for !num
case.

Thanks,
Rayan

Summary: Reduce stack usage in obsolete_params() and who_is_doing_it()
Target: linux-2.6.12-rc1-mm3
Signed-off-by: Yum Rayan <yum.rayan@gmail.com>

--- a/kernel/module.c	2005-03-25 22:11:06.000000000 -0800
+++ b/kernel/module.c	2005-03-29 22:16:09.000000000 -0800
@@ -767,17 +767,27 @@
 			   const char *strtab)
 {
 	struct kernel_param *kp;
-	unsigned int i;
+	char *sym_name;
+	unsigned int sym_name_len, i;
 	int ret;
 
+	if (!num)
+		return 0;
+
 	kp = kmalloc(sizeof(kp[0]) * num, GFP_KERNEL);
 	if (!kp)
 		return -ENOMEM;
 
-	for (i = 0; i < num; i++) {
-		char sym_name[128 + sizeof(MODULE_SYMBOL_PREFIX)];
+	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

Re: [PATCH] Reduce stack usage in module.c

[Randy.Dunlap]

Yum Rayan wrote:
> On Tue, 29 Mar 2005 09:43:12 -0800, Randy.Dunlap <rddunlap@osdl.org> wrote:
> 
>>Yum Rayan wrote:
>>
>>>Attempt to reduce stack usage in module.c (linux-2.6.12-rc1-mm3).
>>>Specifically from checkstack.pl
> 
>>>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?

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

>>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.

Good idea, thanks.

-- 
~Randy

Re: [PATCH] Reduce stack usage in module.c

[Jesper Juhl]

On Wed, 30 Mar 2005, Randy.Dunlap wrote:

> Yum Rayan wrote:
> > On Tue, 29 Mar 2005 09:43:12 -0800, Randy.Dunlap <rddunlap@osdl.org> wrote:
> > 
> > > Yum Rayan wrote:
> > > 
> > > > - 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 }
> > 
> > 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).  :)
> 
Whoops, that boundary error is mine, sorry about that.

-- 
Jesper Juhl