Max Reitz <mre...@redhat.com> writes: > On 30.07.19 10:29, Kevin Wolf wrote: >> Am 30.07.2019 um 08:31 hat Markus Armbruster geschrieben: >>> Kevin Wolf <kw...@redhat.com> 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 <kw...@redhat.com> >>>> --- >>>> 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.
Ha! Cc: John. >> 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. I think this patch is okay as a narrow fix for scsi-cd, thus Reviewed-by: Markus Armbruster <arm...@redhat.com> >>> * 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.) I'd prefer to call this "differently broken" :)
