Re: [PATCH 4.4 128/268] scsi: sd: Keep disk read-only when re-reading partition

[Martin K. Petersen]

Hi Ben,

> (The log message about Write Protect status also reports the
> underlying SCSI device flag and not the combined ro flag, but maybe
> that was intentional.)

I'd prefer for the printk in question to reflect the device-reported
state, not the state of the block device.

> I think this commit should be reverted, both in stable and upstream.
> A proper fix would involve splitting the ro flag into two flags—one
> controlled by user-space and one read from the device—with the
> effective read-only status being the logical-or of those two.

I don't have a problem with distinguishing between current state and an
override flag in the block layer. However, I think an incremental patch
to fix that up is fine. SCSI devices don't typically switch write
protection state on a whim.

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH 4.4 128/268] scsi: sd: Keep disk read-only when re-reading partition

[Martin K. Petersen]

Hi Ben,

> (The log message about Write Protect status also reports the
> underlying SCSI device flag and not the combined ro flag, but maybe
> that was intentional.)

I'd prefer for the printk in question to reflect the device-reported
state, not the state of the block device.

> I think this commit should be reverted, both in stable and upstream.
> A proper fix would involve splitting the ro flag into two flags—one
> controlled by user-space and one read from the device—with the
> effective read-only status being the logical-or of those two.

I don't have a problem with distinguishing between current state and an
override flag in the block layer. However, I think an incremental patch
to fix that up is fine. SCSI devices don't typically switch write
protection state on a whim.

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH 4.4 128/268] scsi: sd: Keep disk read-only when re-reading partition

[Jeremy Cline]

Hi Ben,

On 06/13/2018 05:25 PM, Ben Hutchings wrote:
> On Mon, 2018-05-28 at 12:01 +0200, Greg Kroah-Hartman wrote:
>> 4.4-stable review patch.  If anyone has any objections, please let me know.
>>
>> --
>>
>> From: Jeremy Cline 
>>
>> [ Upstream commit 20bd1d026aacc5399464f8328f305985c493cde3 ]
>>
>> If the read-only flag is true on a SCSI disk, re-reading the partition
>> table sets the flag back to false.
>>
>> To observe this bug, you can run:
>>
>> 1. blockdev --setro /dev/sda
>> 2. blockdev --rereadpt /dev/sda
>> 3. blockdev --getro /dev/sda
>>
>> This commit reads the disk's old state and combines it with the device
>> disk-reported state rather than unconditionally marking it as RW.
> 
> It seems to me that this change is likely to cause a regression: if a
> SCSI device switches from read-only to read-write state then a
> subsequent rescan won't automatically change the block device to read-
> write state.  The administrator will have to use the blockdev command
> too.
> 
> Even if this change in behaviour is acceptable, this commit does not
> implement it consistently.  The function starts by clearing the ro flag
> and this commit only changes one of the three exit paths to preserve
> it.  (The log message about Write Protect status also reports the
> underlying SCSI device flag and not the combined ro flag, but maybe
> that was intentional.)

Yes, it looks like I messed this up.

> 
> I think this commit should be reverted, both in stable and upstream.  A
> proper fix would involve splitting the ro flag into two flags—one
> controlled by user-space and one read from the device—with the
> effective read-only status being the logical-or of those two.

This seems sensible to me, for whatever that's worth. I'm new to the
kernel so I'm not certain I'll produce a reasonable patch, but I'm
willing to give it a shot.

So the change would be something like:

1. Add a flag to the gendisk struct to indicate if the device is
   read-only. The driver sets this.

2. In hd_struct struct's policy flag is one that indicates the
   user-space setting and this gets set by BLKROSET ioctl/set_disk_ro.

3. The effective read-only status reported by the BLKROGET ioctl and
   bdev_read_only is the logical-or of the hd_struct policy flag and the
   gendisk device flag.

Maybe that doesn't make sense at all, though. Thoughts?


Thanks,
Jeremy

Re: [PATCH 4.4 128/268] scsi: sd: Keep disk read-only when re-reading partition

[Jeremy Cline]

Hi Ben,

On 06/13/2018 05:25 PM, Ben Hutchings wrote:
> On Mon, 2018-05-28 at 12:01 +0200, Greg Kroah-Hartman wrote:
>> 4.4-stable review patch.  If anyone has any objections, please let me know.
>>
>> --
>>
>> From: Jeremy Cline 
>>
>> [ Upstream commit 20bd1d026aacc5399464f8328f305985c493cde3 ]
>>
>> If the read-only flag is true on a SCSI disk, re-reading the partition
>> table sets the flag back to false.
>>
>> To observe this bug, you can run:
>>
>> 1. blockdev --setro /dev/sda
>> 2. blockdev --rereadpt /dev/sda
>> 3. blockdev --getro /dev/sda
>>
>> This commit reads the disk's old state and combines it with the device
>> disk-reported state rather than unconditionally marking it as RW.
> 
> It seems to me that this change is likely to cause a regression: if a
> SCSI device switches from read-only to read-write state then a
> subsequent rescan won't automatically change the block device to read-
> write state.  The administrator will have to use the blockdev command
> too.
> 
> Even if this change in behaviour is acceptable, this commit does not
> implement it consistently.  The function starts by clearing the ro flag
> and this commit only changes one of the three exit paths to preserve
> it.  (The log message about Write Protect status also reports the
> underlying SCSI device flag and not the combined ro flag, but maybe
> that was intentional.)

Yes, it looks like I messed this up.

> 
> I think this commit should be reverted, both in stable and upstream.  A
> proper fix would involve splitting the ro flag into two flags—one
> controlled by user-space and one read from the device—with the
> effective read-only status being the logical-or of those two.

This seems sensible to me, for whatever that's worth. I'm new to the
kernel so I'm not certain I'll produce a reasonable patch, but I'm
willing to give it a shot.

So the change would be something like:

1. Add a flag to the gendisk struct to indicate if the device is
   read-only. The driver sets this.

2. In hd_struct struct's policy flag is one that indicates the
   user-space setting and this gets set by BLKROSET ioctl/set_disk_ro.

3. The effective read-only status reported by the BLKROGET ioctl and
   bdev_read_only is the logical-or of the hd_struct policy flag and the
   gendisk device flag.

Maybe that doesn't make sense at all, though. Thoughts?


Thanks,
Jeremy

Re: [PATCH 4.4 128/268] scsi: sd: Keep disk read-only when re-reading partition

[Ben Hutchings]

On Mon, 2018-05-28 at 12:01 +0200, Greg Kroah-Hartman wrote:
> 4.4-stable review patch.  If anyone has any objections, please let me know.
> 
> --
> 
> From: Jeremy Cline 
> 
> [ Upstream commit 20bd1d026aacc5399464f8328f305985c493cde3 ]
> 
> If the read-only flag is true on a SCSI disk, re-reading the partition
> table sets the flag back to false.
> 
> To observe this bug, you can run:
> 
> 1. blockdev --setro /dev/sda
> 2. blockdev --rereadpt /dev/sda
> 3. blockdev --getro /dev/sda
> 
> This commit reads the disk's old state and combines it with the device
> disk-reported state rather than unconditionally marking it as RW.

It seems to me that this change is likely to cause a regression: if a
SCSI device switches from read-only to read-write state then a
subsequent rescan won't automatically change the block device to read-
write state.  The administrator will have to use the blockdev command
too.

Even if this change in behaviour is acceptable, this commit does not
implement it consistently.  The function starts by clearing the ro flag
and this commit only changes one of the three exit paths to preserve
it.  (The log message about Write Protect status also reports the
underlying SCSI device flag and not the combined ro flag, but maybe
that was intentional.)

I think this commit should be reverted, both in stable and upstream.  A
proper fix would involve splitting the ro flag into two flags—one
controlled by user-space and one read from the device—with the
effective read-only status being the logical-or of those two.

Ben.

> Reported-by: Li Ning 
> Signed-off-by: Jeremy Cline 
> Signed-off-by: Martin K. Petersen 
> Signed-off-by: Sasha Levin 
> Signed-off-by: Greg Kroah-Hartman 
> ---
>  drivers/scsi/sd.c |3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2395,6 +2395,7 @@ sd_read_write_protect_flag(struct scsi_d
>   int res;
>   struct scsi_device *sdp = sdkp->device;
>   struct scsi_mode_data data;
> + int disk_ro = get_disk_ro(sdkp->disk);
>   int old_wp = sdkp->write_prot;
>  
>   set_disk_ro(sdkp->disk, 0);
> @@ -2435,7 +2436,7 @@ sd_read_write_protect_flag(struct scsi_d
>     "Test WP failed, assume Write Enabled\n");
>   } else {
>   sdkp->write_prot = ((data.device_specific & 0x80) != 0);
> - set_disk_ro(sdkp->disk, sdkp->write_prot);
> + set_disk_ro(sdkp->disk, sdkp->write_prot || disk_ro);
>   if (sdkp->first_scan || old_wp != sdkp->write_prot) {
>   sd_printk(KERN_NOTICE, sdkp, "Write Protect is %s\n",
>     sdkp->write_prot ? "on" : "off");

-- 
Ben Hutchings, Software Developer Codethink Ltd
https://www.codethink.co.uk/ Dale House, 35 Dale Street
 Manchester, M1 2HF, United Kingdom

Re: [PATCH 4.4 128/268] scsi: sd: Keep disk read-only when re-reading partition

[Ben Hutchings]

On Mon, 2018-05-28 at 12:01 +0200, Greg Kroah-Hartman wrote:
> 4.4-stable review patch.  If anyone has any objections, please let me know.
> 
> --
> 
> From: Jeremy Cline 
> 
> [ Upstream commit 20bd1d026aacc5399464f8328f305985c493cde3 ]
> 
> If the read-only flag is true on a SCSI disk, re-reading the partition
> table sets the flag back to false.
> 
> To observe this bug, you can run:
> 
> 1. blockdev --setro /dev/sda
> 2. blockdev --rereadpt /dev/sda
> 3. blockdev --getro /dev/sda
> 
> This commit reads the disk's old state and combines it with the device
> disk-reported state rather than unconditionally marking it as RW.

It seems to me that this change is likely to cause a regression: if a
SCSI device switches from read-only to read-write state then a
subsequent rescan won't automatically change the block device to read-
write state.  The administrator will have to use the blockdev command
too.

Even if this change in behaviour is acceptable, this commit does not
implement it consistently.  The function starts by clearing the ro flag
and this commit only changes one of the three exit paths to preserve
it.  (The log message about Write Protect status also reports the
underlying SCSI device flag and not the combined ro flag, but maybe
that was intentional.)

I think this commit should be reverted, both in stable and upstream.  A
proper fix would involve splitting the ro flag into two flags—one
controlled by user-space and one read from the device—with the
effective read-only status being the logical-or of those two.

Ben.

> Reported-by: Li Ning 
> Signed-off-by: Jeremy Cline 
> Signed-off-by: Martin K. Petersen 
> Signed-off-by: Sasha Levin 
> Signed-off-by: Greg Kroah-Hartman 
> ---
>  drivers/scsi/sd.c |3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2395,6 +2395,7 @@ sd_read_write_protect_flag(struct scsi_d
>   int res;
>   struct scsi_device *sdp = sdkp->device;
>   struct scsi_mode_data data;
> + int disk_ro = get_disk_ro(sdkp->disk);
>   int old_wp = sdkp->write_prot;
>  
>   set_disk_ro(sdkp->disk, 0);
> @@ -2435,7 +2436,7 @@ sd_read_write_protect_flag(struct scsi_d
>     "Test WP failed, assume Write Enabled\n");
>   } else {
>   sdkp->write_prot = ((data.device_specific & 0x80) != 0);
> - set_disk_ro(sdkp->disk, sdkp->write_prot);
> + set_disk_ro(sdkp->disk, sdkp->write_prot || disk_ro);
>   if (sdkp->first_scan || old_wp != sdkp->write_prot) {
>   sd_printk(KERN_NOTICE, sdkp, "Write Protect is %s\n",
>     sdkp->write_prot ? "on" : "off");

-- 
Ben Hutchings, Software Developer Codethink Ltd
https://www.codethink.co.uk/ Dale House, 35 Dale Street
 Manchester, M1 2HF, United Kingdom

[PATCH 4.4 128/268] scsi: sd: Keep disk read-only when re-reading partition

[Greg Kroah-Hartman]

4.4-stable review patch.  If anyone has any objections, please let me know.

--

From: Jeremy Cline 

[ Upstream commit 20bd1d026aacc5399464f8328f305985c493cde3 ]

If the read-only flag is true on a SCSI disk, re-reading the partition
table sets the flag back to false.

To observe this bug, you can run:

1. blockdev --setro /dev/sda
2. blockdev --rereadpt /dev/sda
3. blockdev --getro /dev/sda

This commit reads the disk's old state and combines it with the device
disk-reported state rather than unconditionally marking it as RW.

Reported-by: Li Ning 
Signed-off-by: Jeremy Cline 
Signed-off-by: Martin K. Petersen 
Signed-off-by: Sasha Levin 
Signed-off-by: Greg Kroah-Hartman 
---
 drivers/scsi/sd.c |3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2395,6 +2395,7 @@ sd_read_write_protect_flag(struct scsi_d
int res;
struct scsi_device *sdp = sdkp->device;
struct scsi_mode_data data;
+   int disk_ro = get_disk_ro(sdkp->disk);
int old_wp = sdkp->write_prot;
 
set_disk_ro(sdkp->disk, 0);
@@ -2435,7 +2436,7 @@ sd_read_write_protect_flag(struct scsi_d
  "Test WP failed, assume Write Enabled\n");
} else {
sdkp->write_prot = ((data.device_specific & 0x80) != 0);
-   set_disk_ro(sdkp->disk, sdkp->write_prot);
+   set_disk_ro(sdkp->disk, sdkp->write_prot || disk_ro);
if (sdkp->first_scan || old_wp != sdkp->write_prot) {
sd_printk(KERN_NOTICE, sdkp, "Write Protect is %s\n",
  sdkp->write_prot ? "on" : "off");

[PATCH 4.4 128/268] scsi: sd: Keep disk read-only when re-reading partition

[Greg Kroah-Hartman]

4.4-stable review patch.  If anyone has any objections, please let me know.

--

From: Jeremy Cline 

[ Upstream commit 20bd1d026aacc5399464f8328f305985c493cde3 ]

If the read-only flag is true on a SCSI disk, re-reading the partition
table sets the flag back to false.

To observe this bug, you can run:

1. blockdev --setro /dev/sda
2. blockdev --rereadpt /dev/sda
3. blockdev --getro /dev/sda

This commit reads the disk's old state and combines it with the device
disk-reported state rather than unconditionally marking it as RW.

Reported-by: Li Ning 
Signed-off-by: Jeremy Cline 
Signed-off-by: Martin K. Petersen 
Signed-off-by: Sasha Levin 
Signed-off-by: Greg Kroah-Hartman 
---
 drivers/scsi/sd.c |3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2395,6 +2395,7 @@ sd_read_write_protect_flag(struct scsi_d
int res;
struct scsi_device *sdp = sdkp->device;
struct scsi_mode_data data;
+   int disk_ro = get_disk_ro(sdkp->disk);
int old_wp = sdkp->write_prot;
 
set_disk_ro(sdkp->disk, 0);
@@ -2435,7 +2436,7 @@ sd_read_write_protect_flag(struct scsi_d
  "Test WP failed, assume Write Enabled\n");
} else {
sdkp->write_prot = ((data.device_specific & 0x80) != 0);
-   set_disk_ro(sdkp->disk, sdkp->write_prot);
+   set_disk_ro(sdkp->disk, sdkp->write_prot || disk_ro);
if (sdkp->first_scan || old_wp != sdkp->write_prot) {
sd_printk(KERN_NOTICE, sdkp, "Write Protect is %s\n",
  sdkp->write_prot ? "on" : "off");