writable packet CD mounting in 2.6.26.x?

writable packet CD mounting in 2.6.26.x?

[Nix]

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.

hades:/tmp# ls -l /dev/pktcdvd
total 0
brw-rw-rw- 1 root root  254,  0 2008-08-05 00:45 cdrw
crw-r--r-- 1 root root   10, 63 2008-08-05 00:44 control
brw-rw-rw- 1 root cdrom 254,  0 2008-08-05 00:45 pktcdvd0


hades:/tmp# mount -o rw /dev/pktcdvd/cdrw /mnt/pcdrw
mount: block device /dev/pktcdvd/cdrw is write-protected, mounting read-only

hades:/tmp# mount -o rw /dev/pktcdvd/pktcdvd0 /mnt/pcdrw
mount: block device /dev/pktcdvd/pktcdvd0 is write-protected, mounting read-only

The strace says that we get -EROFS always, now:

mount("/dev/pktcdvd/cdrw", "/mnt/pcdrw", "udf"..., MS_NOSUID|MS_NODEV|MS_NOEXEC|0x200000, NULL) = -1 EROFS (Read-only file system)


Anyone got any ideas? There are no suspicious-looking changes in
pktcdvd.c itself, so maybe this lies deeper in the block layer. Or
perhaps I'm mounting it wrong in some obscure way.

Re: writable packet CD mounting and non-root formatting in 2.6.26.x?

[Nix]

On 6 Aug 2008, nix@esperi.org.uk told this:

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

Formatting them as non-root (with cdrwtool 1.0.0b3) seems broken too:

nix@hades 129 /home/nix% cdrwtool -d /dev/cdrw -q < /dev/null
using device /dev/cdrw
7294KB internal buffer
setting write speed to 4x
wait_cmd: Bad address
Command failed: bb 00 ff ff 02 c0 00 00 00 00 00 00 - sense 00.00.00
set speed
can't unlock door

It seems to work as root (well, it's got further but the lengthy
formatting process is still continuing).

Re: writable packet CD mounting in 2.6.26.x?

[Alexey Dobriyan]

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.
> 
> hades:/tmp# ls -l /dev/pktcdvd
> total 0
> brw-rw-rw- 1 root root  254,  0 2008-08-05 00:45 cdrw
> crw-r--r-- 1 root root   10, 63 2008-08-05 00:44 control
> brw-rw-rw- 1 root cdrom 254,  0 2008-08-05 00:45 pktcdvd0
> 
> 
> hades:/tmp# mount -o rw /dev/pktcdvd/cdrw /mnt/pcdrw
> mount: block device /dev/pktcdvd/cdrw is write-protected, mounting read-only
> 
> hades:/tmp# mount -o rw /dev/pktcdvd/pktcdvd0 /mnt/pcdrw
> mount: block device /dev/pktcdvd/pktcdvd0 is write-protected, mounting read-only
> 
> The strace says that we get -EROFS always, now:
> 
> mount("/dev/pktcdvd/cdrw", "/mnt/pcdrw", "udf"..., MS_NOSUID|MS_NODEV|MS_NOEXEC|0x200000, NULL) = -1 EROFS (Read-only file system)
> 
> 
> Anyone got any ideas? There are no suspicious-looking changes in
> pktcdvd.c itself, so maybe this lies deeper in the block layer. Or
> perhaps I'm mounting it wrong in some obscure way.

Reproduced. There must be some kick-ass code in pktcdvd driver. :^)


# mount -t udf -o rw /dev/pktcdvd/0  tmp/
mount: block device /dev/pktcdvd/0 is write-protected, mounting read-only
								^^^^^^^^^
Segmentation fault


Linux version 2.6.27-rc2 (ad@x200) (gcc version 4.1.2 20061115 (prerelease) (Debian 4.1.1-21)) #1 SMP PREEMPT Wed Aug 6 14:43:05 MSD 2008
pktcdvd: writer pktcdvd0 mapped to sr0
------------[ cut here ]------------
WARNING: at fs/sysfs/dir.c:463 sysfs_add_one+0x4c/0x50()
sysfs: duplicate filename '254:0' can not be created
Modules linked in: udf crc_itu_t isofs zlib_inflate pktcdvd usbhid fan sbp2 loop uvcvideo compat_ioctl32 videodev v4l1_compat pcmcia sr_mod cdrom snd_hda_intel snd_pcm tifm_7xx1 yenta_socket snd_timer tifm_core i2c_i801 psmouse ehci_hcd rsrc_nonstatic pcmcia_core snd ohci1394 uhci_hcd ata_piix soundcore serio_raw usbcore i2c_core ieee1394 r8169 snd_page_alloc battery ac thermal button evdev [last unloaded: pktcdvd]
Pid: 23706, comm: bash Not tainted 2.6.27-rc2 #1
 [<c0122f36>] warn_slowpath+0x76/0x90
 [<c01d7617>] ida_get_new_above+0x87/0x180
 [<c01d740b>] idr_get_empty_slot+0xeb/0x270
 [<c01d7617>] ida_get_new_above+0x87/0x180
 [<c0183e2e>] find_inode+0x3e/0x70
 [<c01ab7c0>] sysfs_ilookup_test+0x0/0x10
 [<c01abaa1>] sysfs_find_dirent+0x21/0x30
 [<c01abacc>] __sysfs_add_one+0x1c/0x80
 [<c01abb7c>] sysfs_add_one+0x4c/0x50
 [<c01ac978>] sysfs_do_create_link+0x88/0x120
 [<c01aca3f>] sysfs_create_link+0xf/0x20
 [<c022b6e6>] device_add+0x3a6/0x540
 [<c01d7f29>] kobject_init+0x29/0x80
 [<c022b922>] device_create_vargs+0x92/0xc0
 [<c022b97f>] device_create+0x2f/0x40
 [<f8b6f2b6>] pkt_setup_dev+0x356/0x4c0 [pktcdvd]
 [<f8b6f4bc>] class_pktcdvd_store_add+0x9c/0xc0 [pktcdvd]
 [<f8b6f420>] class_pktcdvd_store_add+0x0/0xc0 [pktcdvd]
 [<c022dc09>] class_attr_store+0x29/0x40
 [<c01ab121>] sysfs_write_file+0xa1/0x110
 [<c01ab080>] sysfs_write_file+0x0/0x110
 [<c0171872>] vfs_write+0x92/0xc0
 [<c0171cf1>] sys_write+0x41/0x70
 [<c0102f7d>] sysenter_do_call+0x12/0x25
 [<c02c0000>] msi_ht_cap_enabled+0x20/0xb0
 =======================
---[ end trace ed70665f87e7ceb8 ]---
BUG: unable to handle kernel NULL pointer dereference at 0000000c
IP: [<f8b6d639>] :pktcdvd:pkt_ioctl+0x19/0xd0
*pde = 00000000 
Oops: 0000 [#1] PREEMPT SMP 
Modules linked in: udf crc_itu_t isofs zlib_inflate pktcdvd usbhid fan sbp2 loop uvcvideo compat_ioctl32 videodev v4l1_compat pcmcia sr_mod cdrom snd_hda_intel snd_pcm tifm_7xx1 yenta_socket snd_timer tifm_core i2c_i801 psmouse ehci_hcd rsrc_nonstatic pcmcia_core snd ohci1394 uhci_hcd ata_piix soundcore serio_raw usbcore i2c_core ieee1394 r8169 snd_page_alloc battery ac thermal button evdev [last unloaded: pktcdvd]

Pid: 22816, comm: mount Tainted: G        W (2.6.27-rc2 #1)
EIP: 0060:[<f8b6d639>] EFLAGS: 00210282 CPU: 0
EIP is at pkt_ioctl+0x19/0xd0 [pktcdvd]
EAX: 00000000 EBX: f8b6d620 ECX: c32dfda0 EDX: 00005310
ESI: 00005310 EDI: 00000000 EBP: c32dfda0 ESP: c32dfc54
 DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
Process mount (pid: 22816, ti=c32df000 task=f70d6520 task.ti=c32df000)
Stack: 0000ffff f8b6d620 f73cf400 00000000 f7420aec c01d147f 00000001 f7420a80 
       fffffdfd 00005310 f7420aec c01d17cf 00005310 c32dfda0 c32dfcb4 010e2000 
       00010101 00000000 00000000 f73cf400 ffffffff ffff00bb 0000ffff 00000000 
Call Trace:
 [<f8b6d620>] pkt_ioctl+0x0/0xd0 [pktcdvd]
 [<c01d147f>] blkdev_driver_ioctl+0x2f/0x80
 [<c01d17cf>] blkdev_ioctl+0x2ff/0x830
 [<c0196f05>] set_blocksize+0x85/0x90
 [<f8b70979>] pkt_open+0x59/0x4e0 [pktcdvd]
 [<c01d2267>] exact_lock+0x7/0x10
 [<c022edb8>] kobj_lookup+0x158/0x170
 [<c01dc563>] string+0x23/0xa0
 [<c01dca3b>] vsnprintf+0x45b/0x5e0
 [<c01db519>] strsep+0x19/0x30
 [<f8bb551c>] udf_parse_options+0x6c/0x2f0 [udf]
 [<c0197d3e>] ioctl_by_bdev+0x2e/0x50
 [<f8bb1dea>] udf_get_last_session+0x1a/0x40 [udf]
 [<f8bb7c19>] udf_fill_super+0x849/0x910 [udf]
 [<c01a97fe>] disk_name+0x3e/0xc0
 [<c0173511>] get_sb_bdev+0x101/0x130
 [<c0157dac>] kstrdup+0x3c/0x70
 [<f8bb5441>] udf_get_sb+0x21/0x30 [udf]
 [<f8bb73d0>] udf_fill_super+0x0/0x910 [udf]
 [<c0173063>] vfs_kern_mount+0x43/0x90
 [<c017310d>] do_kern_mount+0x3d/0xe0
 [<c0188de1>] do_new_mount+0x81/0xc0
 [<c0188f97>] do_mount+0x177/0x1d0
 [<c0186c33>] copy_mount_options+0x43/0x150
 [<c0179933>] getname+0xb3/0xe0
 [<c0189067>] sys_mount+0x77/0xc0
 [<c0102f7d>] sysenter_do_call+0x12/0x25
 [<c02c0000>] msi_ht_cap_enabled+0x20/0xb0
 =======================
Code: 8b 5c 24 30 8b 74 24 34 8b 7c 24 38 83 c4 3c c3 89 f6 83 ec 14 89 74 24 08 89 d6 89 7c 24 0c 89 c7 89 6c 24 10 89 cd 89 5c 24 04 <8b> 40 0c 8b 58 0c e8 5c 7e 75 c7 81 fe 09 53 00 00 8b 83 04 01 
EIP: [<f8b6d639>] pkt_ioctl+0x19/0xd0 [pktcdvd] SS:ESP 0068:c32dfc54
---[ end trace ed70665f87e7ceb8 ]---

Re: writable packet CD mounting in 2.6.26.x?

[Alexey Dobriyan]

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.
> > 
> > hades:/tmp# ls -l /dev/pktcdvd
> > total 0
> > brw-rw-rw- 1 root root  254,  0 2008-08-05 00:45 cdrw
> > crw-r--r-- 1 root root   10, 63 2008-08-05 00:44 control
> > brw-rw-rw- 1 root cdrom 254,  0 2008-08-05 00:45 pktcdvd0
> > 
> > 
> > hades:/tmp# mount -o rw /dev/pktcdvd/cdrw /mnt/pcdrw
> > mount: block device /dev/pktcdvd/cdrw is write-protected, mounting read-only
> > 
> > hades:/tmp# mount -o rw /dev/pktcdvd/pktcdvd0 /mnt/pcdrw
> > mount: block device /dev/pktcdvd/pktcdvd0 is write-protected, mounting read-only
> > 
> > The strace says that we get -EROFS always, now:
> > 
> > mount("/dev/pktcdvd/cdrw", "/mnt/pcdrw", "udf"..., MS_NOSUID|MS_NODEV|MS_NOEXEC|0x200000, NULL) = -1 EROFS (Read-only file system)
> > 
> > 
> > Anyone got any ideas? There are no suspicious-looking changes in
> > pktcdvd.c itself, so maybe this lies deeper in the block layer. Or
> > perhaps I'm mounting it wrong in some obscure way.

Ho-hum, I get ->mmc3_profile = 9 here in both .25 anf .26 kernel, so I
can't meaningfully debug it here. :-(

Does it -EROFS because of pkt_writable_disc()? If yes, apply this patch
too.

--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -1934,6 +1934,7 @@ static int pkt_writable_track(struct pktcdvd_device *pd, track_information *ti)
  */
 static int pkt_writable_disc(struct pktcdvd_device *pd, disc_information *di)
 {
+	printk("%s: pd = %p, ->mmc3_profile = %u\n", __func__, pd, pd->mmc3_profile);
 	switch (pd->mmc3_profile) {
 		case 0x0a: /* CD-RW */
 		case 0xffff: /* MMC3 not supported */
@@ -1986,7 +1987,16 @@ static noinline_for_stack int pkt_probe_settings(struct pktcdvd_device *pd)
 	cgc.cmd[0] = GPCMD_GET_CONFIGURATION;
 	cgc.cmd[8] = 8;
 	ret = pkt_generic_packet(pd, &cgc);
+	{
+		int i;
+
+		printk("%s:", __func__);
+		for (i = 0; i < 12; i++)
+			printk(" %02x", buf[i]);
+		printk("\n");
+	}
 	pd->mmc3_profile = ret ? 0xffff : buf[6] << 8 | buf[7];
+	printk("%s: pd = %p, ->mmc3_profile = %u\n", __func__, pd, pd->mmc3_profile);
 
 	memset(&di, 0, sizeof(disc_information));
 	memset(&ti, 0, sizeof(track_information));

Re: writable packet CD mounting in 2.6.26.x?

[Nix]

[Back now, sorry for the delay, but I didn't even have mobile phone access
 in my tent, let alone an Internet connection. I had lots of running water,
 though, lots and lots and lots: I'm not sure it stopped raining for more
 than ten minutes in that whole week.]

On 8 Aug 2008, Alexey Dobriyan uttered the following:
> On Fri, Aug 08, 2008 at 07:14:37AM +0400, Alexey Dobriyan wrote:
>> > Anyone got any ideas? There are no suspicious-looking changes in
>> > pktcdvd.c itself, so maybe this lies deeper in the block layer. Or
>> > perhaps I'm mounting it wrong in some obscure way.
>
> Ho-hum, I get ->mmc3_profile = 9 here in both .25 anf .26 kernel, so I
> can't meaningfully debug it here. :-(
>
> Does it -EROFS because of pkt_writable_disc()? If yes, apply this patch
> too.

Too? Was there another patch?

I see

pkt_probe_settings: 00 00 00 ac 00 00 00 00 00 00 00 00
pkt_probe_settings: pd = edd80ee0, ->mmc3_profile = 0
pkt_writable_disc: pd = edd80ee0, ->mmc3_profile = 0

Thus we get 0 back from pkt_writable_disc(). (This drive is a Yamaha
CRW3200E, at least five years old, but it's a CD-RW so should definitely
support MMC3, I'd have thought.)

That probe result looks really odd. Maybe something's wrong with sg?

(This is an ATAPI device, and I'm using the old IDE layer, not
libata. Maybe I should switch and see if libata works better...)

Writable packet CD-RW mounting broken in 2.6.26 by your commit 68154e90c9d1492d570671ae181d9a8f8530da55

[Nix]

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@lab.ntt.co.jp>
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@lab.ntt.co.jp>
    Cc: Tejun Heo <htejun@gmail.com>
    Signed-off-by: Jens Axboe <jens.axboe@oracle.com>

With this patch from Alexey applied for diagnostics:

--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -1934,6 +1934,7 @@ static int pkt_writable_track(struct pktcdvd_device *pd, track_information *ti)
  */
 static int pkt_writable_disc(struct pktcdvd_device *pd, disc_information *di)
 {
+	printk("%s: pd = %p, ->mmc3_profile = %u\n", __func__, pd, pd->mmc3_profile);
 	switch (pd->mmc3_profile) {
 		case 0x0a: /* CD-RW */
 		case 0xffff: /* MMC3 not supported */
@@ -1986,7 +1987,16 @@ static noinline_for_stack int pkt_probe_settings(struct pktcdvd_device *pd)
 	cgc.cmd[0] = GPCMD_GET_CONFIGURATION;
 	cgc.cmd[8] = 8;
 	ret = pkt_generic_packet(pd, &cgc);
+	{
+		int i;
+
+		printk("%s:", __func__);
+		for (i = 0; i < 12; i++)
+			printk(" %02x", buf[i]);
+		printk("\n");
+	}
 	pd->mmc3_profile = ret ? 0xffff : buf[6] << 8 | buf[7];
+	printk("%s: pd = %p, ->mmc3_profile = %u\n", __func__, pd, pd->mmc3_profile);
 
 	memset(&di, 0, sizeof(disc_information));
 	memset(&ti, 0, sizeof(track_information));

We see output upon packet-written CD-RW mount going from the expected:

Aug 23 22:15:24 hades warning: kernel: pkt_probe_settings: 00 00 00 ac 00 00 00 0a 00 00 00 00
Aug 23 22:15:24 hades warning: kernel: pkt_probe_settings: pd = efce7ec0, ->mmc3_profile = 10
Aug 23 22:15:24 hades warning: kernel: pkt_writable_disc: pd = efce7ec0, ->mmc3_profile = 10

(i.e. `this is a CD-RW'), to the incorrect-by-one-byte

Aug 23 22:06:41 hades warning: kernel: pkt_probe_settings: 00 00 00 ac 00 00 00 00 00 00 00 00
                                                                                ^^
Aug 23 22:06:41 hades warning: kernel: pkt_probe_settings: pd = efc97dc0, ->mmc3_profile = 0
Aug 23 22:06:41 hades warning: kernel: pkt_writable_disc: pd = efc97dc0, ->mmc3_profile = 0

i.e., not a CD-RW, writable mounts impossible.

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

Further investigations soon but I've had enough of rebooting my desktop
for tonight.

-- 
`Not even vi uses vi key bindings for its command line.' --- PdS

Re: Writable packet CD-RW mounting broken in 2.6.26 by your commit 68154e90c9d1492d570671ae181d9a8f8530da55

[FUJITA Tomonori]

On Sat, 23 Aug 2008 22:48:55 +0100
Nix <nix@esperi.org.uk> 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@lab.ntt.co.jp>
> 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@lab.ntt.co.jp>
>     Cc: Tejun Heo <htejun@gmail.com>
>     Signed-off-by: Jens Axboe <jens.axboe@oracle.com>

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);
 }

Re: Writable packet CD-RW mounting broken in 2.6.26 by your commit 68154e90c9d1492d570671ae181d9a8f8530da55

[Nix]

On 24 Aug 2008, FUJITA Tomonori spake thusly:
> I think that I put a bug in bio_copy_kern(). Can you try this patch?

It works, I can back up again!

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

Great.

I suspect this is -stable material, right?

[PATCH 0/2] bio: blk_rq_map_* bounce support fixes

[FUJITA Tomonori]

This patchset fixes blk_rq_map_user_iov and blk_rq_map_kern bounce
support, which were introduced in 2.6.26. They can't handle READ
requests that partially completed.

This is against the for-linus branch in the block tree.

I think that we can clean up the code a bit (e.g. bio_copy_kern could
use __bio_copy_iov) but the patches need to go to the stable tree so I
kept the change to a minimum.

[PATCH 1/2] bio: fix bio_copy_kern() handling of bio->bv_len

[FUJITA Tomonori]

The commit 68154e90c9d1492d570671ae181d9a8f8530da55 introduced
bio_copy_kern() to add bounce support to blk_rq_map_kern.

bio_copy_kern() uses bio->bv_len to copy data for READ commands after
the completion but it doesn't work with a request that partially
completed. SCSI always completes a PC request as a whole but seems
some don't.

This patch fixes bio_copy_kern to handle the above case. As
bio_copy_user does, bio_copy_kern uses struct bio_map_data to store
struct bio_vec.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Reported-by: Nix <nix@esperi.org.uk>
Tested-by: Nix <nix@esperi.org.uk>
Cc: stable@kernel.org
---
 fs/bio.c |   38 ++++++++++++++++++++++++++++----------
 1 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/fs/bio.c b/fs/bio.c
index 8000e2f..8b1f5ee 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -469,20 +469,21 @@ static void bio_free_map_data(struct bio_map_data *bmd)
 	kfree(bmd);
 }
 
-static struct bio_map_data *bio_alloc_map_data(int nr_segs, int iov_count)
+static struct bio_map_data *bio_alloc_map_data(int nr_segs, int iov_count,
+					       gfp_t gfp_mask)
 {
-	struct bio_map_data *bmd = kmalloc(sizeof(*bmd), GFP_KERNEL);
+	struct bio_map_data *bmd = kmalloc(sizeof(*bmd), gfp_mask);
 
 	if (!bmd)
 		return NULL;
 
-	bmd->iovecs = kmalloc(sizeof(struct bio_vec) * nr_segs, GFP_KERNEL);
+	bmd->iovecs = kmalloc(sizeof(struct bio_vec) * nr_segs, gfp_mask);
 	if (!bmd->iovecs) {
 		kfree(bmd);
 		return NULL;
 	}
 
-	bmd->sgvecs = kmalloc(sizeof(struct sg_iovec) * iov_count, GFP_KERNEL);
+	bmd->sgvecs = kmalloc(sizeof(struct sg_iovec) * iov_count, gfp_mask);
 	if (bmd->sgvecs)
 		return bmd;
 
@@ -596,7 +597,7 @@ struct bio *bio_copy_user_iov(struct request_queue *q, struct sg_iovec *iov,
 		len += iov[i].iov_len;
 	}
 
-	bmd = bio_alloc_map_data(nr_pages, iov_count);
+	bmd = bio_alloc_map_data(nr_pages, iov_count, GFP_KERNEL);
 	if (!bmd)
 		return ERR_PTR(-ENOMEM);
 
@@ -942,19 +943,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 +982,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, gfp_mask);
+	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 +1030,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);
 }
-- 
1.5.5.GIT

[PATCH 2/2] bio: fix __bio_copy_iov() handling of bio->bv_len

[FUJITA Tomonori]

The commit c5dec1c3034f1ae3503efbf641ff3b0273b64797 introduced
__bio_copy_iov() to add bounce support to blk_rq_map_user_iov.

__bio_copy_iov() uses bio->bv_len to copy data for READ commands after
the completion but it doesn't work with a request that partially
completed. SCSI always completes a PC request as a whole but seems
some don't.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: stable@kernel.org
---
 fs/bio.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/bio.c b/fs/bio.c
index 8b1f5ee..3cba7ae 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -492,8 +492,8 @@ static struct bio_map_data *bio_alloc_map_data(int nr_segs, int iov_count,
 	return NULL;
 }
 
-static int __bio_copy_iov(struct bio *bio, struct sg_iovec *iov, int iov_count,
-			  int uncopy)
+static int __bio_copy_iov(struct bio *bio, struct bio_vec *iovecs,
+			  struct sg_iovec *iov, int iov_count, int uncopy)
 {
 	int ret = 0, i;
 	struct bio_vec *bvec;
@@ -503,7 +503,7 @@ static int __bio_copy_iov(struct bio *bio, struct sg_iovec *iov, int iov_count,
 
 	__bio_for_each_segment(bvec, bio, i, 0) {
 		char *bv_addr = page_address(bvec->bv_page);
-		unsigned int bv_len = bvec->bv_len;
+		unsigned int bv_len = iovecs[i].bv_len;
 
 		while (bv_len && iov_idx < iov_count) {
 			unsigned int bytes;
@@ -555,7 +555,7 @@ int bio_uncopy_user(struct bio *bio)
 	struct bio_map_data *bmd = bio->bi_private;
 	int ret;
 
-	ret = __bio_copy_iov(bio, bmd->sgvecs, bmd->nr_sgvecs, 1);
+	ret = __bio_copy_iov(bio, bmd->iovecs, bmd->sgvecs, bmd->nr_sgvecs, 1);
 
 	bio_free_map_data(bmd);
 	bio_put(bio);
@@ -634,7 +634,7 @@ struct bio *bio_copy_user_iov(struct request_queue *q, struct sg_iovec *iov,
 	 * success
 	 */
 	if (!write_to_vm) {
-		ret = __bio_copy_iov(bio, iov, iov_count, 0);
+		ret = __bio_copy_iov(bio, bio->bi_io_vec, iov, iov_count, 0);
 		if (ret)
 			goto cleanup;
 	}
-- 
1.5.5.GIT

Re: [PATCH 0/2] bio: blk_rq_map_* bounce support fixes

[Jens Axboe]

On Mon, Aug 25 2008, FUJITA Tomonori wrote:
> This patchset fixes blk_rq_map_user_iov and blk_rq_map_kern bounce
> support, which were introduced in 2.6.26. They can't handle READ
> requests that partially completed.
> 
> This is against the for-linus branch in the block tree.
> 
> I think that we can clean up the code a bit (e.g. bio_copy_kern could
> use __bio_copy_iov) but the patches need to go to the stable tree so I
> kept the change to a minimum.

Agree, for 2.6.27 lets keep it at the simplest. I've applied it, thanks
a lot for fixing this!

-- 
Jens Axboe

Re: Writable packet CD-RW mounting broken in 2.6.26 by your commit 68154e90c9d1492d570671ae181d9a8f8530da55

[FUJITA Tomonori]

On Sun, 24 Aug 2008 18:33:06 +0100
Nix <nix@esperi.org.uk> wrote:

> On 24 Aug 2008, FUJITA Tomonori spake thusly:
> > I think that I put a bug in bio_copy_kern(). Can you try this patch?
> 
> It works, I can back up again!

Thanks for testing.


> > I think that bio_uncopy_user has the same bug. I'll send a patch after
> > fixing this bug.
> 
> Great.
> 
> I suspect this is -stable material, right?

Yes, there are the same bugs in 2.6.26.

I added the Cc: stable tag on the patches so they will go to the
stable tree as soon as they get merged into mainline, I think.

Again, sorry about these bugs and thanks for the investigation.

Re: Writable packet CD-RW mounting broken in 2.6.26 by your commit 68154e90c9d1492d570671ae181d9a8f8530da55

[Nix]

On 25 Aug 2008, FUJITA Tomonori verbalised:

> On Sun, 24 Aug 2008 18:33:06 +0100
> Nix <nix@esperi.org.uk> wrote:
>> I suspect this is -stable material, right?
>
> Yes, there are the same bugs in 2.6.26.

I was testing in 2.6.26, so yes, they're definitely there.

> I added the Cc: stable tag on the patches so they will go to the
> stable tree as soon as they get merged into mainline, I think.
>
> Again, sorry about these bugs and thanks for the investigation.

Hey, everyone has bugs. Not everyone can get them fixed in ~6hrs
in the early morning .jp time :) so I think you come out ahead on
this one.

-- 
`Not even vi uses vi key bindings for its command line.' --- PdS

pktcdvd: BKL pushdown fallout (was Re: writable packet CD mounting in 2.6.26.x?)

[Alexey Dobriyan]

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.
> > 
> > hades:/tmp# ls -l /dev/pktcdvd
> > total 0
> > brw-rw-rw- 1 root root  254,  0 2008-08-05 00:45 cdrw
> > crw-r--r-- 1 root root   10, 63 2008-08-05 00:44 control
> > brw-rw-rw- 1 root cdrom 254,  0 2008-08-05 00:45 pktcdvd0
> > 
> > 
> > hades:/tmp# mount -o rw /dev/pktcdvd/cdrw /mnt/pcdrw
> > mount: block device /dev/pktcdvd/cdrw is write-protected, mounting read-only
> > 
> > hades:/tmp# mount -o rw /dev/pktcdvd/pktcdvd0 /mnt/pcdrw
> > mount: block device /dev/pktcdvd/pktcdvd0 is write-protected, mounting read-only
> > 
> > The strace says that we get -EROFS always, now:
> > 
> > mount("/dev/pktcdvd/cdrw", "/mnt/pcdrw", "udf"..., MS_NOSUID|MS_NODEV|MS_NOEXEC|0x200000, NULL) = -1 EROFS (Read-only file system)
> > 
> > 
> > Anyone got any ideas? There are no suspicious-looking changes in
> > pktcdvd.c itself, so maybe this lies deeper in the block layer. Or
> > perhaps I'm mounting it wrong in some obscure way.
> 
> Reproduced. There must be some kick-ass code in pktcdvd driver. :^)
> 
> 
> # mount -t udf -o rw /dev/pktcdvd/0  tmp/
> mount: block device /dev/pktcdvd/0 is write-protected, mounting read-only
> 								^^^^^^^^^
> Segmentation fault
> 
> 
> Linux version 2.6.27-rc2 (ad@x200) (gcc version 4.1.2 20061115 (prerelease) (Debian 4.1.1-21)) #1 SMP PREEMPT Wed Aug 6 14:43:05 MSD 2008
> pktcdvd: writer pktcdvd0 mapped to sr0


> BUG: unable to handle kernel NULL pointer dereference at 0000000c

	file = NULL

> IP: [<f8b6d639>] :pktcdvd:pkt_ioctl+0x19/0xd0
> *pde = 00000000 
> Oops: 0000 [#1] PREEMPT SMP 
> Modules linked in: udf crc_itu_t isofs zlib_inflate pktcdvd usbhid fan sbp2 loop uvcvideo compat_ioctl32 videodev v4l1_compat pcmcia sr_mod cdrom snd_hda_intel snd_pcm tifm_7xx1 yenta_socket snd_timer tifm_core i2c_i801 psmouse ehci_hcd rsrc_nonstatic pcmcia_core snd ohci1394 uhci_hcd ata_piix soundcore serio_raw usbcore i2c_core ieee1394 r8169 snd_page_alloc battery ac thermal button evdev [last unloaded: pktcdvd]
> 
> Pid: 22816, comm: mount Tainted: G        W (2.6.27-rc2 #1)
> EIP: 0060:[<f8b6d639>] EFLAGS: 00210282 CPU: 0
> EIP is at pkt_ioctl+0x19/0xd0 [pktcdvd]
> EAX: 00000000 EBX: f8b6d620 ECX: c32dfda0 EDX: 00005310
> ESI: 00005310 EDI: 00000000 EBP: c32dfda0 ESP: c32dfc54
>  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
> Process mount (pid: 22816, ti=c32df000 task=f70d6520 task.ti=c32df000)
> Stack: 0000ffff f8b6d620 f73cf400 00000000 f7420aec c01d147f 00000001 f7420a80 
>        fffffdfd 00005310 f7420aec c01d17cf 00005310 c32dfda0 c32dfcb4 010e2000 
>        00010101 00000000 00000000 f73cf400 ffffffff ffff00bb 0000ffff 00000000 
> Call Trace:
>  [<f8b6d620>] pkt_ioctl+0x0/0xd0 [pktcdvd]
>  [<c01d147f>] blkdev_driver_ioctl+0x2f/0x80
>  [<c01d17cf>] blkdev_ioctl+0x2ff/0x830
>  [<c0196f05>] set_blocksize+0x85/0x90
>  [<f8b70979>] pkt_open+0x59/0x4e0 [pktcdvd]
>  [<c01d2267>] exact_lock+0x7/0x10
>  [<c022edb8>] kobj_lookup+0x158/0x170
>  [<c01dc563>] string+0x23/0xa0
>  [<c01dca3b>] vsnprintf+0x45b/0x5e0
>  [<c01db519>] strsep+0x19/0x30
>  [<f8bb551c>] udf_parse_options+0x6c/0x2f0 [udf]
>  [<c0197d3e>] ioctl_by_bdev+0x2e/0x50
>  [<f8bb1dea>] udf_get_last_session+0x1a/0x40 [udf]
>  [<f8bb7c19>] udf_fill_super+0x849/0x910 [udf]
>  [<c01a97fe>] disk_name+0x3e/0xc0
>  [<c0173511>] get_sb_bdev+0x101/0x130
>  [<c0157dac>] kstrdup+0x3c/0x70
>  [<f8bb5441>] udf_get_sb+0x21/0x30 [udf]
>  [<f8bb73d0>] udf_fill_super+0x0/0x910 [udf]
>  [<c0173063>] vfs_kern_mount+0x43/0x90
>  [<c017310d>] do_kern_mount+0x3d/0xe0
>  [<c0188de1>] do_new_mount+0x81/0xc0
>  [<c0188f97>] do_mount+0x177/0x1d0
>  [<c0186c33>] copy_mount_options+0x43/0x150
>  [<c0179933>] getname+0xb3/0xe0
>  [<c0189067>] sys_mount+0x77/0xc0

Not suprisingly, indeed.

	udf_get_last_session
	ioctl_by_bdev(bdev, CDROMMULTISESSION,
	ioctl with file = NULL

pkt_ioctl is ->unlocked_ioctl, so can't tolerate NULL file.

Introduced in 5b6155ee70e9c4d2ad7e6f514c8eee06e2711c3a aka BKL pushdown.