public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* Oops when using growisofs
@ 2008-06-22 16:18 Michael Buesch
  2008-06-22 21:22 ` Arnd Bergmann
  0 siblings, 1 reply; 22+ messages in thread
From: Michael Buesch @ 2008-06-22 16:18 UTC (permalink / raw)
  To: linux-kernel

The following oops happend when trying to append data to a DVD
with growisofs -M.

Burning DVD...
Executing 'genisoimage -C 16,737776 -M /dev/fd/3 -R -J foobar | builtin_dd of=/dev/dvd obs=32k seek=46111'
I: -input-charset not specified, using utf-8 (detected in locale settings)
Rock Ridge signatures found
/dev/dvd: "Current Write Speed" is 8.2x1352KBps.
:-( unable to WRITE@LBA=b41f0h: Input/output error
:-( write failed: Input/output error

This is a wireless-testing.git kernel, which corresponds to 2.6.26-rc6

[28375.893176] Unable to handle kernel paging request for data at address 0x00000008
[28375.893181] Faulting instruction address: 0xc00000000012df84
[28375.893186] Oops: Kernel access of bad area, sig: 11 [#1]
[28375.893189] PREEMPT SMP NR_CPUS=4 NUMA PowerMac
[28375.893200] Modules linked in: nls_cp437 isofs udf rfkill_input hci_usb nfsd exportfs auth_rpcgss deadline_iosched nbd ppdev lp parport nfs lockd sunrpc fuse sr_mod sbp2 arc4 ecb b43 evdev af_packet mac80211 cfg80211 rfkill appledisplay usbhid ssb ide_pci_generic ohci1394 ieee1394 uninorth_agp agpgart
[28375.893287] NIP: c00000000012df84 LR: c00000000012df70 CTR: c000000000137b00
[28375.893293] REGS: c000000116aeb300 TRAP: 0300   Not tainted  (2.6.26-rc6-wl)
[28375.893296] MSR: 9000000000009032 <EE,ME,IR,DR>  CR: 28004842  XER: 000fffff
[28375.893316] DAR: 0000000000000008, DSISR: 0000000040000000
[28375.893320] TASK = c00000011636db00[4667] 'kded' THREAD: c000000116ae8000 CPU: 2
[28375.893327] GPR00: c00000000012df70 c000000116aeb580 c00000000090ff20 0000000000000000 
[28375.893340] GPR04: 0000000000010000 0000000000000001 c00000011bfe37a0 0000000000000010 
[28375.893352] GPR08: f00000000694d280 0000000000000000 c0000000008c0be0 0000000000000000 
[28375.893364] GPR12: 0000000028004842 c000000000941700 0000000000000004 c000000116aeb840 
[28375.893377] GPR16: c0000001195d8f78 c0000000008c0cb8 c0000000000bd064 0000000000000003 
[28375.893389] GPR20: 0000000000000000 c0000001195d8d68 0000000000000004 c0000001195d8f80 
[28375.893402] GPR24: c00000000082c700 0000000000010000 f00000000694d280 0000000000000000 
[28375.893415] GPR28: 0000000000000000 f00000000694d280 c00000000088e640 c000000116aeb580 
[28375.893428] NIP [c00000000012df84] .create_empty_buffers+0x44/0x180
[28375.893439] LR [c00000000012df70] .create_empty_buffers+0x30/0x180
[28375.893446] Call Trace:
[28375.893451] [c000000116aeb580] [c00000000012df70] .create_empty_buffers+0x30/0x180 (unreliable)
[28375.893463] [c000000116aeb620] [c0000000001331a4] .block_read_full_page+0x464/0x480
[28375.893473] [c000000116aeb750] [c000000000137b28] .blkdev_readpage+0x28/0x50
[28375.893483] [c000000116aeb7d0] [c0000000000bd2c8] .__do_page_cache_readahead+0x368/0x390
[28375.893496] [c000000116aeb8e0] [c0000000000bd698] .ondemand_readahead+0x158/0x270
[28375.893505] [c000000116aeb9a0] [c0000000000bd8fc] .page_cache_sync_readahead+0x3c/0x50
[28375.893513] [c000000116aeba20] [c0000000000b2994] .generic_file_aio_read+0x4f4/0x630
[28375.893523] [c000000116aebb50] [c0000000000f6fa4] .do_sync_read+0xe4/0x180
[28375.893534] [c000000116aebcf0] [c0000000000f79c4] .vfs_read+0xf4/0x1c0
[28375.893542] [c000000116aebd90] [c0000000000f8234] .sys_read+0x54/0xa0
[28375.893550] [c000000116aebe30] [c0000000000076d4] syscall_exit+0x0/0x40
[28375.893560] Instruction dump:
[28375.893566] f8010010 f821ff61 7cbb2b78 38a00001 7c7d1b78 7c3f0b78 4bfffe65 7c7c1b78 
[28375.893586] 7c691b78 4800000c 60000000 7d695b78 <e9690008> e8090000 2fab0000 7c00db78 
[28375.893607] ---[ end trace d2a7775e4472c36e ]---

-- 
Greetings Michael.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Oops when using growisofs
  2008-06-22 16:18 Oops when using growisofs Michael Buesch
@ 2008-06-22 21:22 ` Arnd Bergmann
  2008-06-22 21:31   ` Arnd Bergmann
  2008-06-22 22:05   ` Michael Buesch
  0 siblings, 2 replies; 22+ messages in thread
From: Arnd Bergmann @ 2008-06-22 21:22 UTC (permalink / raw)
  To: Michael Buesch; +Cc: linux-kernel, Jens Axboe

On Sunday 22 June 2008, Michael Buesch wrote:
> [28375.893176] Unable to handle kernel paging request for data at address 0x00000008

access at offset 8 of a NULL pointer, maybe bh->b_this_page?

> [28375.893181] Faulting instruction address: 0xc00000000012df84
> [28375.893186] Oops: Kernel access of bad area, sig: 11 [#1]
> [28375.893189] PREEMPT SMP NR_CPUS=4 NUMA PowerMac

Ok, important information: ppc64 architecture. It would be nice to mention
in the bug report, but here we can see it as well.

> [28375.893320] TASK = c00000011636db00[4667] 'kded' THREAD: c000000116ae8000 CPU: 2

task was kded, i.e. not growisofs itself, thouh growisofs is probably the one
that has caused this problem (by exausting memory).

> [28375.893327] GPR00: c00000000012df70 c000000116aeb580 c00000000090ff20 0000000000000000 
> [28375.893340] GPR04: 0000000000010000 0000000000000001 c00000011bfe37a0 0000000000000010 
> [28375.893352] GPR08: f00000000694d280 0000000000000000 c0000000008c0be0 0000000000000000 
> [28375.893364] GPR12: 0000000028004842 c000000000941700 0000000000000004 c000000116aeb840 
> [28375.893377] GPR16: c0000001195d8f78 c0000000008c0cb8 c0000000000bd064 0000000000000003 
> [28375.893389] GPR20: 0000000000000000 c0000001195d8d68 0000000000000004 c0000001195d8f80 
> [28375.893402] GPR24: c00000000082c700 0000000000010000 f00000000694d280 0000000000000000 
> [28375.893415] GPR28: 0000000000000000 f00000000694d280 c00000000088e640 c000000116aeb580 

Note: r9 and r3 are both NULL pointers. r3 is the value returned from alloc_page_buffers.
R9 is a copy of that, which gets accessed.

> [28375.893428] NIP [c00000000012df84] .create_empty_buffers+0x44/0x180
> [28375.893439] LR [c00000000012df70] .create_empty_buffers+0x30/0x180
> [28375.893446] Call Trace:
> [28375.893451] [c000000116aeb580] [c00000000012df70] .create_empty_buffers+0x30/0x180 (unreliable)
> [28375.893463] [c000000116aeb620] [c0000000001331a4] .block_read_full_page+0x464/0x480
> [28375.893473] [c000000116aeb750] [c000000000137b28] .blkdev_readpage+0x28/0x50
> [28375.893483] [c000000116aeb7d0] [c0000000000bd2c8] .__do_page_cache_readahead+0x368/0x390
> [28375.893496] [c000000116aeb8e0] [c0000000000bd698] .ondemand_readahead+0x158/0x270
> [28375.893505] [c000000116aeb9a0] [c0000000000bd8fc] .page_cache_sync_readahead+0x3c/0x50
> [28375.893513] [c000000116aeba20] [c0000000000b2994] .generic_file_aio_read+0x4f4/0x630
> [28375.893523] [c000000116aebb50] [c0000000000f6fa4] .do_sync_read+0xe4/0x180
> [28375.893534] [c000000116aebcf0] [c0000000000f79c4] .vfs_read+0xf4/0x1c0
> [28375.893542] [c000000116aebd90] [c0000000000f8234] .sys_read+0x54/0xa0
> [28375.893550] [c000000116aebe30] [c0000000000076d4] syscall_exit+0x0/0x40

a simple file read, from a random process.

> [28375.893560] Instruction dump:
> [28375.893566] f8010010 f821ff61 7cbb2b78 38a00001 7c7d1b78 7c3f0b78 4bfffe65 7c7c1b78 
> [28375.893586] 7c691b78 4800000c 60000000 7d695b78 <e9690008> e8090000 2fab0000 7c00db78 
> [28375.893607] ---[ end trace d2a7775e4472c36e ]---
> 

4800000c is the branch to alloc_page_buffers
7d695b78 copies the return value of that to r9
e9690008 dereferences r9

Evidently, alloc_page_buffers got an out of memory condition, which was not caught
by create_empty_buffers. No idea how it should be handled, but the fact that it's
not looks like a bug to me ;-).

	Arnd <><

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Oops when using growisofs
  2008-06-22 21:22 ` Arnd Bergmann
@ 2008-06-22 21:31   ` Arnd Bergmann
  2008-06-22 22:09     ` Michael Buesch
  2008-06-22 22:05   ` Michael Buesch
  1 sibling, 1 reply; 22+ messages in thread
From: Arnd Bergmann @ 2008-06-22 21:31 UTC (permalink / raw)
  To: Michael Buesch; +Cc: linux-kernel, Jens Axboe

On Sunday 22 June 2008, Arnd Bergmann wrote:

> Evidently, alloc_page_buffers got an out of memory condition, which was not caught
> by create_empty_buffers. No idea how it should be handled, but the fact that it's
> not looks like a bug to me ;-).

Oh, btw: http://www.kerneloops.org/search.php?search=create_empty_buffers&btnG=Function+Search
finds 15 more instances of the same bug (and one other bug), across lots of
kernel versions and architectures. The bug has been there since we started
using git.

	Arnd <><

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Oops when using growisofs
  2008-06-22 21:22 ` Arnd Bergmann
  2008-06-22 21:31   ` Arnd Bergmann
@ 2008-06-22 22:05   ` Michael Buesch
  2008-06-22 22:28     ` Michael Buesch
  1 sibling, 1 reply; 22+ messages in thread
From: Michael Buesch @ 2008-06-22 22:05 UTC (permalink / raw)
  To: Arnd Bergmann; +Cc: linux-kernel, Jens Axboe

On Sunday 22 June 2008 23:22:04 Arnd Bergmann wrote:
> > [28375.893181] Faulting instruction address: 0xc00000000012df84
> > [28375.893186] Oops: Kernel access of bad area, sig: 11 [#1]
> > [28375.893189] PREEMPT SMP NR_CPUS=4 NUMA PowerMac
> 
> Ok, important information: ppc64 architecture. It would be nice to mention
> in the bug report, but here we can see it as well.

Yeah I'm sorry. I thought this was obvious. :)

> > [28375.893320] TASK = c00000011636db00[4667] 'kded' THREAD: c000000116ae8000 CPU: 2
> 
> task was kded, i.e. not growisofs itself, thouh growisofs is probably the one
> that has caused this problem (by exausting memory).

I don't think it exausted memory. oom-killer messages would have been in the logs.
And this machine has 2.5GiB memory. It continued to run fine after restarting kded.
I sent this bugreport on the machine that oopsed without a reboot.

Is it possible that this was a kernel race between kded and growisofs?
This is a 4-way SMP machine.

> > [28375.893327] GPR00: c00000000012df70 c000000116aeb580 c00000000090ff20 0000000000000000 
> > [28375.893340] GPR04: 0000000000010000 0000000000000001 c00000011bfe37a0 0000000000000010 
> > [28375.893352] GPR08: f00000000694d280 0000000000000000 c0000000008c0be0 0000000000000000 
> > [28375.893364] GPR12: 0000000028004842 c000000000941700 0000000000000004 c000000116aeb840 
> > [28375.893377] GPR16: c0000001195d8f78 c0000000008c0cb8 c0000000000bd064 0000000000000003 
> > [28375.893389] GPR20: 0000000000000000 c0000001195d8d68 0000000000000004 c0000001195d8f80 
> > [28375.893402] GPR24: c00000000082c700 0000000000010000 f00000000694d280 0000000000000000 
> > [28375.893415] GPR28: 0000000000000000 f00000000694d280 c00000000088e640 c000000116aeb580 
> 
> Note: r9 and r3 are both NULL pointers. r3 is the value returned from alloc_page_buffers.
> R9 is a copy of that, which gets accessed.

Hm, yeah. I looked at that code already, but I can't see how it could return
a NULL pointer.

> > [28375.893560] Instruction dump:
> > [28375.893566] f8010010 f821ff61 7cbb2b78 38a00001 7c7d1b78 7c3f0b78 4bfffe65 7c7c1b78 
> > [28375.893586] 7c691b78 4800000c 60000000 7d695b78 <e9690008> e8090000 2fab0000 7c00db78 
> > [28375.893607] ---[ end trace d2a7775e4472c36e ]---
> > 
> 
> 4800000c is the branch to alloc_page_buffers
> 7d695b78 copies the return value of that to r9
> e9690008 dereferences r9
> 
> Evidently, alloc_page_buffers got an out of memory condition, which was not caught
> by create_empty_buffers. No idea how it should be handled, but the fact that it's
> not looks like a bug to me ;-).

alloc_page_buffers should never return a NULL pointer here, as far as I can see.
It clearly is a bug. An oops always is a bug.


-- 
Greetings Michael.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Oops when using growisofs
  2008-06-22 21:31   ` Arnd Bergmann
@ 2008-06-22 22:09     ` Michael Buesch
  0 siblings, 0 replies; 22+ messages in thread
From: Michael Buesch @ 2008-06-22 22:09 UTC (permalink / raw)
  To: Arnd Bergmann; +Cc: linux-kernel, Jens Axboe

On Sunday 22 June 2008 23:31:29 Arnd Bergmann wrote:
> On Sunday 22 June 2008, Arnd Bergmann wrote:
> 
> > Evidently, alloc_page_buffers got an out of memory condition, which was not caught
> > by create_empty_buffers. No idea how it should be handled, but the fact that it's
> > not looks like a bug to me ;-).
> 
> Oh, btw: http://www.kerneloops.org/search.php?search=create_empty_buffers&btnG=Function+Search
> finds 15 more instances of the same bug (and one other bug), across lots of
> kernel versions and architectures. The bug has been there since we started
> using git.

So, ehm. Maybe some debugging code should be added??
We could at least add code that prevents a crash and adds WARN_ONs.


-- 
Greetings Michael.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Oops when using growisofs
  2008-06-22 22:05   ` Michael Buesch
@ 2008-06-22 22:28     ` Michael Buesch
  2008-06-23  6:34       ` Andrew Morton
  0 siblings, 1 reply; 22+ messages in thread
From: Michael Buesch @ 2008-06-22 22:28 UTC (permalink / raw)
  To: Arnd Bergmann; +Cc: linux-kernel, Jens Axboe

On Monday 23 June 2008 00:05:51 Michael Buesch wrote:
> > Note: r9 and r3 are both NULL pointers. r3 is the value returned from alloc_page_buffers.
> > R9 is a copy of that, which gets accessed.
> 
> Hm, yeah. I looked at that code already, but I can't see how it could return
> a NULL pointer.

Well, actually, it can return a NULL pointer.

 928         head = NULL;
 929         offset = PAGE_SIZE;
 930         while ((offset -= size) >= 0) {
...
 949         }
 950         return head;

So if size, which is a passed in as parameter, is > PAGE_SIZE it will return NULL.

The size parameter is calculated by doing
blocksize = 1 << inode->i_blkbits;
in an earlier function in the callchain.

So, well. I dunno what i_blkbits is. There's no docs in struct inode.

-- 
Greetings Michael.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Oops when using growisofs
  2008-06-22 22:28     ` Michael Buesch
@ 2008-06-23  6:34       ` Andrew Morton
  2008-06-23  6:59         ` Nick Piggin
  2008-06-24 17:28         ` Jan Kara
  0 siblings, 2 replies; 22+ messages in thread
From: Andrew Morton @ 2008-06-23  6:34 UTC (permalink / raw)
  To: Michael Buesch; +Cc: Arnd Bergmann, linux-kernel, Jens Axboe, Jan Kara

On Mon, 23 Jun 2008 00:28:20 +0200 Michael Buesch <mb@bu3sch.de> wrote:

> On Monday 23 June 2008 00:05:51 Michael Buesch wrote:
> > > Note: r9 and r3 are both NULL pointers. r3 is the value returned from alloc_page_buffers.
> > > R9 is a copy of that, which gets accessed.
> > 
> > Hm, yeah. I looked at that code already, but I can't see how it could return
> > a NULL pointer.
> 
> Well, actually, it can return a NULL pointer.
> 
>  928         head = NULL;
>  929         offset = PAGE_SIZE;
>  930         while ((offset -= size) >= 0) {
> ...
>  949         }
>  950         return head;
> 
> So if size, which is a passed in as parameter, is > PAGE_SIZE it will return NULL.
> 
> The size parameter is calculated by doing
> blocksize = 1 << inode->i_blkbits;
> in an earlier function in the callchain.

Yes, that's a more likely scenario.  isofs has a history of passing
garbage into the VFS.

> So, well. I dunno what i_blkbits is. There's no docs in struct inode.

It's log2 of the filesystem blocksize.  It'd be interesting to work out
what value isofs is setting it to, and why.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Oops when using growisofs
  2008-06-23  6:34       ` Andrew Morton
@ 2008-06-23  6:59         ` Nick Piggin
  2008-06-24 17:28         ` Jan Kara
  1 sibling, 0 replies; 22+ messages in thread
From: Nick Piggin @ 2008-06-23  6:59 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Michael Buesch, Arnd Bergmann, linux-kernel, Jens Axboe, Jan Kara

On Monday 23 June 2008 16:34, Andrew Morton wrote:
> On Mon, 23 Jun 2008 00:28:20 +0200 Michael Buesch <mb@bu3sch.de> wrote:
> > On Monday 23 June 2008 00:05:51 Michael Buesch wrote:
> > > > Note: r9 and r3 are both NULL pointers. r3 is the value returned from
> > > > alloc_page_buffers. R9 is a copy of that, which gets accessed.
> > >
> > > Hm, yeah. I looked at that code already, but I can't see how it could
> > > return a NULL pointer.
> >
> > Well, actually, it can return a NULL pointer.
> >
> >  928         head = NULL;
> >  929         offset = PAGE_SIZE;
> >  930         while ((offset -= size) >= 0) {
> > ...
> >  949         }
> >  950         return head;
> >
> > So if size, which is a passed in as parameter, is > PAGE_SIZE it will
> > return NULL.
> >
> > The size parameter is calculated by doing
> > blocksize = 1 << inode->i_blkbits;
> > in an earlier function in the callchain.
>
> Yes, that's a more likely scenario.  isofs has a history of passing
> garbage into the VFS.

Yes isofs will pass in a too-big page here (IIRC 32K or something).
And trigger this oops.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Oops when using growisofs
  2008-06-23  6:34       ` Andrew Morton
  2008-06-23  6:59         ` Nick Piggin
@ 2008-06-24 17:28         ` Jan Kara
  2008-06-24 18:39           ` Michael Buesch
  1 sibling, 1 reply; 22+ messages in thread
From: Jan Kara @ 2008-06-24 17:28 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Michael Buesch, Arnd Bergmann, linux-kernel, Jens Axboe, Jan Kara

> On Mon, 23 Jun 2008 00:28:20 +0200 Michael Buesch <mb@bu3sch.de> wrote:
> 
> > On Monday 23 June 2008 00:05:51 Michael Buesch wrote:
> > > > Note: r9 and r3 are both NULL pointers. r3 is the value returned from alloc_page_buffers.
> > > > R9 is a copy of that, which gets accessed.
> > > 
> > > Hm, yeah. I looked at that code already, but I can't see how it could return
> > > a NULL pointer.
> > 
> > Well, actually, it can return a NULL pointer.
> > 
> >  928         head = NULL;
> >  929         offset = PAGE_SIZE;
> >  930         while ((offset -= size) >= 0) {
> > ...
> >  949         }
> >  950         return head;
> > 
> > So if size, which is a passed in as parameter, is > PAGE_SIZE it will return NULL.
> > 
> > The size parameter is calculated by doing
> > blocksize = 1 << inode->i_blkbits;
> > in an earlier function in the callchain.
> 
> Yes, that's a more likely scenario.  isofs has a history of passing
> garbage into the VFS.
> 
> > So, well. I dunno what i_blkbits is. There's no docs in struct inode.
> 
> It's log2 of the filesystem blocksize.  It'd be interesting to work out
> what value isofs is setting it to, and why.
  Well, yes, that looks as a reason at the first sight. But what I don't
get is, how can isofs possibly set such a blocksize when it uses
sb_set_blocksize() which checks whether blocksize isn't larger than page
size... So it must be something less obvious.
  bd_set_size() can possibly set blocksize larger than PAGE_SIZE and
it's called from do_open() but it uses bdev_hardsect_size() and that
shouldn't be larger than PAGE_SIZE either (at least drivers seem to take
care of this).
  I have seen one more report of this Oops for SLES10 kernel and also in that
case an IO error happened so probably that is a trigger... But so far I
don't get the details.

									Honza
-- 
Jan Kara <jack@suse.cz>
SuSE CR Labs

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Oops when using growisofs
  2008-06-24 17:28         ` Jan Kara
@ 2008-06-24 18:39           ` Michael Buesch
  2008-06-25  1:42             ` Arnd Bergmann
  2008-06-25  9:37             ` Jan Kara
  0 siblings, 2 replies; 22+ messages in thread
From: Michael Buesch @ 2008-06-24 18:39 UTC (permalink / raw)
  To: Jan Kara; +Cc: Andrew Morton, Arnd Bergmann, linux-kernel, Jens Axboe, Jan Kara

On Tuesday 24 June 2008 19:28:12 Jan Kara wrote:
> > On Mon, 23 Jun 2008 00:28:20 +0200 Michael Buesch <mb@bu3sch.de> wrote:
> > 
> > > On Monday 23 June 2008 00:05:51 Michael Buesch wrote:
> > > > > Note: r9 and r3 are both NULL pointers. r3 is the value returned from alloc_page_buffers.
> > > > > R9 is a copy of that, which gets accessed.
> > > > 
> > > > Hm, yeah. I looked at that code already, but I can't see how it could return
> > > > a NULL pointer.
> > > 
> > > Well, actually, it can return a NULL pointer.
> > > 
> > >  928         head = NULL;
> > >  929         offset = PAGE_SIZE;
> > >  930         while ((offset -= size) >= 0) {
> > > ...
> > >  949         }
> > >  950         return head;
> > > 
> > > So if size, which is a passed in as parameter, is > PAGE_SIZE it will return NULL.
> > > 
> > > The size parameter is calculated by doing
> > > blocksize = 1 << inode->i_blkbits;
> > > in an earlier function in the callchain.
> > 
> > Yes, that's a more likely scenario.  isofs has a history of passing
> > garbage into the VFS.
> > 
> > > So, well. I dunno what i_blkbits is. There's no docs in struct inode.
> > 
> > It's log2 of the filesystem blocksize.  It'd be interesting to work out
> > what value isofs is setting it to, and why.
>   Well, yes, that looks as a reason at the first sight. But what I don't
> get is, how can isofs possibly set such a blocksize when it uses
> sb_set_blocksize() which checks whether blocksize isn't larger than page
> size... So it must be something less obvious.
>   bd_set_size() can possibly set blocksize larger than PAGE_SIZE and
> it's called from do_open() but it uses bdev_hardsect_size() and that
> shouldn't be larger than PAGE_SIZE either (at least drivers seem to take
> care of this).
>   I have seen one more report of this Oops for SLES10 kernel and also in that
> case an IO error happened so probably that is a trigger... But so far I
> don't get the details.

Yeah the IO error is the trigger.
I noticed that it had obvious troubles accessing the DVD that was in the drive.
It sweeped over it for several seconds, then hung the system for 2 or 3 seconds
and then oopsed. But after that everything continued to work as usual.
(Except kded of course)

-- 
Greetings Michael.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Oops when using growisofs
  2008-06-24 18:39           ` Michael Buesch
@ 2008-06-25  1:42             ` Arnd Bergmann
  2008-06-25  9:37             ` Jan Kara
  1 sibling, 0 replies; 22+ messages in thread
From: Arnd Bergmann @ 2008-06-25  1:42 UTC (permalink / raw)
  To: Michael Buesch
  Cc: Jan Kara, Andrew Morton, linux-kernel, Jens Axboe, Jan Kara

On Tuesday 24 June 2008, Michael Buesch wrote:
> >   I have seen one more report of this Oops for SLES10 kernel and also in that
> > case an IO error happened so probably that is a trigger... But so far I
> > don't get the details.
> 
> Yeah the IO error is the trigger.
> I noticed that it had obvious troubles accessing the DVD that was in the drive.
> It sweeped over it for several seconds, then hung the system for 2 or 3 seconds
> and then oopsed. But after that everything continued to work as usual.
> (Except kded of course)

So maybe a use-after-free bug in the I/O error handling caused an inode to
be overwritten and then passed to an I/O function that used an invalid
inode->i_blkbits?

	Arnd <><

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Oops when using growisofs
  2008-06-24 18:39           ` Michael Buesch
  2008-06-25  1:42             ` Arnd Bergmann
@ 2008-06-25  9:37             ` Jan Kara
  2008-06-25  9:46               ` Michael Buesch
  1 sibling, 1 reply; 22+ messages in thread
From: Jan Kara @ 2008-06-25  9:37 UTC (permalink / raw)
  To: Michael Buesch; +Cc: Andrew Morton, Arnd Bergmann, linux-kernel, Jens Axboe

On Tue 24-06-08 20:39:18, Michael Buesch wrote:
> On Tuesday 24 June 2008 19:28:12 Jan Kara wrote:
> > > On Mon, 23 Jun 2008 00:28:20 +0200 Michael Buesch <mb@bu3sch.de> wrote:
> > > 
> > > > On Monday 23 June 2008 00:05:51 Michael Buesch wrote:
> > > > > > Note: r9 and r3 are both NULL pointers. r3 is the value returned from alloc_page_buffers.
> > > > > > R9 is a copy of that, which gets accessed.
> > > > > 
> > > > > Hm, yeah. I looked at that code already, but I can't see how it could return
> > > > > a NULL pointer.
> > > > 
> > > > Well, actually, it can return a NULL pointer.
> > > > 
> > > >  928         head = NULL;
> > > >  929         offset = PAGE_SIZE;
> > > >  930         while ((offset -= size) >= 0) {
> > > > ...
> > > >  949         }
> > > >  950         return head;
> > > > 
> > > > So if size, which is a passed in as parameter, is > PAGE_SIZE it will return NULL.
> > > > 
> > > > The size parameter is calculated by doing
> > > > blocksize = 1 << inode->i_blkbits;
> > > > in an earlier function in the callchain.
> > > 
> > > Yes, that's a more likely scenario.  isofs has a history of passing
> > > garbage into the VFS.
> > > 
> > > > So, well. I dunno what i_blkbits is. There's no docs in struct inode.
> > > 
> > > It's log2 of the filesystem blocksize.  It'd be interesting to work out
> > > what value isofs is setting it to, and why.
> >   Well, yes, that looks as a reason at the first sight. But what I don't
> > get is, how can isofs possibly set such a blocksize when it uses
> > sb_set_blocksize() which checks whether blocksize isn't larger than page
> > size... So it must be something less obvious.
> >   bd_set_size() can possibly set blocksize larger than PAGE_SIZE and
> > it's called from do_open() but it uses bdev_hardsect_size() and that
> > shouldn't be larger than PAGE_SIZE either (at least drivers seem to take
> > care of this).
> >   I have seen one more report of this Oops for SLES10 kernel and also in that
> > case an IO error happened so probably that is a trigger... But so far I
> > don't get the details.
> 
> Yeah the IO error is the trigger.
> I noticed that it had obvious troubles accessing the DVD that was in the drive.
> It sweeped over it for several seconds, then hung the system for 2 or 3 seconds
> and then oopsed. But after that everything continued to work as usual.
> (Except kded of course)
  Hmm, by "accessing" do you mean that you've mounted the burned DVD and when
browsing it the IO error and the oops occured or that IO error happened
when burning? It is important because in the first case i_blkbits would be
taken from some ISOFS inode desribing some file while in the second case
i_blkbits are from the inode of the device...

									Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Oops when using growisofs
  2008-06-25  9:37             ` Jan Kara
@ 2008-06-25  9:46               ` Michael Buesch
  2008-06-26 17:05                 ` Jan Kara
  0 siblings, 1 reply; 22+ messages in thread
From: Michael Buesch @ 2008-06-25  9:46 UTC (permalink / raw)
  To: Jan Kara; +Cc: Andrew Morton, Arnd Bergmann, linux-kernel, Jens Axboe

On Wednesday 25 June 2008 11:37:00 Jan Kara wrote:
> > Yeah the IO error is the trigger.
> > I noticed that it had obvious troubles accessing the DVD that was in the drive.
> > It sweeped over it for several seconds, then hung the system for 2 or 3 seconds
> > and then oopsed. But after that everything continued to work as usual.
> > (Except kded of course)
>   Hmm, by "accessing" do you mean that you've mounted the burned DVD and when
> browsing it the IO error and the oops occured or that IO error happened
> when burning? It is important because in the first case i_blkbits would be
> taken from some ISOFS inode desribing some file while in the second case
> i_blkbits are from the inode of the device...


I don't know. kded, which caused the oops, is always running. It is a KDE daemon
that polls device state and so on. So yeah, it might have accessed the drive
while growisofs was writing to it.

However with "accessing" I mean the DVD drive motor was spinning up and down
and the laser lens was moving like crazy. The sound that happens, if you put
a completely scratched DVD into the drive and it is unable to make sense of it.
However, this was not scratched. It was a new DVD with one session on it that
I just burnt 5 minutes before that. So I wanted to append another session to it
and it crashed and resulted in IO errors in growisofs.

-- 
Greetings Michael.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Oops when using growisofs
  2008-06-25  9:46               ` Michael Buesch
@ 2008-06-26 17:05                 ` Jan Kara
  2008-06-26 18:11                   ` Jens Axboe
  0 siblings, 1 reply; 22+ messages in thread
From: Jan Kara @ 2008-06-26 17:05 UTC (permalink / raw)
  To: Jens Axboe; +Cc: Michael Buesch, Andrew Morton, Arnd Bergmann, linux-kernel

On Wed 25-06-08 11:46:29, Michael Buesch wrote:
> On Wednesday 25 June 2008 11:37:00 Jan Kara wrote:
> > > Yeah the IO error is the trigger.
> > > I noticed that it had obvious troubles accessing the DVD that was in the drive.
> > > It sweeped over it for several seconds, then hung the system for 2 or 3 seconds
> > > and then oopsed. But after that everything continued to work as usual.
> > > (Except kded of course)
> >   Hmm, by "accessing" do you mean that you've mounted the burned DVD and when
> > browsing it the IO error and the oops occured or that IO error happened
> > when burning? It is important because in the first case i_blkbits would be
> > taken from some ISOFS inode desribing some file while in the second case
> > i_blkbits are from the inode of the device...
> I don't know. kded, which caused the oops, is always running. It is a KDE daemon
> that polls device state and so on. So yeah, it might have accessed the drive
> while growisofs was writing to it.
> 
> However with "accessing" I mean the DVD drive motor was spinning up and down
> and the laser lens was moving like crazy. The sound that happens, if you put
> a completely scratched DVD into the drive and it is unable to make sense of it.
> However, this was not scratched. It was a new DVD with one session on it that
> I just burnt 5 minutes before that. So I wanted to append another session to it
> and it crashed and resulted in IO errors in growisofs.
  I've been looking into this problem for some time. The only way how I see
blocksize can be set so big is in cdrom_read_capacity() in
drivers/ide/ide-cd.c. That basically blindly fills in queue->hardsect_size with what
the drive returns and this can propagate in bd_set_size() to i_blkbits.
Jens, do you think that is possible? Shouldn't ide_cd_read_toc() do some
sanity checks of the blocksize returned?

										Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Oops when using growisofs
  2008-06-26 17:05                 ` Jan Kara
@ 2008-06-26 18:11                   ` Jens Axboe
  2008-06-26 18:21                     ` Michael Buesch
  0 siblings, 1 reply; 22+ messages in thread
From: Jens Axboe @ 2008-06-26 18:11 UTC (permalink / raw)
  To: Jan Kara; +Cc: Michael Buesch, Andrew Morton, Arnd Bergmann, linux-kernel

On Thu, Jun 26 2008, Jan Kara wrote:
> On Wed 25-06-08 11:46:29, Michael Buesch wrote:
> > On Wednesday 25 June 2008 11:37:00 Jan Kara wrote:
> > > > Yeah the IO error is the trigger.
> > > > I noticed that it had obvious troubles accessing the DVD that was in the drive.
> > > > It sweeped over it for several seconds, then hung the system for 2 or 3 seconds
> > > > and then oopsed. But after that everything continued to work as usual.
> > > > (Except kded of course)
> > >   Hmm, by "accessing" do you mean that you've mounted the burned DVD and when
> > > browsing it the IO error and the oops occured or that IO error happened
> > > when burning? It is important because in the first case i_blkbits would be
> > > taken from some ISOFS inode desribing some file while in the second case
> > > i_blkbits are from the inode of the device...
> > I don't know. kded, which caused the oops, is always running. It is a KDE daemon
> > that polls device state and so on. So yeah, it might have accessed the drive
> > while growisofs was writing to it.
> > 
> > However with "accessing" I mean the DVD drive motor was spinning up and down
> > and the laser lens was moving like crazy. The sound that happens, if you put
> > a completely scratched DVD into the drive and it is unable to make sense of it.
> > However, this was not scratched. It was a new DVD with one session on it that
> > I just burnt 5 minutes before that. So I wanted to append another session to it
> > and it crashed and resulted in IO errors in growisofs.
>   I've been looking into this problem for some time. The only way how
>   I see blocksize can be set so big is in cdrom_read_capacity() in
>   drivers/ide/ide-cd.c. That basically blindly fills in
>   queue->hardsect_size with what the drive returns and this can
>   propagate in bd_set_size() to i_blkbits.  Jens, do you think that is
>   possible? Shouldn't ide_cd_read_toc() do some sanity checks of the
>   blocksize returned?

It can't hurt, the value should be >= 512b and <= 4kb. Normally it would
be 2kb, but some devices have a 512b switch so that is also seen. Not
sure that 1kb and 4kb are valid, but at least it would still point to
the drive possibly returning valid data and not garbage. So accept all
those, reject (and complain) if it isn't one of those and default to 2kb.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Oops when using growisofs
  2008-06-26 18:11                   ` Jens Axboe
@ 2008-06-26 18:21                     ` Michael Buesch
  2008-06-26 18:36                       ` Jens Axboe
  0 siblings, 1 reply; 22+ messages in thread
From: Michael Buesch @ 2008-06-26 18:21 UTC (permalink / raw)
  To: Jens Axboe; +Cc: Jan Kara, Andrew Morton, Arnd Bergmann, linux-kernel

On Thursday 26 June 2008 20:11:42 Jens Axboe wrote:
> On Thu, Jun 26 2008, Jan Kara wrote:
> > On Wed 25-06-08 11:46:29, Michael Buesch wrote:
> > > On Wednesday 25 June 2008 11:37:00 Jan Kara wrote:
> > > > > Yeah the IO error is the trigger.
> > > > > I noticed that it had obvious troubles accessing the DVD that was in the drive.
> > > > > It sweeped over it for several seconds, then hung the system for 2 or 3 seconds
> > > > > and then oopsed. But after that everything continued to work as usual.
> > > > > (Except kded of course)
> > > >   Hmm, by "accessing" do you mean that you've mounted the burned DVD and when
> > > > browsing it the IO error and the oops occured or that IO error happened
> > > > when burning? It is important because in the first case i_blkbits would be
> > > > taken from some ISOFS inode desribing some file while in the second case
> > > > i_blkbits are from the inode of the device...
> > > I don't know. kded, which caused the oops, is always running. It is a KDE daemon
> > > that polls device state and so on. So yeah, it might have accessed the drive
> > > while growisofs was writing to it.
> > > 
> > > However with "accessing" I mean the DVD drive motor was spinning up and down
> > > and the laser lens was moving like crazy. The sound that happens, if you put
> > > a completely scratched DVD into the drive and it is unable to make sense of it.
> > > However, this was not scratched. It was a new DVD with one session on it that
> > > I just burnt 5 minutes before that. So I wanted to append another session to it
> > > and it crashed and resulted in IO errors in growisofs.
> >   I've been looking into this problem for some time. The only way how
> >   I see blocksize can be set so big is in cdrom_read_capacity() in
> >   drivers/ide/ide-cd.c. That basically blindly fills in
> >   queue->hardsect_size with what the drive returns and this can
> >   propagate in bd_set_size() to i_blkbits.  Jens, do you think that is
> >   possible? Shouldn't ide_cd_read_toc() do some sanity checks of the
> >   blocksize returned?
> 
> It can't hurt, the value should be >= 512b and <= 4kb. Normally it would
> be 2kb, but some devices have a 512b switch so that is also seen. Not
> sure that 1kb and 4kb are valid, but at least it would still point to
> the drive possibly returning valid data and not garbage. So accept all
> those, reject (and complain) if it isn't one of those and default to 2kb.

I agree with the need for a hardware sanity check and I would happily test
any RFC patch :)

-- 
Greetings Michael.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Oops when using growisofs
  2008-06-26 18:21                     ` Michael Buesch
@ 2008-06-26 18:36                       ` Jens Axboe
  2008-06-26 18:39                         ` Michael Buesch
                                           ` (3 more replies)
  0 siblings, 4 replies; 22+ messages in thread
From: Jens Axboe @ 2008-06-26 18:36 UTC (permalink / raw)
  To: Michael Buesch; +Cc: Jan Kara, Andrew Morton, Arnd Bergmann, linux-kernel

On Thu, Jun 26 2008, Michael Buesch wrote:
> On Thursday 26 June 2008 20:11:42 Jens Axboe wrote:
> > On Thu, Jun 26 2008, Jan Kara wrote:
> > > On Wed 25-06-08 11:46:29, Michael Buesch wrote:
> > > > On Wednesday 25 June 2008 11:37:00 Jan Kara wrote:
> > > > > > Yeah the IO error is the trigger.
> > > > > > I noticed that it had obvious troubles accessing the DVD that was in the drive.
> > > > > > It sweeped over it for several seconds, then hung the system for 2 or 3 seconds
> > > > > > and then oopsed. But after that everything continued to work as usual.
> > > > > > (Except kded of course)
> > > > >   Hmm, by "accessing" do you mean that you've mounted the burned DVD and when
> > > > > browsing it the IO error and the oops occured or that IO error happened
> > > > > when burning? It is important because in the first case i_blkbits would be
> > > > > taken from some ISOFS inode desribing some file while in the second case
> > > > > i_blkbits are from the inode of the device...
> > > > I don't know. kded, which caused the oops, is always running. It is a KDE daemon
> > > > that polls device state and so on. So yeah, it might have accessed the drive
> > > > while growisofs was writing to it.
> > > > 
> > > > However with "accessing" I mean the DVD drive motor was spinning up and down
> > > > and the laser lens was moving like crazy. The sound that happens, if you put
> > > > a completely scratched DVD into the drive and it is unable to make sense of it.
> > > > However, this was not scratched. It was a new DVD with one session on it that
> > > > I just burnt 5 minutes before that. So I wanted to append another session to it
> > > > and it crashed and resulted in IO errors in growisofs.
> > >   I've been looking into this problem for some time. The only way how
> > >   I see blocksize can be set so big is in cdrom_read_capacity() in
> > >   drivers/ide/ide-cd.c. That basically blindly fills in
> > >   queue->hardsect_size with what the drive returns and this can
> > >   propagate in bd_set_size() to i_blkbits.  Jens, do you think that is
> > >   possible? Shouldn't ide_cd_read_toc() do some sanity checks of the
> > >   blocksize returned?
> > 
> > It can't hurt, the value should be >= 512b and <= 4kb. Normally it would
> > be 2kb, but some devices have a 512b switch so that is also seen. Not
> > sure that 1kb and 4kb are valid, but at least it would still point to
> > the drive possibly returning valid data and not garbage. So accept all
> > those, reject (and complain) if it isn't one of those and default to 2kb.
> 
> I agree with the need for a hardware sanity check and I would happily test
> any RFC patch :)

Something like this, totally untested...

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 68e7f19..5c1e663 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -1308,13 +1308,29 @@ static int cdrom_read_capacity(ide_drive_t *drive, unsigned long *capacity,
 	req.cmd_flags |= REQ_QUIET;
 
 	stat = ide_cd_queue_pc(drive, &req);
-	if (stat == 0) {
-		*capacity = 1 + be32_to_cpu(capbuf.lba);
-		*sectors_per_frame =
-			be32_to_cpu(capbuf.blocklen) >> SECTOR_BITS;
+	if (stat)
+		return stat;
+
+	/*
+	 * Sanity check the given block size
+	 */
+	switch (capbuf.blocklen) {
+	case 512:
+	case 1024:
+	case 2048:
+	case 4096:
+		break;
+	default:
+		printk(KERN_ERR "ide-cd: weird block size %u\n",
+							capbuf.blocklen);
+		printk(KERN_ERR "ide-cd: default to 2kb block size\n");
+		capbuf.blocklen = 2048;
+		break;
 	}
 
-	return stat;
+	*capacity = 1 + be32_to_cpu(capbuf.lba);
+	*sectors_per_frame = be32_to_cpu(capbuf.blocklen) >> SECTOR_BITS;
+	return 0;
 }
 
 static int cdrom_read_tocentry(ide_drive_t *drive, int trackno, int msf_flag,

-- 
Jens Axboe


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: Oops when using growisofs
  2008-06-26 18:36                       ` Jens Axboe
@ 2008-06-26 18:39                         ` Michael Buesch
  2008-06-26 18:41                           ` Jens Axboe
  2008-06-29 19:39                         ` Michael Buesch
                                           ` (2 subsequent siblings)
  3 siblings, 1 reply; 22+ messages in thread
From: Michael Buesch @ 2008-06-26 18:39 UTC (permalink / raw)
  To: Jens Axboe; +Cc: Jan Kara, Andrew Morton, Arnd Bergmann, linux-kernel

On Thursday 26 June 2008 20:36:11 Jens Axboe wrote:
> Something like this, totally untested...
> 
> diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
> index 68e7f19..5c1e663 100644
> --- a/drivers/ide/ide-cd.c
> +++ b/drivers/ide/ide-cd.c
> @@ -1308,13 +1308,29 @@ static int cdrom_read_capacity(ide_drive_t *drive, unsigned long *capacity,
>  	req.cmd_flags |= REQ_QUIET;
>  
>  	stat = ide_cd_queue_pc(drive, &req);
> -	if (stat == 0) {
> -		*capacity = 1 + be32_to_cpu(capbuf.lba);
> -		*sectors_per_frame =
> -			be32_to_cpu(capbuf.blocklen) >> SECTOR_BITS;
> +	if (stat)
> +		return stat;
> +
> +	/*
> +	 * Sanity check the given block size
> +	 */
> +	switch (capbuf.blocklen) {
> +	case 512:
> +	case 1024:
> +	case 2048:
> +	case 4096:
> +		break;
> +	default:
> +		printk(KERN_ERR "ide-cd: weird block size %u\n",
> +							capbuf.blocklen);
> +		printk(KERN_ERR "ide-cd: default to 2kb block size\n");

KERN_WARNING

> +		capbuf.blocklen = 2048;
> +		break;
>  	}
>  
> -	return stat;
> +	*capacity = 1 + be32_to_cpu(capbuf.lba);
> +	*sectors_per_frame = be32_to_cpu(capbuf.blocklen) >> SECTOR_BITS;
> +	return 0;
>  }
>  
>  static int cdrom_read_tocentry(ide_drive_t *drive, int trackno, int msf_flag,
> 

I'll test this. Thanks a lot. :)
However I cannot reproduce the bug. So I cannot tell whether it fixes it.

-- 
Greetings Michael.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Oops when using growisofs
  2008-06-26 18:39                         ` Michael Buesch
@ 2008-06-26 18:41                           ` Jens Axboe
  0 siblings, 0 replies; 22+ messages in thread
From: Jens Axboe @ 2008-06-26 18:41 UTC (permalink / raw)
  To: Michael Buesch; +Cc: Jan Kara, Andrew Morton, Arnd Bergmann, linux-kernel

On Thu, Jun 26 2008, Michael Buesch wrote:
> On Thursday 26 June 2008 20:36:11 Jens Axboe wrote:
> > Something like this, totally untested...
> > 
> > diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
> > index 68e7f19..5c1e663 100644
> > --- a/drivers/ide/ide-cd.c
> > +++ b/drivers/ide/ide-cd.c
> > @@ -1308,13 +1308,29 @@ static int cdrom_read_capacity(ide_drive_t *drive, unsigned long *capacity,
> >  	req.cmd_flags |= REQ_QUIET;
> >  
> >  	stat = ide_cd_queue_pc(drive, &req);
> > -	if (stat == 0) {
> > -		*capacity = 1 + be32_to_cpu(capbuf.lba);
> > -		*sectors_per_frame =
> > -			be32_to_cpu(capbuf.blocklen) >> SECTOR_BITS;
> > +	if (stat)
> > +		return stat;
> > +
> > +	/*
> > +	 * Sanity check the given block size
> > +	 */
> > +	switch (capbuf.blocklen) {
> > +	case 512:
> > +	case 1024:
> > +	case 2048:
> > +	case 4096:
> > +		break;
> > +	default:
> > +		printk(KERN_ERR "ide-cd: weird block size %u\n",
> > +							capbuf.blocklen);
> > +		printk(KERN_ERR "ide-cd: default to 2kb block size\n");
> 
> KERN_WARNING

It's pretty serious, so...

> > +		capbuf.blocklen = 2048;
> > +		break;
> >  	}
> >  
> > -	return stat;
> > +	*capacity = 1 + be32_to_cpu(capbuf.lba);
> > +	*sectors_per_frame = be32_to_cpu(capbuf.blocklen) >> SECTOR_BITS;
> > +	return 0;
> >  }
> >  
> >  static int cdrom_read_tocentry(ide_drive_t *drive, int trackno, int msf_flag,
> > 
> 
> I'll test this. Thanks a lot. :)
> However I cannot reproduce the bug. So I cannot tell whether it fixes it.

The patch wont hurt, probably we should just add it just in case.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Oops when using growisofs
  2008-06-26 18:36                       ` Jens Axboe
  2008-06-26 18:39                         ` Michael Buesch
@ 2008-06-29 19:39                         ` Michael Buesch
  2008-07-09 18:46                         ` Jan Kara
  2008-07-22  9:25                         ` Andrew Morton
  3 siblings, 0 replies; 22+ messages in thread
From: Michael Buesch @ 2008-06-29 19:39 UTC (permalink / raw)
  To: Jens Axboe; +Cc: Jan Kara, Andrew Morton, Arnd Bergmann, linux-kernel

On Thursday 26 June 2008 20:36:11 Jens Axboe wrote:
> Something like this, totally untested...

> +	switch (capbuf.blocklen) {
> +	case 512:
> +	case 1024:
> +	case 2048:
> +	case 4096:
> +		break;
> +	default:
> +		printk(KERN_ERR "ide-cd: weird block size %u\n",
> +							capbuf.blocklen);
> +		printk(KERN_ERR "ide-cd: default to 2kb block size\n");
> +		capbuf.blocklen = 2048;
> +		break;
>  	}

So I applied this patch and it works fine.
However I cannot say if this fixed it. The warning does not appear, however,
it's probably possible that the drive does only return a wrong blocksize
under weird conditions (firmware bug).

So in any case, I think this patch should be applied. Checking device sanity
is always a good thing.

-- 
Greetings Michael.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Oops when using growisofs
  2008-06-26 18:36                       ` Jens Axboe
  2008-06-26 18:39                         ` Michael Buesch
  2008-06-29 19:39                         ` Michael Buesch
@ 2008-07-09 18:46                         ` Jan Kara
  2008-07-22  9:25                         ` Andrew Morton
  3 siblings, 0 replies; 22+ messages in thread
From: Jan Kara @ 2008-07-09 18:46 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Michael Buesch, Jan Kara, Andrew Morton, Arnd Bergmann,
	linux-kernel

On Thu 26-06-08 20:36:11, Jens Axboe wrote:
> On Thu, Jun 26 2008, Michael Buesch wrote:
> > On Thursday 26 June 2008 20:11:42 Jens Axboe wrote:
> > > On Thu, Jun 26 2008, Jan Kara wrote:
> > > > On Wed 25-06-08 11:46:29, Michael Buesch wrote:
> > > > > On Wednesday 25 June 2008 11:37:00 Jan Kara wrote:
> > > > > > > Yeah the IO error is the trigger.
> > > > > > > I noticed that it had obvious troubles accessing the DVD that was in the drive.
> > > > > > > It sweeped over it for several seconds, then hung the system for 2 or 3 seconds
> > > > > > > and then oopsed. But after that everything continued to work as usual.
> > > > > > > (Except kded of course)
> > > > > >   Hmm, by "accessing" do you mean that you've mounted the burned DVD and when
> > > > > > browsing it the IO error and the oops occured or that IO error happened
> > > > > > when burning? It is important because in the first case i_blkbits would be
> > > > > > taken from some ISOFS inode desribing some file while in the second case
> > > > > > i_blkbits are from the inode of the device...
> > > > > I don't know. kded, which caused the oops, is always running. It is a KDE daemon
> > > > > that polls device state and so on. So yeah, it might have accessed the drive
> > > > > while growisofs was writing to it.
> > > > > 
> > > > > However with "accessing" I mean the DVD drive motor was spinning up and down
> > > > > and the laser lens was moving like crazy. The sound that happens, if you put
> > > > > a completely scratched DVD into the drive and it is unable to make sense of it.
> > > > > However, this was not scratched. It was a new DVD with one session on it that
> > > > > I just burnt 5 minutes before that. So I wanted to append another session to it
> > > > > and it crashed and resulted in IO errors in growisofs.
> > > >   I've been looking into this problem for some time. The only way how
> > > >   I see blocksize can be set so big is in cdrom_read_capacity() in
> > > >   drivers/ide/ide-cd.c. That basically blindly fills in
> > > >   queue->hardsect_size with what the drive returns and this can
> > > >   propagate in bd_set_size() to i_blkbits.  Jens, do you think that is
> > > >   possible? Shouldn't ide_cd_read_toc() do some sanity checks of the
> > > >   blocksize returned?
> > > 
> > > It can't hurt, the value should be >= 512b and <= 4kb. Normally it would
> > > be 2kb, but some devices have a 512b switch so that is also seen. Not
> > > sure that 1kb and 4kb are valid, but at least it would still point to
> > > the drive possibly returning valid data and not garbage. So accept all
> > > those, reject (and complain) if it isn't one of those and default to 2kb.
> > 
> > I agree with the need for a hardware sanity check and I would happily test
> > any RFC patch :)
> 
> Something like this, totally untested...
  BTW, just to confirm some positive result: On one of my machines I was
able to trigger the warning message in this patch. So it definitely fixes
some problem.

									Honza

> diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
> index 68e7f19..5c1e663 100644
> --- a/drivers/ide/ide-cd.c
> +++ b/drivers/ide/ide-cd.c
> @@ -1308,13 +1308,29 @@ static int cdrom_read_capacity(ide_drive_t *drive, unsigned long *capacity,
>  	req.cmd_flags |= REQ_QUIET;
>  
>  	stat = ide_cd_queue_pc(drive, &req);
> -	if (stat == 0) {
> -		*capacity = 1 + be32_to_cpu(capbuf.lba);
> -		*sectors_per_frame =
> -			be32_to_cpu(capbuf.blocklen) >> SECTOR_BITS;
> +	if (stat)
> +		return stat;
> +
> +	/*
> +	 * Sanity check the given block size
> +	 */
> +	switch (capbuf.blocklen) {
> +	case 512:
> +	case 1024:
> +	case 2048:
> +	case 4096:
> +		break;
> +	default:
> +		printk(KERN_ERR "ide-cd: weird block size %u\n",
> +							capbuf.blocklen);
> +		printk(KERN_ERR "ide-cd: default to 2kb block size\n");
> +		capbuf.blocklen = 2048;
> +		break;
>  	}
>  
> -	return stat;
> +	*capacity = 1 + be32_to_cpu(capbuf.lba);
> +	*sectors_per_frame = be32_to_cpu(capbuf.blocklen) >> SECTOR_BITS;
> +	return 0;
>  }
>  
>  static int cdrom_read_tocentry(ide_drive_t *drive, int trackno, int msf_flag,
> 
> -- 
> Jens Axboe
> 

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Oops when using growisofs
  2008-06-26 18:36                       ` Jens Axboe
                                           ` (2 preceding siblings ...)
  2008-07-09 18:46                         ` Jan Kara
@ 2008-07-22  9:25                         ` Andrew Morton
  3 siblings, 0 replies; 22+ messages in thread
From: Andrew Morton @ 2008-07-22  9:25 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Michael Buesch, Jan Kara, Arnd Bergmann, linux-kernel,
	Bartlomiej Zolnierkiewicz, linux-ide, stable

On Thu, 26 Jun 2008 20:36:11 +0200 Jens Axboe <jens.axboe@oracle.com> wrote:

> On Thu, Jun 26 2008, Michael Buesch wrote:
> > On Thursday 26 June 2008 20:11:42 Jens Axboe wrote:
> > > On Thu, Jun 26 2008, Jan Kara wrote:
> > > > On Wed 25-06-08 11:46:29, Michael Buesch wrote:
> > > > > On Wednesday 25 June 2008 11:37:00 Jan Kara wrote:
> > > > > > > Yeah the IO error is the trigger.
> > > > > > > I noticed that it had obvious troubles accessing the DVD that was in the drive.
> > > > > > > It sweeped over it for several seconds, then hung the system for 2 or 3 seconds
> > > > > > > and then oopsed. But after that everything continued to work as usual.
> > > > > > > (Except kded of course)
> > > > > >   Hmm, by "accessing" do you mean that you've mounted the burned DVD and when
> > > > > > browsing it the IO error and the oops occured or that IO error happened
> > > > > > when burning? It is important because in the first case i_blkbits would be
> > > > > > taken from some ISOFS inode desribing some file while in the second case
> > > > > > i_blkbits are from the inode of the device...
> > > > > I don't know. kded, which caused the oops, is always running. It is a KDE daemon
> > > > > that polls device state and so on. So yeah, it might have accessed the drive
> > > > > while growisofs was writing to it.
> > > > > 
> > > > > However with "accessing" I mean the DVD drive motor was spinning up and down
> > > > > and the laser lens was moving like crazy. The sound that happens, if you put
> > > > > a completely scratched DVD into the drive and it is unable to make sense of it.
> > > > > However, this was not scratched. It was a new DVD with one session on it that
> > > > > I just burnt 5 minutes before that. So I wanted to append another session to it
> > > > > and it crashed and resulted in IO errors in growisofs.
> > > >   I've been looking into this problem for some time. The only way how
> > > >   I see blocksize can be set so big is in cdrom_read_capacity() in
> > > >   drivers/ide/ide-cd.c. That basically blindly fills in
> > > >   queue->hardsect_size with what the drive returns and this can
> > > >   propagate in bd_set_size() to i_blkbits.  Jens, do you think that is
> > > >   possible? Shouldn't ide_cd_read_toc() do some sanity checks of the
> > > >   blocksize returned?
> > > 
> > > It can't hurt, the value should be >= 512b and <= 4kb. Normally it would
> > > be 2kb, but some devices have a 512b switch so that is also seen. Not
> > > sure that 1kb and 4kb are valid, but at least it would still point to
> > > the drive possibly returning valid data and not garbage. So accept all
> > > those, reject (and complain) if it isn't one of those and default to 2kb.
> > 
> > I agree with the need for a hardware sanity check and I would happily test
> > any RFC patch :)
> 
> Something like this, totally untested...
> 
> diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
> index 68e7f19..5c1e663 100644
> --- a/drivers/ide/ide-cd.c
> +++ b/drivers/ide/ide-cd.c
> @@ -1308,13 +1308,29 @@ static int cdrom_read_capacity(ide_drive_t *drive, unsigned long *capacity,
>  	req.cmd_flags |= REQ_QUIET;
>  
>  	stat = ide_cd_queue_pc(drive, &req);
> -	if (stat == 0) {
> -		*capacity = 1 + be32_to_cpu(capbuf.lba);
> -		*sectors_per_frame =
> -			be32_to_cpu(capbuf.blocklen) >> SECTOR_BITS;
> +	if (stat)
> +		return stat;
> +
> +	/*
> +	 * Sanity check the given block size
> +	 */
> +	switch (capbuf.blocklen) {
> +	case 512:
> +	case 1024:
> +	case 2048:
> +	case 4096:
> +		break;
> +	default:
> +		printk(KERN_ERR "ide-cd: weird block size %u\n",
> +							capbuf.blocklen);
> +		printk(KERN_ERR "ide-cd: default to 2kb block size\n");
> +		capbuf.blocklen = 2048;
> +		break;
>  	}
>  
> -	return stat;
> +	*capacity = 1 + be32_to_cpu(capbuf.lba);
> +	*sectors_per_frame = be32_to_cpu(capbuf.blocklen) >> SECTOR_BITS;
> +	return 0;
>  }
>  
>  static int cdrom_read_tocentry(ide_drive_t *drive, int trackno, int msf_flag,
> 

This was all nearly a month ago and we missed 2.6.25.x and 2.6.26.

Jan has confirmed that the patch did fix the oops.  So I have put
toegether the below patch, which I will send in Bart's direction.  I
believe that Jens is offline at present.

Unfortunately the surrounding code has changed a bit in the current
post-2.6.26 mainline, but it is a small syntactic thing and the patch
can easily be backported.

I believe that the fix is needed in both 2.6.25.x and 2.6.26.x.


From: Jens Axboe <jens.axboe@oracle.com>

cdrom_read_capacity() will blindly return the capacity from the device
without sanity-checking it.  This later causes code in fs/buffer.c to
oops.

Fix this by checking that the device is telling us sensible things.

Cc: Michael Buesch <mb@bu3sch.de>
Cc: Jan Kara <jack@suse.cz>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/ide/ide-cd.c |   26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff -puN drivers/ide/ide-cd.c~oops-when-using-growisofs drivers/ide/ide-cd.c
--- a/drivers/ide/ide-cd.c~oops-when-using-growisofs
+++ a/drivers/ide/ide-cd.c
@@ -1309,13 +1309,29 @@ static int cdrom_read_capacity(ide_drive
 
 	stat = ide_cd_queue_pc(drive, cmd, 0, &capbuf, &len, sense, 0,
 			       REQ_QUIET);
-	if (stat == 0) {
-		*capacity = 1 + be32_to_cpu(capbuf.lba);
-		*sectors_per_frame =
-			be32_to_cpu(capbuf.blocklen) >> SECTOR_BITS;
+	if (stat)
+		return stat;
+
+	/*
+	 * Sanity check the given block size
+	 */
+	switch (capbuf.blocklen) {
+	case 512:
+	case 1024:
+	case 2048:
+	case 4096:
+		break;
+	default:
+		printk(KERN_ERR "ide-cd: weird block size %u\n",
+							capbuf.blocklen);
+		printk(KERN_ERR "ide-cd: default to 2kb block size\n");
+		capbuf.blocklen = 2048;
+		break;
 	}
 
-	return stat;
+	*capacity = 1 + be32_to_cpu(capbuf.lba);
+	*sectors_per_frame = be32_to_cpu(capbuf.blocklen) >> SECTOR_BITS;
+	return 0;
 }
 
 static int cdrom_read_tocentry(ide_drive_t *drive, int trackno, int msf_flag,
_


^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2008-07-22  9:26 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-06-22 16:18 Oops when using growisofs Michael Buesch
2008-06-22 21:22 ` Arnd Bergmann
2008-06-22 21:31   ` Arnd Bergmann
2008-06-22 22:09     ` Michael Buesch
2008-06-22 22:05   ` Michael Buesch
2008-06-22 22:28     ` Michael Buesch
2008-06-23  6:34       ` Andrew Morton
2008-06-23  6:59         ` Nick Piggin
2008-06-24 17:28         ` Jan Kara
2008-06-24 18:39           ` Michael Buesch
2008-06-25  1:42             ` Arnd Bergmann
2008-06-25  9:37             ` Jan Kara
2008-06-25  9:46               ` Michael Buesch
2008-06-26 17:05                 ` Jan Kara
2008-06-26 18:11                   ` Jens Axboe
2008-06-26 18:21                     ` Michael Buesch
2008-06-26 18:36                       ` Jens Axboe
2008-06-26 18:39                         ` Michael Buesch
2008-06-26 18:41                           ` Jens Axboe
2008-06-29 19:39                         ` Michael Buesch
2008-07-09 18:46                         ` Jan Kara
2008-07-22  9:25                         ` Andrew Morton

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox