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 <jeremy@xxxxxxxxxx>
>>
>> [ 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