[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-4.1] scsi-cd: Fix inserting read-only media
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH for-4.1] scsi-cd: Fix inserting read-only media in empty drive |
Date: |
Tue, 30 Jul 2019 12:09:13 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 |
On 30.07.19 10:29, Kevin Wolf wrote:
> Am 30.07.2019 um 08:31 hat Markus Armbruster geschrieben:
>> Kevin Wolf <address@hidden> writes:
>>
>>> scsi-disks decides whether it has a read-only device by looking at
>>> whether the BlockBackend specified as drive=... is read-only. In the
>>> case of an anonymous BlockBackend (with a node name specified in
>>> drive=...), this is the read-only flag of the attached node. In the case
>>> of an empty anonymous BlockBackend, it's always read-write because
>>> nothing prevented it from being read-write.
>>>
>>> This is a problem because scsi-cd would take write permissions on the
>>> anonymous BlockBackend of an empty drive created without a drive=...
>>> option. Using blockdev-insert-medium with a read-only node fails then
>>> with the error message "Block node is read-only".
>>>
>>> Fix scsi_realize() so that scsi-cd devices always take read-only
>>> permissions on their BlockBackend instead.
>>>
>>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1733920
>>> Signed-off-by: Kevin Wolf <address@hidden>
>>> ---
>>> hw/scsi/scsi-disk.c | 10 ++++++++--
>>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
>>> index 8e95e3e38d..af3e622dc5 100644
>>> --- a/hw/scsi/scsi-disk.c
>>> +++ b/hw/scsi/scsi-disk.c
>>> @@ -2318,6 +2318,7 @@ static void
>>> scsi_disk_unit_attention_reported(SCSIDevice *dev)
>>> static void scsi_realize(SCSIDevice *dev, Error **errp)
>>> {
>>> SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
>>> + bool read_only;
>>>
>>> if (!s->qdev.conf.blk) {
>>> error_setg(errp, "drive property not set");
>>> @@ -2351,8 +2352,13 @@ static void scsi_realize(SCSIDevice *dev, Error
>>> **errp)
>>> return;
>>> }
>>> }
>>> - if (!blkconf_apply_backend_options(&dev->conf,
>>> - blk_is_read_only(s->qdev.conf.blk),
>>> +
>>> + read_only = blk_is_read_only(s->qdev.conf.blk);
>>> + if (dev->type == TYPE_ROM) {
>>> + read_only = true;
>>> + }
>>> +
>>> + if (!blkconf_apply_backend_options(&dev->conf, read_only,
>>> dev->type == TYPE_DISK, errp)) {
>>> return;
>>> }
>>
>> For what it's worth, we have code similar to the one after this patch in
>>
>> * ide_dev_initfn()
>>
>> * xen_block_realize() (I guess)
>>
>> We have code similar to the one before this patch in
>>
>> * floppy_drive_realize()
>>
>> I figure we avoid the problem by recomputing read-only on media
>> change, in fd_change_cb(). Funny: looks like a medium's
>> read-only-ness lingers after unload until the next medium is loaded.
>
> We may try to, but it looks something is broken for floppies.
>
> The bug only came to my attention yesterday, so I haven't got a full
> test case yet, but the half that I already have fails for floppy. I'll
> look into this, but it was more important to me to get at least the
> scsi-cd fix into 4.1.
>
>> * nvme_realize()
>>
>> * virtio_blk_device_realize()
>>
>> * scsi_generic_realize()
>>
>> * usb_msd_storage_realize()
>>
>> Are these all okay? Should they work more like floppy? If not, what
>> makes floppy special?
>
> Most of them aren't relevant in this context because this is a problem
> with removable media, and most devices don't support that. So as far as
> I know all we need to check is floppy, ATAPI and SCSI CD-ROM.
>
> Floppy is special because it's the only read-write device that supports
> removable media (and you can insert a read-only floppy after ejecting a
> read-write one or vice versa). CDs can be simpler because they are
> always read-only.
There are also SD cards.
(The SD code just rejects read-only BBs, and it takes PERM_WRITE on it.
So I suppose it’s good because this way you simply can never insert
read-only nodes.)
Max
signature.asc
Description: OpenPGP digital signature