Re: [PATCH] bio_uncopy_user mem leak

Re: [PATCH] bio_uncopy_user mem leak

[Greg Afinogenov]

I'd just like to point out that this patch does not, as may be expected,
result in functional audio CDs.  It merely results in a successful burn
process and a CD full of noise.

Perhaps this should be tested/fixed?

Greg

(I'm not subscribed to this list; please CC me any responses; thanks!)

 

Re: [PATCH] bio_uncopy_user mem leak

[Con Kolivas]

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

Greg Afinogenov wrote:
> I'd just like to point out that this patch does not, as may be expected,
> result in functional audio CDs.  It merely results in a successful burn
> process and a CD full of noise.
> 
> Perhaps this should be tested/fixed?

Ok I just tested this patch discretely and indeed the memory leak goes 
away but it still produces coasters so something is still amuck. Just as 
a data point; burning DVDs and data cds is ok. Burning audio *and 
videocds* is not.

Con

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 256 bytes --]

Re: [PATCH] bio_uncopy_user mem leak

[Chris Mason]

On Thu, 2004-08-19 at 07:07, Con Kolivas wrote:
> Greg Afinogenov wrote:
> > I'd just like to point out that this patch does not, as may be expected,
> > result in functional audio CDs.  It merely results in a successful burn
> > process and a CD full of noise.
> > 
> > Perhaps this should be tested/fixed?
> 
> Ok I just tested this patch discretely and indeed the memory leak goes 
> away but it still produces coasters so something is still amuck. Just as 
> a data point; burning DVDs and data cds is ok. Burning audio *and 
> videocds* is not.

It might be the cold medicine talking, but I think we need something
like this.  gcc tested it for me, beyond that I make no promises....

--- l/fs/bio.c.1	2004-08-19 09:36:13.596858736 -0400
+++ l/fs/bio.c	2004-08-19 09:47:46.392537784 -0400
@@ -454,6 +454,7 @@
 	 */
 	if (!ret) {
 		if (!write_to_vm) {
+			unsigned long p = uaddr;
 			bio->bi_rw |= (1 << BIO_RW);
 			/*
 	 		 * for a write, copy in data to kernel pages
@@ -462,8 +463,9 @@
 			bio_for_each_segment(bvec, bio, i) {
 				char *addr = page_address(bvec->bv_page);
 
-				if (copy_from_user(addr, (char *) uaddr, bvec->bv_len))
+				if (copy_from_user(addr, (char *) p, bvec->bv_len))
 					goto cleanup;
+				p += bvec->bv_len;
 			}
 		}
 

Re: [PATCH] bio_uncopy_user mem leak

[Kurt Garloff]

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

Hi Chris,

On Thu, Aug 19, 2004 at 09:51:34AM -0400, Chris Mason wrote:
> On Thu, 2004-08-19 at 07:07, Con Kolivas wrote:
> > Ok I just tested this patch discretely and indeed the memory leak goes 
> > away but it still produces coasters so something is still amuck. Just as 
> > a data point; burning DVDs and data cds is ok. Burning audio *and 
> > videocds* is not.
> 
> It might be the cold medicine talking, but I think we need something
> like this.  gcc tested it for me, beyond that I make no promises....
> 
> --- l/fs/bio.c.1	2004-08-19 09:36:13.596858736 -0400
> +++ l/fs/bio.c	2004-08-19 09:47:46.392537784 -0400
> @@ -454,6 +454,7 @@
>  	 */
>  	if (!ret) {
>  		if (!write_to_vm) {
> +			unsigned long p = uaddr;
>  			bio->bi_rw |= (1 << BIO_RW);
>  			/*
>  	 		 * for a write, copy in data to kernel pages
> @@ -462,8 +463,9 @@
>  			bio_for_each_segment(bvec, bio, i) {
>  				char *addr = page_address(bvec->bv_page);
>  
> -				if (copy_from_user(addr, (char *) uaddr, bvec->bv_len))
> +				if (copy_from_user(addr, (char *) p, bvec->bv_len))
>  					goto cleanup;
> +				p += bvec->bv_len;
>  			}
>  		}
>  

Hmm, that patch would make a lot of sense to me.

It matches the problem description; burning data CDs, we don't
use bounce buffers, so that does not use this code path. Here,
it looks like we copied the same userspace page again and again
into a multisegment BIO. Ouch!

Not yet tested either :-(

Regards,
-- 
Kurt Garloff, Director SUSE Labs, Novell Inc.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] bio_uncopy_user mem leak

[Con Kolivas]

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

Kurt Garloff wrote:
> Hi Chris,
> 
> On Thu, Aug 19, 2004 at 09:51:34AM -0400, Chris Mason wrote:
> 
>>On Thu, 2004-08-19 at 07:07, Con Kolivas wrote:
>>
>>>Ok I just tested this patch discretely and indeed the memory leak goes 
>>>away but it still produces coasters so something is still amuck. Just as 
>>>a data point; burning DVDs and data cds is ok. Burning audio *and 
>>>videocds* is not.
>>
>>It might be the cold medicine talking, but I think we need something
>>like this.  gcc tested it for me, beyond that I make no promises....
>>
>>--- l/fs/bio.c.1	2004-08-19 09:36:13.596858736 -0400
>>+++ l/fs/bio.c	2004-08-19 09:47:46.392537784 -0400
>>@@ -454,6 +454,7 @@
>> 	 */
>> 	if (!ret) {
>> 		if (!write_to_vm) {
>>+			unsigned long p = uaddr;
>> 			bio->bi_rw |= (1 << BIO_RW);
>> 			/*
>> 	 		 * for a write, copy in data to kernel pages
>>@@ -462,8 +463,9 @@
>> 			bio_for_each_segment(bvec, bio, i) {
>> 				char *addr = page_address(bvec->bv_page);
>> 
>>-				if (copy_from_user(addr, (char *) uaddr, bvec->bv_len))
>>+				if (copy_from_user(addr, (char *) p, bvec->bv_len))
>> 					goto cleanup;
>>+				p += bvec->bv_len;
>> 			}
>> 		}
>> 
> 
> 
> Hmm, that patch would make a lot of sense to me.
> 
> It matches the problem description; burning data CDs, we don't
> use bounce buffers, so that does not use this code path. Here,
> it looks like we copied the same userspace page again and again
> into a multisegment BIO. Ouch!
> 
> Not yet tested either :-(

Ok looks like your cold medicine is working well for you ;-). This patch 
on top of the other patch has the memory freeing _and_ burns good cds. 
Well done. I only tested with a video cd. Can someone confirm audio cd 
(although it seems obvious it would help both).

Andrew did you threaten to make a 2.6.8.2 since 2.6.8{,.1} cannot safely 
burn an audio cd?

Cheers,
Con

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 256 bytes --]

Re: [PATCH] bio_uncopy_user mem leak

[Andrew Morton]

Con Kolivas <kernel@kolivas.org> wrote:
>
>  Andrew did you threaten to make a 2.6.8.2 since 2.6.8{,.1} cannot safely 
>  burn an audio cd?

Uh, I guess that depends on how rested Linus feels when he returns.  I
think there's a fairly significant networking fix too.  As I said: we'll see.

Re: [PATCH] bio_uncopy_user mem leak

[Kurt Garloff]

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

Hi Andrew,

On Thu, Aug 19, 2004 at 11:31:55PM -0700, Andrew Morton wrote:
> Con Kolivas <kernel@kolivas.org> wrote:
> >
> >  Andrew did you threaten to make a 2.6.8.2 since 2.6.8{,.1} cannot safely 
> >  burn an audio cd?
> 
> Uh, I guess that depends on how rested Linus feels when he returns.  I
> think there's a fairly significant networking fix too.  As I said: we'll see.

I must have overlooked the network thing. But in internal testing we do 
see problems with 2.6.8, which seem to be related to networking and/or 
NFS.

Care to work out?

Regards,
-- 
Kurt Garloff, Director SUSE Labs, Novell Inc.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] bio_uncopy_user mem leak

[sandr8]

Andrew Morton wrote:

>Con Kolivas <kernel@kolivas.org> wrote:
>
>Uh, I guess that depends on how rested Linus feels when he returns.  I
>think there's a fairly significant networking fix too.  As I said: we'll see
>
is that the one related to the qdisc private data alignment? if not, 
that one would be a urgent one too imho...
i got many oops from the ext3 commit and this because all of the 
schedulers in the 2.6.8 and 2.6.8.1 traffic control suite are accessing 
data at slightly wrong positions.
this because if any discipline different from pfifo fast is used, the 
kmalloc()ed memory is aligned in the old way, but then it is used in the 
new one.
the results could be catastrophic whenever the accessed data is a 
pointer and the memory referenced is written to. :-\

Alessandro Salvatori

Re: [PATCH] bio_uncopy_user mem leak

[Greg Afinogenov]

On Thu, 2004-08-19 at 22:19, Con Kolivas wrote:
> Kurt Garloff wrote:
> > Hi Chris,
> > 
> > On Thu, Aug 19, 2004 at 09:51:34AM -0400, Chris Mason wrote:
> > 
> >>On Thu, 2004-08-19 at 07:07, Con Kolivas wrote:
> >>
> >>>Ok I just tested this patch discretely and indeed the memory leak goes 
> >>>away but it still produces coasters so something is still amuck. Just as 
> >>>a data point; burning DVDs and data cds is ok. Burning audio *and 
> >>>videocds* is not.
> >>
> >>It might be the cold medicine talking, but I think we need something
> >>like this.  gcc tested it for me, beyond that I make no promises....
> >>
> >>--- l/fs/bio.c.1	2004-08-19 09:36:13.596858736 -0400
> >>+++ l/fs/bio.c	2004-08-19 09:47:46.392537784 -0400
> >>@@ -454,6 +454,7 @@
> >> 	 */
> >> 	if (!ret) {
> >> 		if (!write_to_vm) {
> >>+			unsigned long p = uaddr;
> >> 			bio->bi_rw |= (1 << BIO_RW);
> >> 			/*
> >> 	 		 * for a write, copy in data to kernel pages
> >>@@ -462,8 +463,9 @@
> >> 			bio_for_each_segment(bvec, bio, i) {
> >> 				char *addr = page_address(bvec->bv_page);
> >> 
> >>-				if (copy_from_user(addr, (char *) uaddr, bvec->bv_len))
> >>+				if (copy_from_user(addr, (char *) p, bvec->bv_len))
> >> 					goto cleanup;
> >>+				p += bvec->bv_len;
> >> 			}
> >> 		}
> >> 
> > 
> > 
> > Hmm, that patch would make a lot of sense to me.
> > 
> > It matches the problem description; burning data CDs, we don't
> > use bounce buffers, so that does not use this code path. Here,
> > it looks like we copied the same userspace page again and again
> > into a multisegment BIO. Ouch!
> > 
> > Not yet tested either :-(
> 
> Ok looks like your cold medicine is working well for you ;-). This patch 
> on top of the other patch has the memory freeing _and_ burns good cds. 
> Well done. I only tested with a video cd. Can someone confirm audio cd 
> (although it seems obvious it would help both).
> 
> Andrew did you threaten to make a 2.6.8.2 since 2.6.8{,.1} cannot safely 
> burn an audio cd?
> 
> Cheers,
> Con

Yup, it works fine with audio CDs too.  Great!

Re: [PATCH] bio_uncopy_user mem leak

[Kurt Garloff]

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

Hi,

On Fri, Aug 20, 2004 at 01:19:37PM +1000, Con Kolivas wrote:
> Kurt Garloff wrote:
> >Not yet tested either :-(
> 
> Ok looks like your cold medicine is working well for you ;-). This patch 
> on top of the other patch has the memory freeing _and_ burns good cds. 
> Well done. I only tested with a video cd. Can someone confirm audio cd 
> (although it seems obvious it would help both).

Tested here as well now. Could reproduce the noise CD before and have
nice music again, after applying Chris' fix as well. 
Found a CD-RW to test ;-)

Thx,
-- 
Kurt Garloff, Director SUSE Labs, Novell Inc.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH] bio_uncopy_user mem leak

[Kurt Garloff]

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

Hi Andrew,

When using bounce buffers for SG_IO commands with unaligned 
buffers in blk_rq_map_user(), we should free the pages from
blk_rq_unmap_user() which calls bio_uncopy_user() for the 
non-BIO_USER_MAPPED case. That function failed to free the
pages for write requests.

So we leaked pages and you machine would go OOM. Rebooting 
helped ;-)

This bug was triggered by writing audio CDs (but not on data 
CDs), as the audio frames are not aligned well (2352 bytes),
so the user pages don't just get mapped.

Bug was reported by Mathias Homan and debugged by Chris Mason + me.
(Jens is away.)

Signed-off-by: Kurt Garloff <garloff@suse.de>

 bio.c |   21 +++++++++------------
 1 files changed, 9 insertions(+), 12 deletions(-)

--- linux-2.6.8.x86/fs/bio.c.orig	2004-08-14 07:37:15.000000000 +0200
+++ linux-2.6.8.x86/fs/bio.c	2004-08-17 17:41:52.022012902 +0200
@@ -388,20 +388,17 @@ int bio_uncopy_user(struct bio *bio)
 	struct bio_vec *bvec;
 	int i, ret = 0;
 
-	if (bio_data_dir(bio) == READ) {
-		char *uaddr = bio->bi_private;
-
-		__bio_for_each_segment(bvec, bio, i, 0) {
-			char *addr = page_address(bvec->bv_page);
-
-			if (!ret && copy_to_user(uaddr, addr, bvec->bv_len))
-				ret = -EFAULT;
+	char *uaddr = bio->bi_private;
+	
+	__bio_for_each_segment(bvec, bio, i, 0) {
+		char *addr = page_address(bvec->bv_page);
+		if (bio_data_dir(bio) == READ && !ret && 
+		    copy_to_user(uaddr, addr, bvec->bv_len))
+			ret = -EFAULT;
 
-			__free_page(bvec->bv_page);
-			uaddr += bvec->bv_len;
-		}
+		__free_page(bvec->bv_page);
+		uaddr += bvec->bv_len;
 	}
-
 	bio_put(bio);
 	return ret;
 }

-- 
Kurt Garloff, Director SUSE Labs, Novell

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] bio_uncopy_user mem leak

[Gene Heskett]

On Tuesday 17 August 2004 11:59, Kurt Garloff wrote:

Posted a patch for a memory leak.  I have no idea whats got into me, 
but I just applied this one to see if it has anything to do with my 
"possible dcache problem".

FWIW, I had to do it by hand even though it looked dead on for 
2.6.8-rc4/fs/bio.c

No errors when recompiling.

-- 
Cheers, Gene
"There are four boxes to be used in defense of liberty:
 soap, ballot, jury, and ammo. Please use in that order."
-Ed Howdershelt (Author)
99.24% setiathome rank, not too shabby for a WV hillbilly
Yahoo.com attorneys please note, additions to this message
by Gene Heskett are:
Copyright 2004 by Maurice Eugene Heskett, all rights reserved.

Re: [PATCH] bio_uncopy_user mem leak

[Andrew Morton]

Kurt Garloff <garloff@suse.de> wrote:
>
> When using bounce buffers for SG_IO commands with unaligned 
> buffers in blk_rq_map_user(), we should free the pages from
> blk_rq_unmap_user() which calls bio_uncopy_user() for the 
> non-BIO_USER_MAPPED case. That function failed to free the
> pages for write requests.
> 
> So we leaked pages and you machine would go OOM. Rebooting 
> helped ;-)

Eureka.  Thanks.

This really should trigger a 2.6.8.2.  We'll see.

Re: [PATCH] bio_uncopy_user mem leak

[Colin Leroy]

Hi,

> This bug was triggered by writing audio CDs (but not on data 
> CDs), as the audio frames are not aligned well (2352 bytes),
> so the user pages don't just get mapped.

This patch has been reported by a few people as fixing the leak problem,
but the produced audio CDs are corrupt.
(I didn't try it myself, already trashed 2 CDs ;-))

see http://kerneltrap.org/node/view/3659 for example

Thanks for your work,
-- 
Colin