Re: Writable packet CD-RW mounting broken in 2.6.26 by your commit68154e90c9d1492d570671ae181d9a8f8530da55

[FUJITA Tomonori]

On Sat, 23 Aug 2008 22:48:55 +0100
Nix <nix@xxxxxxxxxxxxx> wrote:

> On 8 Aug 2008, Alexey Dobriyan told this:
> 
> > On Fri, Aug 08, 2008 at 07:14:37AM +0400, Alexey Dobriyan wrote:
> >> On Wed, Aug 06, 2008 at 08:12:49PM +0100, Nix wrote:
> >> > It seems to be impossible to mount packet-written CD-RWs writably in
> >> > 2.6.26.x, even as root. Things worked in 2.6.25.x.
> 
> Bisected. The bug starts here:
> 
> commit 68154e90c9d1492d570671ae181d9a8f8530da55
> Author: FUJITA Tomonori <fujita.tomonori@xxxxxxxxxxxxx>
> Date:   Fri Apr 25 12:47:50 2008 +0200
> 
>     block: add dma alignment and padding support to blk_rq_map_kern
> 
>     This patch adds bio_copy_kern similar to
>     bio_copy_user. blk_rq_map_kern uses bio_copy_kern instead of
>     bio_map_kern if necessary.
> 
>     bio_copy_kern uses temporary pages and the bi_end_io callback frees
>     these pages. bio_copy_kern saves the original kernel buffer at
>     bio->bi_private it doesn't use something like struct bio_map_data to
>     store the information about the caller.
> 
>     Signed-off-by: FUJITA Tomonori <fujita.tomonori@xxxxxxxxxxxxx>
>     Cc: Tejun Heo <htejun@xxxxxxxxx>
>     Signed-off-by: Jens Axboe <jens.axboe@xxxxxxxxxx>

Sorry about the bug and thanks for the investigation.


> So something in this patch seems to be corrupting the results of at
> least some ATAPI sg requests going to or from the block layer: and
> indeed blk_rq_map_kern(), called by pkt_generic_packet(), is changed by
> this patch.
> 
> Tomonori, Jens, can you see any immediate cause of this? It seems to me
> that the alignment check in blk_rq_map_kern() *must* be going wrong,
> somehow: this can't be an unaligned buffer or the code wouldn't have
> worked before the patch, yet if the buffer was aligned, we should be
> calling bio_map_kern(), which this patch hasn't changed. Perhaps the
> buffer length of 12 is leading it to believe that it should go via a
> bounce buffer and is calling the new code (and tripping some bug in it,
> since presumably even the new code isn't expected to corrupt the data in
> flight ;) )

Using bio_copy_kern() should be fine since it does the same thing that
bio_map_kern() does (the difference is that bio_copy_kern() does extra
memory copy).

Actually, pkt_probe_settings() should use bio_copy_kern because it
tries to do DMA with a buffer on the stack (it doesn't work on some
architectures). blk_rq_map_kern() in the latest git has the stack
checking too.

I think that I put a bug in bio_copy_kern(). Can you try this patch?

I think that bio_uncopy_user has the same bug. I'll send a patch after
fixing this bug.


diff --git a/fs/bio.c b/fs/bio.c
index 8000e2f..f58a7ba 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -942,19 +942,22 @@ static void bio_copy_kern_endio(struct bio *bio, int err)
 {
 	struct bio_vec *bvec;
 	const int read = bio_data_dir(bio) == READ;
-	char *p = bio->bi_private;
+	struct bio_map_data *bmd = bio->bi_private;
 	int i;
+	char *p = bmd->sgvecs[0].iov_base;
 
 	__bio_for_each_segment(bvec, bio, i, 0) {
 		char *addr = page_address(bvec->bv_page);
+		int len = bmd->iovecs[i].bv_len;
 
 		if (read && !err)
-			memcpy(p, addr, bvec->bv_len);
+			memcpy(p, addr, len);
 
 		__free_page(bvec->bv_page);
-		p += bvec->bv_len;
+		p += len;
 	}
 
+	bio_free_map_data(bmd);
 	bio_put(bio);
 }
 
@@ -978,11 +981,21 @@ struct bio *bio_copy_kern(struct request_queue *q, void *data, unsigned int len,
 	const int nr_pages = end - start;
 	struct bio *bio;
 	struct bio_vec *bvec;
+	struct bio_map_data *bmd;
 	int i, ret;
+	struct sg_iovec iov;
+
+	iov.iov_base = data;
+	iov.iov_len = len;
+
+	bmd = bio_alloc_map_data(nr_pages, 1);
+	if (!bmd)
+		return ERR_PTR(-ENOMEM);
 
+	ret = -ENOMEM;
 	bio = bio_alloc(gfp_mask, nr_pages);
 	if (!bio)
-		return ERR_PTR(-ENOMEM);
+		goto out_bmd;
 
 	while (len) {
 		struct page *page;
@@ -1016,14 +1029,18 @@ struct bio *bio_copy_kern(struct request_queue *q, void *data, unsigned int len,
 		}
 	}
 
-	bio->bi_private = data;
+	bio->bi_private = bmd;
 	bio->bi_end_io = bio_copy_kern_endio;
+
+	bio_set_map_data(bmd, bio, &iov, 1);
 	return bio;
 cleanup:
 	bio_for_each_segment(bvec, bio, i)
 		__free_page(bvec->bv_page);
 
 	bio_put(bio);
+out_bmd:
+	bio_free_map_data(bmd);
 
 	return ERR_PTR(ret);
 }



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/