lkml.org 
[lkml]   [2018]   [Jun]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 4.4 128/268] scsi: sd: Keep disk read-only when re-reading partition
From
Date
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@jcline.org>
>>
>> [ 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

\
 
 \ /
  Last update: 2018-06-14 20:50    [W:0.269 / U:0.152 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site