hw/scsi/scsi-disk.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
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 <kwolf@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;
}
--
2.20.1
Kevin Wolf <kwolf@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 <kwolf@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.
* 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?
Am 30.07.2019 um 08:31 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@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 <kwolf@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.
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.
Kevin
On 30.07.19 10:29, Kevin Wolf wrote:
> Am 30.07.2019 um 08:31 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@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 <kwolf@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.
>
> 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
Max Reitz <mreitz@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 <kwolf@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 <kwolf@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 <armbru@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" :)
Kevin Wolf 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 <kwolf@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;
> + }
Is there a reason to check blk_is_read_only even for CD-ROM?
If not, why not make it a "else"?
> +
> + if (!blkconf_apply_backend_options(&dev->conf, read_only,
> dev->type == TYPE_DISK, errp)) {
> return;
> }
--
Cheers,
Christophe de Dinechin (IRC c3d)
On 29.07.19 18:42, Kevin Wolf wrote: > 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 <kwolf@redhat.com> > --- > hw/scsi/scsi-disk.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) Reviewed-by: Max Reitz <mreitz@redhat.com>
On 7/29/19 6:42 PM, Kevin Wolf wrote:
> 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 <kwolf@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;
> }
>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
© 2016 - 2025 Red Hat, Inc.