split hard read-only vs read-only policy

split hard read-only vs read-only policy

[Christoph Hellwig]

Hi Jens,

this series resurrects a patch from Martin to properly split the flag
indicating a disk has been set read-only by the hardware vs the userspace
policy set through the BLKROSET ioctl.

[PATCH 1/3] block: Fix read-only block device setting after revalidate

[Christoph Hellwig]

From: "Martin K. Petersen" <martin.petersen@oracle.com>

Commit 20bd1d026aac ("scsi: sd: Keep disk read-only when re-reading
partition") addressed a long-standing problem with user read-only
policy being overridden as a result of a device-initiated revalidate.
The commit has since been reverted due to a regression that left some
USB devices read-only indefinitely.

To fix the underlying problems with revalidate we need to keep track
of hardware state and user policy separately. Every time the state is
changed, either via a hardware event or the BLKROSET ioctl, the
per-partition read-only state is updated based on the combination of
device state and policy. The resulting active state is stored in a
separate hd_struct flag to avoid introducing additional lookups in the
I/O hot path.

The gendisk has been updated to reflect the current hardware state set
by the device driver. This is done to allow returning the device to
the hardware state once the user clears the BLKROSET flag.

For partitions, the existing hd_struct 'policy' flag is replaced with a
new HD_RO_POLICY flag in a new state variable and changed to only
indicate whether the user has administratively set partition read-only
via the BLKROSET ioctl.

The bdev_read_only helper and its open coded variants also check the
whole device administratively policy as well as the hardware state.

The resulting semantics are as follows:

 - If BLKROSET is used to set a whole-disk device read-only, any
   partitions will end up in a read-only state until the user
   explicitly clears the flag.

 - If BLKROSET sets a given partition read-only, that partition will
   remain read-only even if the underlying storage stack initiates a
   revalidate. However, the BLKRRPART ioctl will cause the partition
   table to be dropped and any user policy on partitions will be lost.

 - If BLKROSET has not been set, both the whole disk device and any
   partitions will reflect the current write-protect state of the
   underlying device.

Reported-by: Oleksii Kurochko <olkuroch@cisco.com>
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=201221
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
[hch: rebased.  don't mirror the compound read-only flag into a field,
	use flag instead of bools]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-core.c        |  4 ++-
 block/blk.h             |  3 +++
 block/genhd.c           | 59 ++++++++++++++++++++++++++++++++---------
 block/ioctl.c           |  5 +++-
 block/partitions/core.c |  7 +++--
 include/linux/genhd.h   | 10 ++++---
 6 files changed, 69 insertions(+), 19 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 2db8bda43b6e6d..b6dea07e08c9c7 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -695,7 +695,9 @@ static inline bool bio_check_ro(struct bio *bio, struct hd_struct *part)
 {
 	const int op = bio_op(bio);
 
-	if (part->policy && op_is_write(op)) {
+	if (op_is_write(op) &&
+	    (test_bit(GD_READ_ONLY, &bio->bi_disk->state) ||
+	     test_bit(HD_RO_POLICY, &part->state))) {
 		char b[BDEVNAME_SIZE];
 
 		if (op_is_flush(bio->bi_opf) && !bio_sectors(bio))
diff --git a/block/blk.h b/block/blk.h
index dfab98465db9a5..446776853cedd9 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -444,4 +444,7 @@ int bio_add_hw_page(struct request_queue *q, struct bio *bio,
 		struct page *page, unsigned int len, unsigned int offset,
 		unsigned int max_sectors, bool *same_page);
 
+void update_part_ro_state(struct hd_struct *part, bool read_only);
+void update_all_part_ro_state(struct gendisk *disk, bool read_only);
+
 #endif /* BLK_INTERNAL_H */
diff --git a/block/genhd.c b/block/genhd.c
index 15b90e56a1a6ea..65f6b744f4ebd5 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1835,31 +1835,66 @@ static void set_disk_ro_uevent(struct gendisk *gd, int ro)
 	kobject_uevent_env(&disk_to_dev(gd)->kobj, KOBJ_CHANGE, envp);
 }
 
-void set_disk_ro(struct gendisk *disk, int flag)
+void update_part_ro_state(struct hd_struct *part, bool read_only)
 {
-	struct disk_part_iter piter;
-	struct hd_struct *part;
+	if (read_only)
+		set_bit(HD_RO_POLICY, &part->state);
+	else
+		clear_bit(HD_RO_POLICY, &part->state);
+}
 
-	if (disk->part0.policy != flag) {
-		set_disk_ro_uevent(disk, flag);
-		disk->part0.policy = flag;
-	}
+/**
+ * update_all_part_ro_state - iterate over partitions to update read-only state
+ * @disk:	The disk device
+ *
+ * This function updates the read-only state for all partitions on a
+ * given disk device. This is required every time a hardware event
+ * signals that the device write-protect state has changed. It is also
+ * necessary when the user sets or clears the read-only flag on the
+ * whole-disk device.
+ */
+void update_all_part_ro_state(struct gendisk *disk, bool read_only)
+{
+ 	struct disk_part_iter piter;
+ 	struct hd_struct *part;
 
-	disk_part_iter_init(&piter, disk, DISK_PITER_INCL_EMPTY);
+	disk_part_iter_init(&piter, disk, DISK_PITER_INCL_EMPTY_PART0);
 	while ((part = disk_part_iter_next(&piter)))
-		part->policy = flag;
-	disk_part_iter_exit(&piter);
+		update_part_ro_state(part, read_only);
+ 	disk_part_iter_exit(&piter);
 }
 
+/**
+ * set_disk_ro - set a gendisk read-only
+ * @disk:	The disk device
+ * @state:	true or false
+ *
+ * This function is used to indicate whether a given disk device
+ * should have its read-only flag set. set_disk_ro() is typically used
+ * by device drivers to indicate whether the underlying physical
+ * device is write-protected.
+ */
+void set_disk_ro(struct gendisk *disk, bool read_only)
+{
+	if (read_only) {
+		if (test_and_set_bit(GD_READ_ONLY, &disk->state))
+			return;
+	} else {
+		if (!test_and_clear_bit(GD_READ_ONLY, &disk->state))
+			return;
+	}
+	update_all_part_ro_state(disk, read_only);
+	set_disk_ro_uevent(disk, read_only);
+}
 EXPORT_SYMBOL(set_disk_ro);
 
 int bdev_read_only(struct block_device *bdev)
 {
 	if (!bdev)
 		return 0;
-	return bdev->bd_part->policy;
+	return test_bit(GD_READ_ONLY, &bdev->bd_disk->state) ||
+		test_bit(HD_RO_POLICY, &bdev->bd_part->state);
 }
-
 EXPORT_SYMBOL(bdev_read_only);
 
 /*
diff --git a/block/ioctl.c b/block/ioctl.c
index 6b785181344fe1..ce3c9a4dd66d34 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -354,7 +354,10 @@ static int blkdev_roset(struct block_device *bdev, fmode_t mode,
 		if (ret)
 			return ret;
 	}
-	bdev->bd_part->policy = n;
+	if (bdev_is_partition(bdev))
+		update_part_ro_state(bdev->bd_part, n);
+	else
+		update_all_part_ro_state(bdev->bd_disk, n);
 	return 0;
 }
 
diff --git a/block/partitions/core.c b/block/partitions/core.c
index a02e224115943d..aabf9d6d39e59e 100644
--- a/block/partitions/core.c
+++ b/block/partitions/core.c
@@ -192,7 +192,9 @@ static ssize_t part_ro_show(struct device *dev,
 			    struct device_attribute *attr, char *buf)
 {
 	struct hd_struct *p = dev_to_part(dev);
-	return sprintf(buf, "%d\n", p->policy ? 1 : 0);
+	return sprintf(buf, "%d\n",
+		    	test_bit(GD_READ_ONLY, &part_to_disk(p)->state) ||
+		    	test_bit(HD_RO_POLICY, &p->state));
 }
 
 static ssize_t part_alignment_offset_show(struct device *dev,
@@ -414,7 +416,8 @@ static struct hd_struct *add_partition(struct gendisk *disk, int partno,
 	p->start_sect = start;
 	p->nr_sects = len;
 	p->partno = partno;
-	p->policy = get_disk_ro(disk);
+	if (get_disk_ro(disk))
+		set_bit(HD_RO_POLICY, &p->state);
 
 	if (info) {
 		struct partition_meta_info *pinfo;
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 79e334159778a5..2609bc78ff131b 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -67,7 +67,9 @@ struct hd_struct {
 
 	struct device __dev;
 	struct kobject *holder_dir;
-	int policy, partno;
+	int partno;
+	unsigned long state;
+#define HD_RO_POLICY			0
 	struct partition_meta_info *info;
 #ifdef CONFIG_FAIL_MAKE_REQUEST
 	int make_it_fail;
@@ -193,6 +195,7 @@ struct gendisk {
 	int flags;
 	unsigned long state;
 #define GD_NEED_PART_SCAN		0
+#define GD_READ_ONLY			1
 	struct rw_semaphore lookup_sem;
 	struct kobject *slave_dir;
 
@@ -304,11 +307,12 @@ extern void del_gendisk(struct gendisk *gp);
 extern struct gendisk *get_gendisk(dev_t dev, int *partno);
 extern struct block_device *bdget_disk(struct gendisk *disk, int partno);
 
-extern void set_disk_ro(struct gendisk *disk, int flag);
+void set_disk_ro(struct gendisk *disk, bool read_only);
 
 static inline int get_disk_ro(struct gendisk *disk)
 {
-	return disk->part0.policy;
+	return test_bit(GD_READ_ONLY, &disk->state) ||
+		test_bit(HD_RO_POLICY, &disk->part0.state);
 }
 
 extern void disk_block_events(struct gendisk *disk);
-- 
2.28.0

Re: [PATCH 1/3] block: Fix read-only block device setting after revalidate

[Martin K. Petersen]

Hi Christoph!

> [hch: rebased.  don't mirror the compound read-only flag into a field,

My original patch separated "should-write-bios-be-rejected?" state from
"did-the-user-set-this-partition-ro?". In the rebased version a
full-device state transition in update_all_part_ro_state() blows away
any policy the user has set on a given partition.

The blktests that fail are due to something like:

# modprobe scsi_debug num_parts=2
# blockdev --setro /dev/sda2
# grep . /sys/block/sda/sda2/ro
1
# echo 1 > /sys/module/scsi_debug/parameters/wp
# echo 1 > /sys/block/sda/device/rescan
# echo 0 > /sys/module/scsi_debug/parameters/wp
# echo 1 > /sys/block/sda/device/rescan
# grep . /sys/block/sda/sda2/ro
0

The user expectation is that since they set partition 2 readonly it
should remain that way until they either clear the flag or issue
BLKRRPART to cause the partition table to be reread.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 1/3] block: Fix read-only block device setting after revalidate

[Christoph Hellwig]

On Sun, Nov 15, 2020 at 10:55:36PM -0500, Martin K. Petersen wrote:
> My original patch separated "should-write-bios-be-rejected?" state from
> "did-the-user-set-this-partition-ro?". In the rebased version a
> full-device state transition in update_all_part_ro_state() blows away
> any policy the user has set on a given partition.
> 
> The blktests that fail are due to something like:
> 
> # modprobe scsi_debug num_parts=2
> # blockdev --setro /dev/sda2
> # grep . /sys/block/sda/sda2/ro
> 1
> # echo 1 > /sys/module/scsi_debug/parameters/wp
> # echo 1 > /sys/block/sda/device/rescan
> # echo 0 > /sys/module/scsi_debug/parameters/wp
> # echo 1 > /sys/block/sda/device/rescan
> # grep . /sys/block/sda/sda2/ro
> 0
> 
> The user expectation is that since they set partition 2 readonly it
> should remain that way until they either clear the flag or issue
> BLKRRPART to cause the partition table to be reread.

True.  But then again I think the whole idea that a BLKROSET on the
whole device has any effect on the partitions is probably a bad idea.
Even more so once we have the proper hard ro flag in the disk.

I think I'll respin without that.

[PATCH 2/3] rbd: remove the ->set_read_only methods

[Christoph Hellwig]

Now that the hardware read-only state can't be changed by the BLKROSET
ioctl, the code in this method is not required anymore.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/rbd.c | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 671733e459cf47..6ff1193d3d06d0 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -692,29 +692,10 @@ static void rbd_release(struct gendisk *disk, fmode_t mode)
 	put_device(&rbd_dev->dev);
 }
 
-static int rbd_set_read_only(struct block_device *bdev, bool ro)
-{
-	struct rbd_device *rbd_dev = bdev->bd_disk->private_data;
-
-	/*
-	 * Both images mapped read-only and snapshots can't be marked
-	 * read-write.
-	 */
-	if (!ro) {
-		if (rbd_is_ro(rbd_dev))
-			return -EROFS;
-
-		rbd_assert(!rbd_is_snap(rbd_dev));
-	}
-
-	return 0;
-}
-
 static const struct block_device_operations rbd_bd_ops = {
 	.owner			= THIS_MODULE,
 	.open			= rbd_open,
 	.release		= rbd_release,
-	.set_read_only		= rbd_set_read_only,
 };
 
 /*
-- 
2.28.0

[PATCH 3/3] block: replace hd_struct.make_it_fail with a flag

[Christoph Hellwig]

Use a single bit in ->state instead of a whole int.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-core.c      |  3 ++-
 block/genhd.c         | 11 +++++++----
 include/linux/genhd.h |  4 +---
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index b6dea07e08c9c7..de20aeb7d5f0e0 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -668,7 +668,8 @@ __setup("fail_make_request=", setup_fail_make_request);
 
 static bool should_fail_request(struct hd_struct *part, unsigned int bytes)
 {
-	return part->make_it_fail && should_fail(&fail_make_request, bytes);
+	return test_bit(HD_FAIL_REQUEST, &part->state) &&
+		should_fail(&fail_make_request, bytes);
 }
 
 static int __init fail_make_request_debugfs(void)
diff --git a/block/genhd.c b/block/genhd.c
index 65f6b744f4ebd5..bd49fb43554213 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1407,7 +1407,7 @@ ssize_t part_fail_show(struct device *dev,
 {
 	struct hd_struct *p = dev_to_part(dev);
 
-	return sprintf(buf, "%d\n", p->make_it_fail);
+	return sprintf(buf, "%d\n", test_bit(HD_FAIL_REQUEST, &p->state));
 }
 
 ssize_t part_fail_store(struct device *dev,
@@ -1417,9 +1417,12 @@ ssize_t part_fail_store(struct device *dev,
 	struct hd_struct *p = dev_to_part(dev);
 	int i;
 
-	if (count > 0 && sscanf(buf, "%d", &i) > 0)
-		p->make_it_fail = (i == 0) ? 0 : 1;
-
+	if (count > 0 && sscanf(buf, "%d", &i) > 0) {
+		if (i)
+			clear_bit(HD_FAIL_REQUEST, &p->state);
+		else
+			set_bit(HD_FAIL_REQUEST, &p->state);
+	}
 	return count;
 }
 
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 2609bc78ff131b..91b37a738b52b5 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -70,10 +70,8 @@ struct hd_struct {
 	int partno;
 	unsigned long state;
 #define HD_RO_POLICY			0
+#define HD_FAIL_REQUEST			1
 	struct partition_meta_info *info;
-#ifdef CONFIG_FAIL_MAKE_REQUEST
-	int make_it_fail;
-#endif
 	struct rcu_work rcu_work;
 };
 
-- 
2.28.0

Re: split hard read-only vs read-only policy

[Martin K. Petersen]

Christoph,

> this series resurrects a patch from Martin to properly split the flag
> indicating a disk has been set read-only by the hardware vs the
> userspace policy set through the BLKROSET ioctl.

Looks good in general but two of my test cases failed. Will debug.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: split hard read-only vs read-only policy

[Sagi Grimberg]

> Hi Jens,
> 
> this series resurrects a patch from Martin to properly split the flag
> indicating a disk has been set read-only by the hardware vs the userspace
> policy set through the BLKROSET ioctl.
> 

Looks good,

Doesn't this miss restoring the clear disk ro in nvme?

Re: split hard read-only vs read-only policy

[Christoph Hellwig]

On Fri, Nov 13, 2020 at 11:51:38AM -0800, Sagi Grimberg wrote:
>
>> Hi Jens,
>>
>> this series resurrects a patch from Martin to properly split the flag
>> indicating a disk has been set read-only by the hardware vs the userspace
>> policy set through the BLKROSET ioctl.
>>
>
> Looks good,
>
> Doesn't this miss restoring the clear disk ro in nvme?

Yes, eventually.