On Wed, 2019-03-27 at 12:21 -0400, Martin K. Petersen wrote:
> Some devices come online in write protected state and switch to
> read-write once they are ready to process I/O requests. These devices
> broke with commit 20bd1d026aac ("scsi: sd: Keep disk read-only when
> re-reading partition") because we had no way to distinguish between a
> user decision to set a block_device read-only and the actual hardware
> device being write-protected.
>
> Because partitions are dropped and recreated on revalidate we are
> unable to persist any user-provided policy in hd_struct. Introduce a
> bitmap in struct gendisk to track the user configuration. This bitmap
> is updated when BLKROSET is called on a given disk or partition.
>
> A helper function, get_user_ro(), is provided to determine whether the
> ioctl has forced read-only state for a given block device. This helper
> is used by set_disk_ro() and add_partition() to ensure that both
> existing and newly created partitions will get the correct state.
>
> - If BLKROSET sets a whole disk device read-only, all partitions will
> now end up in a read-only state.
>
> - If BLKROSET sets a given partition read-only, that partition will
> remain read-only post revalidate.
>
> - Otherwise both the whole disk device and any partitions will
> reflect the write protect state of the underlying device.
>
> Since nobody knows what "policy" means, rename the field to
> "read_only" for clarity.
>
> Cc: Jeremy Cline <jer...@jcline.org>
> Cc: Oleksii Kurochko <olkur...@cisco.com>
> Cc: sta...@vger.kernel.org # v4.16+
> Reported-by: Oleksii Kurochko <olkur...@cisco.com>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=201221
> Fixes: 20bd1d026aac ("scsi: sd: Keep disk read-only when re-reading
> partition")
> Signed-off-by: Martin K. Petersen <martin.peter...@oracle.com>
>
> ---
>
> v3:
> - Drop ?: since gcc complains about mixing int and bool (zeroday)
> - Drop EXPORT_SYMBOL (hch)
> - s/policy/read_only/ and make it a boolean
>
> v2:
> - Track user read-only state in a bitmap
>
> - Work around the regression that caused us to drop user
> preferences on revalidate
> ---
> block/blk-core.c | 2 +-
> block/genhd.c | 34 ++++++++++++++++++++++++----------
> block/ioctl.c | 4 ++++
> block/partition-generic.c | 7 +++++--
> drivers/scsi/sd.c | 4 +---
> include/linux/genhd.h | 11 +++++++----
> 6 files changed, 42 insertions(+), 20 deletions(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 4673ebe42255..932f179a9095 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -792,7 +792,7 @@ 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 (part->read_only && op_is_write(op)) {
> char b[BDEVNAME_SIZE];
>
> if (op_is_flush(bio->bi_opf) && !bio_sectors(bio))
> diff --git a/block/genhd.c b/block/genhd.c
> index 703267865f14..9f3a93ca1851 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -1539,26 +1539,40 @@ static void set_disk_ro_uevent(struct gendisk *gd,
> int ro)
> kobject_uevent_env(&disk_to_dev(gd)->kobj, KOBJ_CHANGE, envp);
> }
>
> -void set_device_ro(struct block_device *bdev, int flag)
> +void set_device_ro(struct block_device *bdev, bool state)
> {
> - bdev->bd_part->policy = flag;
> + bdev->bd_part->read_only = state;
> }
>
> EXPORT_SYMBOL(set_device_ro);
>
> -void set_disk_ro(struct gendisk *disk, int flag)
> +bool get_user_ro(struct gendisk *disk, unsigned int partno)
> +{
> + /* Is the user read-only bit set for the whole disk device? */
> + if (test_bit(0, disk->user_ro_bitmap))
> + return true;
> +
> + /* Is the user read-only bit set for this particular partition? */
> + if (test_bit(partno, disk->user_ro_bitmap))
> + return true;
> +
> + return false;
> +}
> +
> +void set_disk_ro(struct gendisk *disk, bool state)
> {
> struct disk_part_iter piter;
> struct hd_struct *part;
>
> - if (disk->part0.policy != flag) {
> - set_disk_ro_uevent(disk, flag);
> - disk->part0.policy = flag;
> - }
> + if (disk->part0.read_only != state)
> + set_disk_ro_uevent(disk, state);
>
> - 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;
> + if (get_user_ro(disk, part->partno))
> + part->read_only = true;
> + else
> + part->read_only = state;
> disk_part_iter_exit(&piter);
> }
>
> @@ -1568,7 +1582,7 @@ int bdev_read_only(struct block_device *bdev)
> {
> if (!bdev)
> return 0;
> - return bdev->bd_part->policy;
> + return bdev->bd_part->read_only;
> }
>
> EXPORT_SYMBOL(bdev_read_only);
> diff --git a/block/ioctl.c b/block/ioctl.c
> index 4825c78a6baa..41206df89485 100644
> --- a/block/ioctl.c
> +++ b/block/ioctl.c
> @@ -451,6 +451,10 @@ static int blkdev_roset(struct block_device *bdev,
> fmode_t mode,
> return ret;
> if (get_user(n, (int __user *)arg))
> return -EFAULT;
> + if (n)
> + set_bit(bdev->bd_partno, bdev->bd_disk->user_ro_bitmap);
> + else
> + clear_bit(bdev->bd_partno, bdev->bd_disk->user_ro_bitmap);
> set_device_ro(bdev, n);
> return 0;
> }
> diff --git a/block/partition-generic.c b/block/partition-generic.c
> index 8e596a8dff32..2bade849cc5c 100644
> --- a/block/partition-generic.c
> +++ b/block/partition-generic.c
> @@ -98,7 +98,7 @@ 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, "%u\n", p->read_only ? 1 : 0);
> }
>
> static ssize_t part_alignment_offset_show(struct device *dev,
> @@ -338,7 +338,10 @@ struct hd_struct *add_partition(struct gendisk *disk,
> int partno,
> queue_limit_discard_alignment(&disk->queue->limits, start);
> p->nr_sects = len;
> p->partno = partno;
> - p->policy = get_disk_ro(disk);
> + if (get_user_ro(disk, partno))
> + p->read_only = true;
> + else
> + p->read_only = get_disk_ro(disk);
>
> if (info) {
> struct partition_meta_info *pinfo = alloc_part_info(disk);
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 251db30d0882..9d8e15d03d2b 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2608,10 +2608,8 @@ sd_read_write_protect_flag(struct scsi_disk *sdkp,
> unsigned char *buffer)
> int res;
> struct scsi_device *sdp = sdkp->device;
> struct scsi_mode_data data;
> - int disk_ro = get_disk_ro(sdkp->disk);
> int old_wp = sdkp->write_prot;
>
> - set_disk_ro(sdkp->disk, 0);
> if (sdp->skip_ms_page_3f) {
> sd_first_printk(KERN_NOTICE, sdkp, "Assuming Write Enabled\n");
> return;
> @@ -2649,7 +2647,7 @@ sd_read_write_protect_flag(struct scsi_disk *sdkp,
> unsigned char *buffer)
> "Test WP failed, assume Write Enabled\n");
> } else {
> sdkp->write_prot = ((data.device_specific & 0x80) != 0);
> - set_disk_ro(sdkp->disk, sdkp->write_prot || disk_ro);
> + set_disk_ro(sdkp->disk, sdkp->write_prot);
> if (sdkp->first_scan || old_wp != sdkp->write_prot) {
> sd_printk(KERN_NOTICE, sdkp, "Write Protect is %s\n",
> sdkp->write_prot ? "on" : "off");
> diff --git a/include/linux/genhd.h b/include/linux/genhd.h
> index 06c0fd594097..1abde0e88ccb 100644
> --- a/include/linux/genhd.h
> +++ b/include/linux/genhd.h
> @@ -118,7 +118,8 @@ struct hd_struct {
> unsigned int discard_alignment;
> struct device __dev;
> struct kobject *holder_dir;
> - int policy, partno;
> + bool read_only;
> + int partno;
> struct partition_meta_info *info;
> #ifdef CONFIG_FAIL_MAKE_REQUEST
> int make_it_fail;
> @@ -194,6 +195,7 @@ struct gendisk {
> */
> struct disk_part_tbl __rcu *part_tbl;
> struct hd_struct part0;
> + DECLARE_BITMAP(user_ro_bitmap, DISK_MAX_PARTS);
>
> const struct block_device_operations *fops;
> struct request_queue *queue;
> @@ -431,12 +433,13 @@ 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_device_ro(struct block_device *bdev, int flag);
> -extern void set_disk_ro(struct gendisk *disk, int flag);
> +extern void set_device_ro(struct block_device *bdev, bool state);
> +extern void set_disk_ro(struct gendisk *disk, bool state);
> +extern bool get_user_ro(struct gendisk *disk, unsigned int partno);
>
> static inline int get_disk_ro(struct gendisk *disk)
> {
> - return disk->part0.policy;
> + return disk->part0.read_only;
> }
>
> extern void disk_block_events(struct gendisk *disk);
Looks good.
Reviewed-by: Ewan D. Milne <emi...@redhat.com>
