Re: [PATCH 4.4 128/268] scsi: sd: Keep disk read-only when re-reading partition
From: Jeremy Cline
Date: Thu Jun 14 2018 - 14:50:16 EST
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
> 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?