cdrom_open() called check_disk_change() after the rest of open path
succeeded which leads to the following bizarre behavior.
* After media change, if the device opened without O_NONBLOCK,
open_for_data() naturally fails with -ENOMEDIA and
check_disk_change() is never called. The media is known to be gone
and the open failure makes it obvious to the userland but device
invalidation never happens.
* But if the device is opened with O_NONBLOCK, all the checks are
bypassed and cdrom_open() doesn't notice that the media is not there
and check_disk_change() is called and invalidation happens.
There's nothing to be gained by avoiding calling check_disk_change()
on open failure. Common cases end up calling check_disk_change()
anyway. All we get is inconsistent behavior.
Fix it by moving check_disk_change() invocation to the top of
cdrom_open() so that it always gets called regardless of how the rest
of open proceeds.
Signed-off-by: Tejun Heo <[email protected]>
Reported-by: Amit Shah <[email protected]>
Tested-by: Amit Shah <[email protected]>
---
drivers/cdrom/cdrom.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index e2c48a7..5ade78a 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -986,6 +986,9 @@ int cdrom_open(struct cdrom_device_info *cdi, struct block_device *bdev, fmode_t
cdinfo(CD_OPEN, "entering cdrom_open\n");
+ /* open is event synchronization point, check events first */
+ check_disk_change(bdev);
+
/* if this was a O_NONBLOCK open and we should honor the flags,
* do a quick open without drive/disc integrity checks. */
cdi->use_count++;
@@ -1012,9 +1015,6 @@ int cdrom_open(struct cdrom_device_info *cdi, struct block_device *bdev, fmode_t
cdinfo(CD_OPEN, "Use count for \"/dev/%s\" now %d\n",
cdi->name, cdi->use_count);
- /* Do this on open. Don't wait for mount, because they might
- not be mounting, but opening with O_NONBLOCK */
- check_disk_change(bdev);
return 0;
err_release:
if (CDROM_CAN(CDC_LOCK) && cdi->options & CDO_LOCK) {
__blkdev_get() doesn't rescan partitions if disk->fops->open() fails,
which leads to ghost partition devices lingering after medimum removal
is known to both the kernel and userland. The behavior also creates a
subtle inconsistency where O_NONBLOCK open, which doesn't fail even if
there's no medium, clears the ghots partitions, which is exploited to
work around the problem from userland.
Fix it by updating __blkdev_get() to issue partition rescan after
-ENOMEDIA too.
This was reported in the following bz.
https://bugzilla.kernel.org/show_bug.cgi?id=13029
Signed-off-by: Tejun Heo <[email protected]>
Reported-by: David Zeuthen <[email protected]>
Reported-by: Martin Pitt <[email protected]>
Reported-by: Kay Sievers <[email protected]>
Tested-by: Kay Sievers <[email protected]>
Cc: Alan Cox <[email protected]>
---
fs/block_dev.c | 27 ++++++++++++++++++---------
1 file changed, 18 insertions(+), 9 deletions(-)
diff --git a/fs/block_dev.c b/fs/block_dev.c
index c1511c6..a926ad4 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1102,6 +1102,7 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
if (!bdev->bd_part)
goto out_clear;
+ ret = 0;
if (disk->fops->open) {
ret = disk->fops->open(bdev, mode);
if (ret == -ERESTARTSYS) {
@@ -1118,9 +1119,18 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
put_disk(disk);
goto restart;
}
- if (ret)
- goto out_clear;
}
+ /*
+ * If the device is invalidated, rescan partition
+ * if open succeeded or failed with -ENOMEDIUM.
+ * The latter is necessary to prevent ghost
+ * partitions on a removed medium.
+ */
+ if (bdev->bd_invalidated && (!ret || ret == -ENOMEDIUM))
+ rescan_partitions(disk, bdev);
+ if (ret)
+ goto out_clear;
+
if (!bdev->bd_openers) {
bd_set_size(bdev,(loff_t)get_capacity(disk)<<9);
bdi = blk_get_backing_dev_info(bdev);
@@ -1128,8 +1138,6 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
bdi = &default_backing_dev_info;
bdev_inode_switch_bdi(bdev->bd_inode, bdi);
}
- if (bdev->bd_invalidated)
- rescan_partitions(disk, bdev);
} else {
struct block_device *whole;
whole = bdget_disk(disk, 0);
@@ -1153,13 +1161,14 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
}
} else {
if (bdev->bd_contains == bdev) {
- if (bdev->bd_disk->fops->open) {
+ ret = 0;
+ if (bdev->bd_disk->fops->open)
ret = bdev->bd_disk->fops->open(bdev, mode);
- if (ret)
- goto out_unlock_bdev;
- }
- if (bdev->bd_invalidated)
+ /* the same as first opener case, read comment there */
+ if (bdev->bd_invalidated && (!ret || ret == -ENOMEDIUM))
rescan_partitions(bdev->bd_disk, bdev);
+ if (ret)
+ goto out_unlock_bdev;
}
/* only one opener holds refs to the module and disk */
module_put(disk->fops->owner);
On (Wed) 06 Apr 2011 [05:20:09], Tejun Heo wrote:
> cdrom_open() called check_disk_change() after the rest of open path
> succeeded which leads to the following bizarre behavior.
>
> * After media change, if the device opened without O_NONBLOCK,
> open_for_data() naturally fails with -ENOMEDIA and
> check_disk_change() is never called. The media is known to be gone
> and the open failure makes it obvious to the userland but device
> invalidation never happens.
>
> * But if the device is opened with O_NONBLOCK, all the checks are
> bypassed and cdrom_open() doesn't notice that the media is not there
> and check_disk_change() is called and invalidation happens.
>
> There's nothing to be gained by avoiding calling check_disk_change()
> on open failure. Common cases end up calling check_disk_change()
> anyway. All we get is inconsistent behavior.
>
> Fix it by moving check_disk_change() invocation to the top of
> cdrom_open() so that it always gets called regardless of how the rest
> of open proceeds.
>
> Signed-off-by: Tejun Heo <[email protected]>
> Reported-by: Amit Shah <[email protected]>
> Tested-by: Amit Shah <[email protected]>
Ping?
Also, please mark this for stable-2.6.38.
Thanks,
Amit
On 2011-04-29 08:57, Amit Shah wrote:
> On (Wed) 06 Apr 2011 [05:20:09], Tejun Heo wrote:
>> cdrom_open() called check_disk_change() after the rest of open path
>> succeeded which leads to the following bizarre behavior.
>>
>> * After media change, if the device opened without O_NONBLOCK,
>> open_for_data() naturally fails with -ENOMEDIA and
>> check_disk_change() is never called. The media is known to be gone
>> and the open failure makes it obvious to the userland but device
>> invalidation never happens.
>>
>> * But if the device is opened with O_NONBLOCK, all the checks are
>> bypassed and cdrom_open() doesn't notice that the media is not there
>> and check_disk_change() is called and invalidation happens.
>>
>> There's nothing to be gained by avoiding calling check_disk_change()
>> on open failure. Common cases end up calling check_disk_change()
>> anyway. All we get is inconsistent behavior.
>>
>> Fix it by moving check_disk_change() invocation to the top of
>> cdrom_open() so that it always gets called regardless of how the rest
>> of open proceeds.
>>
>> Signed-off-by: Tejun Heo <[email protected]>
>> Reported-by: Amit Shah <[email protected]>
>> Tested-by: Amit Shah <[email protected]>
>
> Ping?
>
> Also, please mark this for stable-2.6.38.
Done, added for 2.6.39 and marked stable for 2.6.38.
--
Jens Axboe
On Fri, Apr 29, 2011 at 12:27:18PM +0530, Amit Shah wrote:
> On (Wed) 06 Apr 2011 [05:20:09], Tejun Heo wrote:
> > cdrom_open() called check_disk_change() after the rest of open path
> > succeeded which leads to the following bizarre behavior.
> >
> > * After media change, if the device opened without O_NONBLOCK,
> > open_for_data() naturally fails with -ENOMEDIA and
> > check_disk_change() is never called. The media is known to be gone
> > and the open failure makes it obvious to the userland but device
> > invalidation never happens.
> >
> > * But if the device is opened with O_NONBLOCK, all the checks are
> > bypassed and cdrom_open() doesn't notice that the media is not there
> > and check_disk_change() is called and invalidation happens.
> >
> > There's nothing to be gained by avoiding calling check_disk_change()
> > on open failure. Common cases end up calling check_disk_change()
> > anyway. All we get is inconsistent behavior.
> >
> > Fix it by moving check_disk_change() invocation to the top of
> > cdrom_open() so that it always gets called regardless of how the rest
> > of open proceeds.
> >
> > Signed-off-by: Tejun Heo <[email protected]>
> > Reported-by: Amit Shah <[email protected]>
> > Tested-by: Amit Shah <[email protected]>
>
> Ping?
>
> Also, please mark this for stable-2.6.38.
Jens?
--
tejun
On Fri, Apr 29, 2011 at 10:15:50AM +0200, Jens Axboe wrote:
> > Also, please mark this for stable-2.6.38.
>
> Done, added for 2.6.39 and marked stable for 2.6.38.
Heh, mid-air collision. Good morning, Jens. :)
--
tejun
On 2011-04-29 10:16, Tejun Heo wrote:
> On Fri, Apr 29, 2011 at 10:15:50AM +0200, Jens Axboe wrote:
>>> Also, please mark this for stable-2.6.38.
>>
>> Done, added for 2.6.39 and marked stable for 2.6.38.
>
> Heh, mid-air collision. Good morning, Jens. :)
Indeed :-)
--
Jens Axboe
On (Fri) 29 Apr 2011 [10:15:50], Jens Axboe wrote:
> On 2011-04-29 08:57, Amit Shah wrote:
> > On (Wed) 06 Apr 2011 [05:20:09], Tejun Heo wrote:
> >> cdrom_open() called check_disk_change() after the rest of open path
> >> succeeded which leads to the following bizarre behavior.
> >>
> >> * After media change, if the device opened without O_NONBLOCK,
> >> open_for_data() naturally fails with -ENOMEDIA and
> >> check_disk_change() is never called. The media is known to be gone
> >> and the open failure makes it obvious to the userland but device
> >> invalidation never happens.
> >>
> >> * But if the device is opened with O_NONBLOCK, all the checks are
> >> bypassed and cdrom_open() doesn't notice that the media is not there
> >> and check_disk_change() is called and invalidation happens.
> >>
> >> There's nothing to be gained by avoiding calling check_disk_change()
> >> on open failure. Common cases end up calling check_disk_change()
> >> anyway. All we get is inconsistent behavior.
> >>
> >> Fix it by moving check_disk_change() invocation to the top of
> >> cdrom_open() so that it always gets called regardless of how the rest
> >> of open proceeds.
> >>
> >> Signed-off-by: Tejun Heo <[email protected]>
> >> Reported-by: Amit Shah <[email protected]>
> >> Tested-by: Amit Shah <[email protected]>
> >
> > Ping?
> >
> > Also, please mark this for stable-2.6.38.
>
> Done, added for 2.6.39 and marked stable for 2.6.38.
Ping again. Don't see this yet in Linus's git tree.
Amit
On 2011-05-10 08:42, Amit Shah wrote:
> On (Fri) 29 Apr 2011 [10:15:50], Jens Axboe wrote:
>> On 2011-04-29 08:57, Amit Shah wrote:
>>> On (Wed) 06 Apr 2011 [05:20:09], Tejun Heo wrote:
>>>> cdrom_open() called check_disk_change() after the rest of open path
>>>> succeeded which leads to the following bizarre behavior.
>>>>
>>>> * After media change, if the device opened without O_NONBLOCK,
>>>> open_for_data() naturally fails with -ENOMEDIA and
>>>> check_disk_change() is never called. The media is known to be gone
>>>> and the open failure makes it obvious to the userland but device
>>>> invalidation never happens.
>>>>
>>>> * But if the device is opened with O_NONBLOCK, all the checks are
>>>> bypassed and cdrom_open() doesn't notice that the media is not there
>>>> and check_disk_change() is called and invalidation happens.
>>>>
>>>> There's nothing to be gained by avoiding calling check_disk_change()
>>>> on open failure. Common cases end up calling check_disk_change()
>>>> anyway. All we get is inconsistent behavior.
>>>>
>>>> Fix it by moving check_disk_change() invocation to the top of
>>>> cdrom_open() so that it always gets called regardless of how the rest
>>>> of open proceeds.
>>>>
>>>> Signed-off-by: Tejun Heo <[email protected]>
>>>> Reported-by: Amit Shah <[email protected]>
>>>> Tested-by: Amit Shah <[email protected]>
>>>
>>> Ping?
>>>
>>> Also, please mark this for stable-2.6.38.
>>
>> Done, added for 2.6.39 and marked stable for 2.6.38.
>
> Ping again. Don't see this yet in Linus's git tree.
It was added for 2.6.40 and marked for stable.
--
Jens Axboe
On (Tue) 10 May 2011 [09:44:28], Jens Axboe wrote:
> On 2011-05-10 08:42, Amit Shah wrote:
> > On (Fri) 29 Apr 2011 [10:15:50], Jens Axboe wrote:
> >> On 2011-04-29 08:57, Amit Shah wrote:
> >>> On (Wed) 06 Apr 2011 [05:20:09], Tejun Heo wrote:
> >>>> cdrom_open() called check_disk_change() after the rest of open path
> >>>> succeeded which leads to the following bizarre behavior.
> >>>>
> >>>> * After media change, if the device opened without O_NONBLOCK,
> >>>> open_for_data() naturally fails with -ENOMEDIA and
> >>>> check_disk_change() is never called. The media is known to be gone
> >>>> and the open failure makes it obvious to the userland but device
> >>>> invalidation never happens.
> >>>>
> >>>> * But if the device is opened with O_NONBLOCK, all the checks are
> >>>> bypassed and cdrom_open() doesn't notice that the media is not there
> >>>> and check_disk_change() is called and invalidation happens.
> >>>>
> >>>> There's nothing to be gained by avoiding calling check_disk_change()
> >>>> on open failure. Common cases end up calling check_disk_change()
> >>>> anyway. All we get is inconsistent behavior.
> >>>>
> >>>> Fix it by moving check_disk_change() invocation to the top of
> >>>> cdrom_open() so that it always gets called regardless of how the rest
> >>>> of open proceeds.
> >>>>
> >>>> Signed-off-by: Tejun Heo <[email protected]>
> >>>> Reported-by: Amit Shah <[email protected]>
> >>>> Tested-by: Amit Shah <[email protected]>
> >>>
> >>> Ping?
> >>>
> >>> Also, please mark this for stable-2.6.38.
> >>
> >> Done, added for 2.6.39 and marked stable for 2.6.38.
> >
> > Ping again. Don't see this yet in Linus's git tree.
>
> It was added for 2.6.40 and marked for stable.
OK, Thanks.
Amit