bio_uncopy_user mem leak

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?

Quoted 5 lines · 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?

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.

Quoted 4 lines · Con Kolivas

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

fs/bio.c renamed from fs/bio.c.1 +1 -0

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

Hi Chris,

Quoted 18 lines · Chris Mason

Quoted 1 line · Con Kolivas

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

fs/bio.c renamed from fs/bio.c.1 +1 -0

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

fs/bio.c line 471

Quoted 10 lines

fs/bio.c renamed from fs/bio.c.1 +2 -1

@@ -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 :-(

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

Quoted 3 lines · Con Kolivas

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.

Con Kolivas <[email protected]> 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

Quoted 39 lines · Kurt Garloff

Hi Chris,

Quoted 21 lines · Chris Mason

Quoted 4 lines · Con Kolivas

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

fs/bio.c renamed from fs/bio.c.1 +1 -0

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

fs/bio.c line 471

Quoted 10 lines

fs/bio.c renamed from fs/bio.c.1 +2 -1

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

Quoted 6 lines

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

Quoted 1 line · Kurt Garloff

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