Fix memory leak in swsusp

Fix memory leak in swsusp

[Pavel Machek]

Hi!

This fixes 2 memory leaks in swsusp: during relocating pagedir, eaten
pages were not properly freed in error path and even regular freeing
path was freeing one page too little. Please apply,

								Pavel

--- linux-cvs/kernel/power/swsusp.c	2004-05-22 19:39:01.000000000 +0200
+++ linux/kernel/power/swsusp.c	2004-06-06 00:30:09.000000000 +0200
@@ -455,7 +455,7 @@
@@ -503,6 +503,9 @@
 		if (!pbe)
 			continue;
 		pbe->orig_address = (long) page_address(page);
+		/* Copy page is dangerous: it likes to mess with
+		   preempt count on specific cpus. Wrong preempt count is then copied,
+		   oops. */
 		copy_page((void *)pbe->address, (void *)pbe->orig_address);
 		pbe++;
 	}
@@ -923,8 +952,9 @@
 	suspend_pagedir_t *new_pagedir, *old_pagedir = pagedir_nosave;
 	void **eaten_memory = NULL;
 	void **c = eaten_memory, *m, *f;
+	int ret = 0;
 
-	printk("Relocating pagedir");
+	printk("Relocating pagedir ");
 
 	if(!does_collide_order(old_pagedir, (unsigned long)old_pagedir, pagedir_order)) {
 		printk("not necessary\n");
@@ -941,22 +971,23 @@
 		c = eaten_memory;
 	}
 
-	if (!m)
-		return -ENOMEM;
-
-	pagedir_nosave = new_pagedir = m;
-	copy_pagedir(new_pagedir, old_pagedir);
+	if (!m) {
+		printk("out of memory\n");
+		ret = -ENOMEM;
+	} else {
+		pagedir_nosave = new_pagedir = m;
+		copy_pagedir(new_pagedir, old_pagedir);
+	}
 
 	c = eaten_memory;
-	while(c) {
+	while (c) {
 		printk(":");
-		f = *c;
+		f = c;
 		c = *c;
-		if (f)
-			free_pages((unsigned long)f, pagedir_order);
+		free_pages((unsigned long)f, pagedir_order);
 	}
 	printk("|\n");
-	return 0;
+	return ret;
 }
 
 /*

-- 
934a471f20d6580d5aad759bf0d97ddc

Re: Fix memory leak in swsusp

[Herbert Xu]

Pavel Machek <pavel@ucw.cz> wrote:
> 
> This fixes 2 memory leaks in swsusp: during relocating pagedir, eaten
> pages were not properly freed in error path and even regular freeing
> path was freeing one page too little. Please apply,

Thanks for the patch.

Here is the same fix for pmdisk.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email:  Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
===== kernel/power/pmdisk.c 1.84 vs edited =====
--- 1.84/kernel/power/pmdisk.c	2004-04-13 03:55:18 +10:00
+++ edited/kernel/power/pmdisk.c	2004-06-10 20:47:15 +10:00
@@ -795,6 +795,7 @@
 	suspend_pagedir_t *new_pagedir, *old_pagedir = pm_pagedir_nosave;
 	void **eaten_memory = NULL;
 	void **c = eaten_memory, *m, *f;
+	int err;
 
 	pr_debug("pmdisk: Relocating pagedir\n");
 
@@ -803,32 +804,31 @@
 		return 0;
 	}
 
+	err = -ENOMEM;
 	while ((m = (void *) __get_free_pages(GFP_ATOMIC, pagedir_order))) {
 		memset(m, 0, PAGE_SIZE);
-		if (!does_collide_order(old_pagedir, (unsigned long)m, pagedir_order))
+		if (!does_collide_order(old_pagedir, (unsigned long)m,
+					pagedir_order)) {
+			pm_pagedir_nosave = new_pagedir = m;
+			copy_pagedir(new_pagedir, old_pagedir);
+			err = 0;
 			break;
+		}
 		eaten_memory = m;
 		printk( "." ); 
 		*eaten_memory = c;
 		c = eaten_memory;
 	}
 
-	if (!m)
-		return -ENOMEM;
-
-	pm_pagedir_nosave = new_pagedir = m;
-	copy_pagedir(new_pagedir, old_pagedir);
-
 	c = eaten_memory;
 	while(c) {
 		printk(":");
-		f = *c;
+		f = c;
 		c = *c;
-		if (f)
-			free_pages((unsigned long)f, pagedir_order);
+		free_pages((unsigned long)f, pagedir_order);
 	}
 	printk("|\n");
-	return 0;
+	return err;
 }
 
 

Re: Fix memory leak in swsusp

[Herbert Xu]

On Thu, Jun 10, 2004 at 08:50:12PM +1000, Herbert Xu wrote:
>
> @@ -803,32 +804,31 @@
>  		return 0;
>  	}
>  
> +	err = -ENOMEM;
>  	while ((m = (void *) __get_free_pages(GFP_ATOMIC, pagedir_order))) {
>  		memset(m, 0, PAGE_SIZE);

BTW, what does this memset do?

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email:  Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Fix memory leak in swsusp

[Pavel Machek]

Hi!

> > @@ -803,32 +804,31 @@
> >  		return 0;
> >  	}
> >  
> > +	err = -ENOMEM;
> >  	while ((m = (void *) __get_free_pages(GFP_ATOMIC, pagedir_order))) {
> >  		memset(m, 0, PAGE_SIZE);
> 
> BTW, what does this memset do?

Someone (me?) was trying to be carefull, and was not carefull enough.
AFAICS it should be memset(..., PAGE_SIZE << pagedir_order)
-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         

Re: Fix memory leak in swsusp

[Pavel Machek]

Hi!

> > @@ -803,32 +804,31 @@
> >  		return 0;
> >  	}
> >  
> > +	err = -ENOMEM;
> >  	while ((m = (void *) __get_free_pages(GFP_ATOMIC, pagedir_order))) {
> >  		memset(m, 0, PAGE_SIZE);
> 
> BTW, what does this memset do?

Here's incremental fix, it compiles but not tested.

BTW I have problems getting mail to you:

This is the Postfix program at host atrey.karlin.mff.cuni.cz.

I'm sorry to have to inform you that the message returned
below could not be delivered to one or more destinations.

For further assistance, please send mail to <postmaster>

If you do so, please include this problem report. You can
delete your own text from the message returned below.

                        The Postfix program

<herbert@gondor.apana.org.au>: host arnor.apana.org.au[203.14.152.115]said:
    550 mail from 195.113.31.123 rejected: administrative prohibition

									Pavel

--- tmp/linux/kernel/power/swsusp.c	2004-06-10 23:16:05.000000000 +0200
+++ linux/kernel/power/swsusp.c	2004-06-10 23:09:07.000000000 +0200
@@ -962,7 +962,7 @@
 	}
 
 	while ((m = (void *) __get_free_pages(GFP_ATOMIC, pagedir_order))) {
-		memset(m, 0, PAGE_SIZE);
+		memset(m, 0, PAGE_SIZE << pagedir_order);
 		if (!does_collide_order(old_pagedir, (unsigned long)m, pagedir_order))
 			break;
 		eaten_memory = m;

-- 
People were complaining that M$ turns users into beta-testers...

Re: Fix memory leak in swsusp

[Herbert Xu]

On Thu, Jun 10, 2004 at 11:24:48PM +0200, Pavel Machek wrote:
> > > @@ -803,32 +804,31 @@
> > >  		return 0;
> > >  	}
> > >  
> > > +	err = -ENOMEM;
> > >  	while ((m = (void *) __get_free_pages(GFP_ATOMIC, pagedir_order))) {
> > >  		memset(m, 0, PAGE_SIZE);
> > 
> > BTW, what does this memset do?
> 
> Here's incremental fix, it compiles but not tested.

Thanks, but my question remains: why do we need the memset?  The
area allocated is either thrown away because it collides or is
overwritten with the pagedir stuff straight away.

> BTW I have problems getting mail to you:

Sorry, that should be fixed now.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email:  Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Fix memory leak in swsusp

[Pavel Machek]

Hi!

> > > > @@ -803,32 +804,31 @@
> > > >  		return 0;
> > > >  	}
> > > >  
> > > > +	err = -ENOMEM;
> > > >  	while ((m = (void *) __get_free_pages(GFP_ATOMIC, pagedir_order))) {
> > > >  		memset(m, 0, PAGE_SIZE);
> > > 
> > > BTW, what does this memset do?
> > 
> > Here's incremental fix, it compiles but not tested.
> 
> Thanks, but my question remains: why do we need the memset?  The
> area allocated is either thrown away because it collides or is
> overwritten with the pagedir stuff straight away.

You are right, it should not be needed.
								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: Fix memory leak in swsusp

[Herbert Xu]

[-- Attachment #1: Type: text/plain, Size: 656 bytes --]

On Fri, Jun 11, 2004 at 11:48:44AM +0200, Pavel Machek wrote:
>
> > Thanks, but my question remains: why do we need the memset?  The
> > area allocated is either thrown away because it collides or is
> > overwritten with the pagedir stuff straight away.
> 
> You are right, it should not be needed.

Great.  Here's the patch that removes the memset calls from both
pmdisk and swsusp.  It depends on the previous patches in this
thread.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email:  Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: p --]
[-- Type: text/plain, Size: 809 bytes --]

diff -Nru a/kernel/power/pmdisk.c b/kernel/power/pmdisk.c
--- a/kernel/power/pmdisk.c	2004-06-11 20:14:54 +10:00
+++ b/kernel/power/pmdisk.c	2004-06-11 20:14:54 +10:00
@@ -806,7 +806,6 @@
 
 	err = -ENOMEM;
 	while ((m = (void *) __get_free_pages(GFP_ATOMIC, pagedir_order))) {
-		memset(m, 0, PAGE_SIZE);
 		if (!does_collide_order(old_pagedir, (unsigned long)m,
 					pagedir_order)) {
 			pm_pagedir_nosave = new_pagedir = m;
--- linux-2.5/kernel/power/swsusp.c.orig	2004-06-11 20:14:02.000000000 +1000
+++ linux-2.5/kernel/power/swsusp.c	2004-06-11 20:14:07.000000000 +1000
@@ -936,7 +936,6 @@
 	}
 
 	while ((m = (void *) __get_free_pages(GFP_ATOMIC, pagedir_order))) {
-		memset(m, 0, PAGE_SIZE);
 		if (!does_collide_order(old_pagedir, (unsigned long)m, pagedir_order))
 			break;
 		eaten_memory = m;

Re: Fix memory leak in swsusp

[Pavel Machek]

Hi!

> > > Thanks, but my question remains: why do we need the memset?  The
> > > area allocated is either thrown away because it collides or is
> > > overwritten with the pagedir stuff straight away.
> > 
> > You are right, it should not be needed.
> 
> Great.  Here's the patch that removes the memset calls from both
> pmdisk and swsusp.  It depends on the previous patches in this
> thread.

If you want more cleanups, copy_pagedir() should be probably replaced
by simple memset...
								Pavel

-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: Fix memory leak in swsusp

[Herbert Xu]

[-- Attachment #1: Type: text/plain, Size: 497 bytes --]

On Fri, Jun 11, 2004 at 12:23:27PM +0200, Pavel Machek wrote:
> 
> If you want more cleanups, copy_pagedir() should be probably replaced
> by simple memset...

Yes that's a great idea.  Here is the patch to do that.  Again it
relies on all the previous patches in this thread.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email:  Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: p --]
[-- Type: text/plain, Size: 2609 bytes --]

===== kernel/power/pmdisk.c 1.86 vs edited =====
--- 1.86/kernel/power/pmdisk.c	2004-06-11 18:23:44 +10:00
+++ edited/kernel/power/pmdisk.c	2004-06-11 21:01:41 +10:00
@@ -732,19 +732,6 @@
 
 /* More restore stuff */
 
-/* FIXME: Why not memcpy(to, from, 1<<pagedir_order*PAGE_SIZE)? */
-static void __init copy_pagedir(suspend_pagedir_t *to, suspend_pagedir_t *from)
-{
-	int i;
-	char *topointer=(char *)to, *frompointer=(char *)from;
-
-	for(i=0; i < 1 << pagedir_order; i++) {
-		copy_page(topointer, frompointer);
-		topointer += PAGE_SIZE;
-		frompointer += PAGE_SIZE;
-	}
-}
-
 #define does_collide(addr) does_collide_order(pm_pagedir_nosave, addr, 0)
 
 /*
@@ -792,7 +779,7 @@
 	 * We have to avoid recursion (not to overflow kernel stack),
 	 * and that's why code looks pretty cryptic 
 	 */
-	suspend_pagedir_t *new_pagedir, *old_pagedir = pm_pagedir_nosave;
+	suspend_pagedir_t *old_pagedir = pm_pagedir_nosave;
 	void **eaten_memory = NULL;
 	void **c = eaten_memory, *m, *f;
 	int err;
@@ -808,8 +795,9 @@
 	while ((m = (void *) __get_free_pages(GFP_ATOMIC, pagedir_order))) {
 		if (!does_collide_order(old_pagedir, (unsigned long)m,
 					pagedir_order)) {
-			pm_pagedir_nosave = new_pagedir = m;
-			copy_pagedir(new_pagedir, old_pagedir);
+			pm_pagedir_nosave =
+				memcpy(m, old_pagedir,
+				       PAGE_SIZE << pagedir_order);
 			err = 0;
 			break;
 		}
--- linux-2.5/kernel/power/swsusp.c.orig	2004-06-11 20:54:24.000000000 +1000
+++ linux-2.5/kernel/power/swsusp.c	2004-06-11 21:02:22.000000000 +1000
@@ -863,19 +863,6 @@
 
 /* More restore stuff */
 
-/* FIXME: Why not memcpy(to, from, 1<<pagedir_order*PAGE_SIZE)? */
-static void copy_pagedir(suspend_pagedir_t *to, suspend_pagedir_t *from)
-{
-	int i;
-	char *topointer=(char *)to, *frompointer=(char *)from;
-
-	for(i=0; i < 1 << pagedir_order; i++) {
-		copy_page(topointer, frompointer);
-		topointer += PAGE_SIZE;
-		frompointer += PAGE_SIZE;
-	}
-}
-
 #define does_collide(addr) does_collide_order(pagedir_nosave, addr, 0)
 
 /*
@@ -923,7 +910,7 @@
 	 * We have to avoid recursion (not to overflow kernel stack),
 	 * and that's why code looks pretty cryptic 
 	 */
-	suspend_pagedir_t *new_pagedir, *old_pagedir = pagedir_nosave;
+	suspend_pagedir_t *old_pagedir = pagedir_nosave;
 	void **eaten_memory = NULL;
 	void **c = eaten_memory, *m, *f;
 	int ret = 0;
@@ -948,8 +935,8 @@
 		printk("out of memory\n");
 		ret = -ENOMEM;
 	} else {
-		pagedir_nosave = new_pagedir = m;
-		copy_pagedir(new_pagedir, old_pagedir);
+		pagedir_nosave =
+			memcpy(m, old_pagedir, PAGE_SIZE << pagedir_order);
 	}
 
 	c = eaten_memory;

Re: Fix memory leak in swsusp

[Nigel Cunningham]

Hi.

Herbert Xu wrote:

[...]

> +			pm_pagedir_nosave =
> +				memcpy(m, old_pagedir,
> +				       PAGE_SIZE << pagedir_order);

[...]

> +		pagedir_nosave =
> +			memcpy(m, old_pagedir, PAGE_SIZE << pagedir_order);
>  	}
>  
>  	c = eaten_memory;

We were avoiding the use of memcpy because it messes up the preempt count with 3DNow, and 
potentially as other unseen side effects. The preempt could possibly simply be reset at resume time, 
but the point remains.

Regards,

Nigel
-- 
Nigel & Michelle Cunningham
C/- Westminster Presbyterian Church Belconnen
61 Templeton Street, Cook, ACT 2614.
+61 (417) 100 574 (mobile)

Re: Fix memory leak in swsusp

[Andrew Morton]

Nigel Cunningham <ncunningham@linuxmail.org> wrote:
>
>  We were avoiding the use of memcpy because it messes up the preempt count with 3DNow, and 
>  potentially as other unseen side effects. The preempt could possibly simply be reset at resume time, 
>  but the point remains.

eh?  memcpy just copies memory.  Maybe your meant copy_*_user()?

Re: Fix memory leak in swsusp

[Dave Jones]

On Fri, Jun 11, 2004 at 09:00:59PM -0700, Andrew Morton wrote:
 > Nigel Cunningham <ncunningham@linuxmail.org> wrote:
 > >
 > >  We were avoiding the use of memcpy because it messes up the preempt count with 3DNow, and 
 > >  potentially as other unseen side effects. The preempt could possibly simply be reset at resume time, 
 > >  but the point remains.
 > 
 > eh?  memcpy just copies memory.  Maybe your meant copy_*_user()?

See arch/i386/lib/memcpy.c  The 3dnow routine does kernel_fpu_begin()/..end()
which futzes with preempt count.  What I'm missing though is that the count
afterwards should be the same as it was before the memcpy. Why is this
a problem for the suspend folks?

		Dave

Re: Fix memory leak in swsusp

[Pavel Machek]

Hi!

>  > >  We were avoiding the use of memcpy because it messes up the preempt count with 3DNow, and 
>  > >  potentially as other unseen side effects. The preempt could possibly simply be reset at resume time, 
>  > >  but the point remains.
>  > 
>  > eh?  memcpy just copies memory.  Maybe your meant copy_*_user()?
> 
> See arch/i386/lib/memcpy.c  The 3dnow routine does kernel_fpu_begin()/..end()
> which futzes with preempt count.  What I'm missing though is that the count
> afterwards should be the same as it was before the memcpy. Why is this
> a problem for the suspend folks?

It does not hurt here, but as someone else explained already it is a
problem when doing atomic copy of memory. (You increment, than copy
whole memory, you decrement; but you only decrement in original, not
in copy...)
									Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: Fix memory leak in swsusp

[Nigel Cunningham]

Hi.

Andrew Morton wrote:
> Nigel Cunningham <ncunningham@linuxmail.org> wrote:
> 
>> We were avoiding the use of memcpy because it messes up the preempt count with 3DNow, and 
>> potentially as other unseen side effects. The preempt could possibly simply be reset at resume time, 
>> but the point remains.
> 
> 
> eh?  memcpy just copies memory.  Maybe your meant copy_*_user()?

At some stage, you copy the page that contains the preempt count for the process that is doing the 
suspending. If you use memcpy on a 3Dnow machine, the preempt count is incremented prior to doing 
the copy of the page. Then, at resume time, it is one too high.

Regards,

Nigel
-- 
Nigel & Michelle Cunningham
C/- Westminster Presbyterian Church Belconnen
61 Templeton Street, Cook, ACT 2614.
+61 (417) 100 574 (mobile)

Re: Fix memory leak in swsusp

[Jeff Sipek]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Saturday 12 June 2004 18:07, Nigel Cunningham wrote:
> Hi.
> At some stage, you copy the page that contains the preempt count for the
> process that is doing the suspending. If you use memcpy on a 3Dnow machine,
> the preempt count is incremented prior to doing the copy of the page. Then,
> at resume time, it is one too high.

Well, if we know that it is one too high, why not decrement right after the 
resume?

Jeff.

- -- 
"Reality is merely an illusion, albeit a very persistent one."
		- Albert Einstein
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (GNU/Linux)

iD8DBQFAy5KfwFP0+seVj/4RAlTXAJsFHALWNJ+mgVu7xEhfsk2Hqcq/wwCgmEmh
EpOv1mA9B35xcbzxT+lSHIo=
=Bn1K
-----END PGP SIGNATURE-----

Re: Fix memory leak in swsusp

[Pavel Machek]

Hi!

> > At some stage, you copy the page that contains the preempt count for the
> > process that is doing the suspending. If you use memcpy on a 3Dnow machine,
> > the preempt count is incremented prior to doing the copy of the page. Then,
> > at resume time, it is one too high.
> 
> Well, if we know that it is one too high, why not decrement right after the 
> resume?

You are suggesting very dirty hack. See archives why it is basically
undoable.

									Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: Fix memory leak in swsusp

[Herbert Xu]

Nigel Cunningham <ncunningham@linuxmail.org> wrote:
> 
> At some stage, you copy the page that contains the preempt count for the process that is doing the 
> suspending. If you use memcpy on a 3Dnow machine, the preempt count is incremented prior to doing 
> the copy of the page. Then, at resume time, it is one too high.

Nope, this function only copies the swsusp page data structure so
this is irrelevant.
-- 
Visit Openswan at http://www.openswan.org/
Email:  Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Fix memory leak in swsusp

[Herbert Xu]

On Sat, Jun 12, 2004 at 01:17:30PM +1000, Nigel Cunningham wrote:
> 
> We were avoiding the use of memcpy because it messes up the preempt count 
> with 3DNow, and potentially as other unseen side effects. The preempt could 
> possibly simply be reset at resume time, but the point remains.

In what way does it mess up the preempt count?
-- 
Visit Openswan at http://www.openswan.org/
Email:  Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

RE:Re: Fix memory leak in swsusp

[pavel]

At this point it is okay to memcpy - it is copying pagedir, at that point we are outside any critical session. --p

------
Written-on-t68i. Sorry.

Re: Fix memory leak in swsusp

[Nigel Cunningham]

pavel@ucw.cz wrote:
> At this point it is okay to memcpy - it is copying pagedir, at that point we are outside any critical session. --p

Ah. So you're not doing the atomic copy in this routine? Humble apologies.

Nigel

-- 
Nigel & Michelle Cunningham
C/- Westminster Presbyterian Church Belconnen
61 Templeton Street, Cook, ACT 2614.
+61 (417) 100 574 (mobile)