Messages in this thread | | | Date | Sun, 30 Nov 2008 15:53:36 +0000 | From | Al Viro <> | Subject | Re: [PATCH] fix pktcdvd breakage from commit e105b8bfc769b0545b6f0f395179d1e43cbee822 |
| |
On Sun, Nov 30, 2008 at 02:52:21PM +0000, Al Viro wrote: > On Sun, Nov 30, 2008 at 03:28:15PM +0100, Kay Sievers wrote: > > > So we need to preserve the layout, with the easiest way probably being "add > > > one more ktype and use kobject_init_and_add() instead of that device_create()". > > > Sigh... > > > > What do you mean? We just need to replace the bogus "pd->pkt_dev" with > > MKDEV(0, 0) and we are fine. > > Userland-visible change - right now cat /sys/class/pktcdvd/pktcdvd3/dev will > give you dev_t of the block device in question.
Looking at the locking scheme there, it appears that we have an unpleasant race of the same kind as discussed in md-with-eviction thread. If removal hits between finding (and grabbing) gendisk and actual call of ->open(), it will succeed just fine and we might get an open for _different_ object, while holding a reference to disk that had already gone through del_gendisk(). Call ioctl() on that sucker and you've got struct pktcdvd_device *pd = bdev->bd_disk->private_data; which will point to already freed object.
It really appears that we need to * add ->use() and ->unuse() callbacks * have exact_lock() call ->use() in addition to get_disk() - either on each hit, or on the "it's currently not in use" ones. * have ->unuse() called on failure exits in __blkdev_get() and in blkdev_put(), with rules matching those for calls of ->use().
That would allow to close that kind of races, both here and in md. I'd probably prefer the second variant for calling rules - we would be able to bracket the "it's associated with underlying objects" intervals by those calls... Hell knows; I really need to get some sleep and look through the ->open() and ->release() instances for block devices. Later...
| |