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
