Bug 11401

Summary: pktcdvd: BUG, NULL pointer dereference in pkt_ioctl, bisected
Product: Drivers Reporter: Laurent Riffard (laurent.riffard)
Component: OtherAssignee: drivers_other
Status: CLOSED CODE_FIX    
Severity: normal CC: axboe, bunk, rjw
Priority: P1    
Hardware: All   
OS: Linux   
Kernel Version: 2.6.27-rc4 Subsystem:
Regression: Yes Bisected commit-id:
Bug Depends on:    
Bug Blocks: 11167    
Attachments: Full dmesg
.config

Description Laurent Riffard 2008-08-22 08:16:44 UTC
Latest working kernel version: 2.6.26

Earliest failing kernel version: 2.6.27-rc1

Steps to reproduce:

# pktsetup dvd /dev/dvdrw1
pktcdvd: writer pktcdvd0 mapped to hdc

(insert an UDF-formatted DVD-RW)
# mount -tauto /dev/pktcdvd/dvd /media/pkt

pktcdvd: Fixed packets, 16 blocks, Mode-1 disc
pktcdvd: write speed 2770kB/s
pktcdvd: 4595774kB available on disc
BUG: unable to handle kernel NULL pointer dereference at 0000000c
IP: [<e1cd15be>] :pktcdvd:pkt_ioctl+0xa/0x97
*pde = 00000000 
Oops: 0000 [#1] PREEMPT 
Modules linked in: udf pktcdvd radeon drm lp nls_iso8859_1 nls_cp850 vfat fat reiserfs ipv6 eeprom w83781d hwmon_vid usblp snd_ens1371 gameport snd_ac97_codec af_packet ac97_bus snd_pcm_oss snd_mixer_oss snd_pcm snd_seq_oss snd_seq_midi snd_rawmidi sr_mod snd_seq_midi_event cdrom sg snd_seq snd_timer firewire_ohci snd_seq_device firewire_core snd crc_itu_t via686a ata_generic parport_pc soundcore uhci_hcd via_agp snd_page_alloc i2c_viapro parport agpgart ohci1394 usbcore ieee1394 evdev rtc pcspkr floppy dm_snapshot reiser4 lzo_decompress lzo_compress sd_mod pata_via libata scsi_mod dock dm_mirror dm_log dm_mod

Pid: 4310, comm: mount Not tainted (2.6.27-rc4-00022-g7a10e7d #65)
EIP: 0060:[<e1cd15be>] EFLAGS: 00010286 CPU: 0
EIP is at pkt_ioctl+0xa/0x97 [pktcdvd]
EAX: 00000000 EBX: e1cd15b4 ECX: de910dcc EDX: 00005310
ESI: 00005310 EDI: 00000000 EBP: de910dcc ESP: de910c90
 DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068
Process mount (pid: 4310, ti=de910000 task=dfbeadc0 task.ti=de910000)
Stack: e1cd15b4 dfaae000 00000000 de910dcc c01bd38a dba2cdb4 00005310 fffffdfd 
       00000000 dba2cd00 c01bdaeb 00005310 de910dcc dba2cd0c c028e865 dba2cd00 
       00000000 00005310 00000000 dba2cdb4 dfaae000 dba2cd0c e1cd7000 00000000 
Call Trace:
 [<e1cd15b4>] pkt_ioctl+0x0/0x97 [pktcdvd]
 [<c01bd38a>] blkdev_driver_ioctl+0x26/0x5b
 [<c01bdaeb>] blkdev_ioctl+0x72c/0x74d
 [<c028e865>] __mutex_unlock_slowpath+0xdf/0xf1
 [<c017b786>] __blkdev_get+0x63/0x6e
 [<c01c70ed>] string+0x27/0x6d
 [<c01c73ae>] vsnprintf+0x27b/0x43e
 [<c01588b6>] cpu_cache_get+0x8/0xd
 [<c01593e5>] ____cache_alloc+0x14/0x469
 [<c017bb10>] ioctl_by_bdev+0x23/0x32
 [<e1e1b6f8>] udf_get_last_session+0x1a/0x32 [udf]
 [<e1e1f97e>] udf_fill_super+0x162/0x7a3 [udf]
 [<c01588b6>] cpu_cache_get+0x8/0xd
 [<c018b918>] disk_name+0x1f/0x5b
 [<c01cffff>] pci_read_config+0x9d/0x1a6
 [<c015e0ce>] get_sb_bdev+0xc9/0x10b
 [<e1e1dffb>] udf_get_sb+0x12/0x16 [udf]
 [<e1e1f81c>] udf_fill_super+0x0/0x7a3 [udf]
 [<c015db6c>] vfs_kern_mount+0x39/0x72
 [<c015de71>] do_kern_mount+0x2f/0xad
 [<c016e1ed>] do_new_mount+0x55/0x89
 [<c016f143>] do_mount+0x17c/0x19b
 [<c0145628>] alloc_pages_node+0x1a/0x1d
 [<c014565f>] __get_free_pages+0xb/0x26
 [<c016da86>] copy_mount_options+0x26/0x10d
 [<c016f5f0>] sys_mount+0x6d/0xaa
 [<c01038cd>] sysenter_do_call+0x12/0x35
 =======================
Code: a6 78 cd ff c6 44 24 04 1e 85 f6 58 89 f8 89 e2 0f 95 44 24 04 e8 3a fd ff ff 83 c4 2c 5b 5e 5f c3 55 89 cd 57 89 c7 56 89 d6 53 <8b> 40 0c 8b 58 24 e8 6b e3 5b de 81 fe 09 53 00 00 8b 83 b4 01 
EIP: [<e1cd15be>] pkt_ioctl+0xa/0x97 [pktcdvd] SS:ESP 0068:de910c90
---[ end trace 0986c4dfec814e67 ]---


git-bisect said:

5b6155ee70e9c4d2ad7e6f514c8eee06e2711c3a is first bad commit
commit 5b6155ee70e9c4d2ad7e6f514c8eee06e2711c3a
Author: Alan Cox <alan@lxorguk.ukuu.org.uk>
Date:   Fri Jul 4 09:29:16 2008 +0200

    pktcdvd: push BKL down into driver
    
    Push the lock_kernel down into the driver and switch to unlocked_ioctl
    
    [akpm@linux-foundation.org: build fix]
    Signed-off-by: Alan Cox <alan@redhat.com>
    Acked-by: Peter Osterlund <petero2@telia.com>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Jens Axboe <jens.axboe@oracle.com>

:040000 040000 0c7aeb48357bd31ac8f115f89106f5182e25c3ee b8363c3541c4d00a83ab6b28657dabcef890a7dc M	drivers


Indeed, udf_get_last_session calls ioctl_by_bdev, which calls blkdev_ioctl with a NULL file. As a result, pkt_ioctl is called with a NULL file.
Comment 1 Laurent Riffard 2008-08-22 08:17:40 UTC
Created attachment 17372 [details]
Full dmesg
Comment 2 Laurent Riffard 2008-08-22 08:18:37 UTC
Created attachment 17373 [details]
.config
Comment 3 Alan 2008-08-22 08:46:21 UTC
Yes blkdev_ioctl is buggy and needs updating for a whole load of ioctls
Comment 4 Rafael J. Wysocki 2008-08-24 14:14:22 UTC
On Sunday, 24 of August 2008, Linus Torvalds wrote:
> 
> On Sat, 23 Aug 2008, Rafael J. Wysocki wrote:
> > 
> > Bug-Entry   : http://bugzilla.kernel.org/show_bug.cgi?id=11401
> > Subject             : pktcdvd: BUG, NULL pointer dereference in pkt_ioctl,
> bisected
> > Submitter   : Laurent Riffard <laurent.riffard@free.fr>
> > Date                : 2008-08-22 08:16 (2 days old)
> 
> This one looks irritating.
> 
> It's bisected to 5b6155ee70e9c4d2ad7e6f514c8eee06e2711c3a ("pktcdvd: push 
> BKL down into driver"), but the problem goes deeper than that.
> 
> The "unlocked" ioctl's do not get a "struct inode *" pointer, they _only_ 
> get the "struct file *". And this is very much historical usage, where 
> some internal functions only passed in the inode (good or not, whatever).
> 
> And ioctl_by_bdev() doesn't have a "struct file *" and has depended on 
> passing in a NUMM "struct file *" and its own "struct inode *", and 
> expects the ioctl's to just use that instead. But the unlocked ioctl just 
> drops it on the floor, and uses just the (unusable) file pointer.
> 
> Grr.
> 
> And some other cases (like pkt_ioctl() itself) that simply pass in a 
> _different_ inode than the file itself is attached to. It does
> 
>       blkdev_ioctl(pd->bdev->bd_inode, file, cmd, arg);
> 
> where "file" points to the pkt_ioctl thing, but "inode" points to the 
> inode "behind" the pkt interface.
> 
> Double grr.
> 
> I really think the only sane model is to literally make "unlocked_ioctl()" 
> have the same calling convention as the old "ioctl()" thing had, and pass 
> in both file * and inode *. It was a stupid "cleanup" to try to have a 
> simpler interface for the unlocked version. Having two different models, 
> where we have actually _depended_ on the old model and then are trying to 
> convert to a (weaker) new model, is not a good idea.
> 
> The alternative is to do this _only_ for the blkdev_ioctl's, and have 
> those only take the "inode *", and then create a new fake "struct file *" 
> to go with it, regardless of what "struct file" was passed in (exactly 
> because the blockdev ones really think that the inode is the important 
> part).
> 
> Hmm?
> 
> We need to fix this.
Comment 5 Adrian Bunk 2008-08-27 15:46:45 UTC
fixed by commit 8560c650f340565b720fd57d1f9c99ab216d99d0