* Re: [PATCH v2] scsi: sd: block: Fix regressions in read-only block device handling
2019-02-13 2:57 ` [PATCH v2] scsi: sd: block: Fix regressions in read-only block device handling Martin K. Petersen
@ 2019-02-13 7:13 ` Hannes Reinecke
2019-02-16 3:02 ` Martin K. Petersen
` (2 subsequent siblings)
3 siblings, 0 replies; 17+ messages in thread
From: Hannes Reinecke @ 2019-02-13 7:13 UTC (permalink / raw)
To: Martin K. Petersen, linux-scsi, linux-block
Cc: Jeremy Cline, Oleksii Kurochko, stable
On 2/13/19 3:57 AM, 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.
>
> Cc: Jeremy Cline <jeremy@jcline.org>
> Cc: Oleksii Kurochko <olkuroch@cisco.com>
> Cc: stable@vger.kernel.org # v4.16+
> Reported-by: Oleksii Kurochko <olkuroch@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.petersen@oracle.com>
>
> ---
>
> v2:
> - Track user read-only state in a bitmap
>
> - Work around the regression that caused us to drop user
> preferences on revalidate
> ---
> block/genhd.c | 22 +++++++++++++++++-----
> block/ioctl.c | 4 ++++
> block/partition-generic.c | 2 +-
> drivers/scsi/sd.c | 4 +---
> include/linux/genhd.h | 2 ++
> 5 files changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/block/genhd.c b/block/genhd.c
> index 1dd8fd6613b8..34667eb1d3cc 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -1544,19 +1544,31 @@ void set_device_ro(struct block_device *bdev, int flag)
>
> EXPORT_SYMBOL(set_device_ro);
>
> +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;
> +}
> +EXPORT_SYMBOL(get_user_ro);
> +
> void set_disk_ro(struct gendisk *disk, int flag)
> {
> struct disk_part_iter piter;
> struct hd_struct *part;
>
> - if (disk->part0.policy != flag) {
> + if (disk->part0.policy != flag)
> set_disk_ro_uevent(disk, flag);
> - disk->part0.policy = flag;
> - }
>
> - 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;
> + part->policy = get_user_ro(disk, part->partno) ?: flag;
> disk_part_iter_exit(&piter);
> }
>
> 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..c6a3c21c2496 100644
> --- a/block/partition-generic.c
> +++ b/block/partition-generic.c
> @@ -338,7 +338,7 @@ 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);
> + p->policy = get_user_ro(disk, partno) ?: 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 67cc439b86e4..5dfe37b08d3b 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2591,10 +2591,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;
> @@ -2632,7 +2630,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..9645c2604465 100644
> --- a/include/linux/genhd.h
> +++ b/include/linux/genhd.h
> @@ -194,6 +194,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;
> @@ -433,6 +434,7 @@ 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 bool get_user_ro(struct gendisk *disk, unsigned int partno);
>
> static inline int get_disk_ro(struct gendisk *disk)
> {
>
Hmm. Can't say I like it. But as we're dropping partitions on revalidate
I guess we'll have to do it that way. Oh well.
Reviewed-by: Hannes Reinecke <hare@suse.com>
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@suse.de +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2] scsi: sd: block: Fix regressions in read-only block device handling
2019-02-13 2:57 ` [PATCH v2] scsi: sd: block: Fix regressions in read-only block device handling Martin K. Petersen
2019-02-13 7:13 ` Hannes Reinecke
@ 2019-02-16 3:02 ` Martin K. Petersen
2019-02-19 1:36 ` Ming Lei
2019-02-22 14:29 ` Christoph Hellwig
3 siblings, 0 replies; 17+ messages in thread
From: Martin K. Petersen @ 2019-02-16 3:02 UTC (permalink / raw)
To: Christoph Hellwig, Jens Axboe
Cc: linux-scsi, linux-block, Jeremy Cline, Oleksii Kurochko, stable
Christoph? Jens?
> 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.
>
> Cc: Jeremy Cline <jeremy@jcline.org>
> Cc: Oleksii Kurochko <olkuroch@cisco.com>
> Cc: stable@vger.kernel.org # v4.16+
> Reported-by: Oleksii Kurochko <olkuroch@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.petersen@oracle.com>
>
> ---
>
> v2:
> - Track user read-only state in a bitmap
>
> - Work around the regression that caused us to drop user
> preferences on revalidate
> ---
> block/genhd.c | 22 +++++++++++++++++-----
> block/ioctl.c | 4 ++++
> block/partition-generic.c | 2 +-
> drivers/scsi/sd.c | 4 +---
> include/linux/genhd.h | 2 ++
> 5 files changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/block/genhd.c b/block/genhd.c
> index 1dd8fd6613b8..34667eb1d3cc 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -1544,19 +1544,31 @@ void set_device_ro(struct block_device *bdev, int flag)
>
> EXPORT_SYMBOL(set_device_ro);
>
> +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;
> +}
> +EXPORT_SYMBOL(get_user_ro);
> +
> void set_disk_ro(struct gendisk *disk, int flag)
> {
> struct disk_part_iter piter;
> struct hd_struct *part;
>
> - if (disk->part0.policy != flag) {
> + if (disk->part0.policy != flag)
> set_disk_ro_uevent(disk, flag);
> - disk->part0.policy = flag;
> - }
>
> - 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;
> + part->policy = get_user_ro(disk, part->partno) ?: flag;
> disk_part_iter_exit(&piter);
> }
>
> 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..c6a3c21c2496 100644
> --- a/block/partition-generic.c
> +++ b/block/partition-generic.c
> @@ -338,7 +338,7 @@ 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);
> + p->policy = get_user_ro(disk, partno) ?: 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 67cc439b86e4..5dfe37b08d3b 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2591,10 +2591,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;
> @@ -2632,7 +2630,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..9645c2604465 100644
> --- a/include/linux/genhd.h
> +++ b/include/linux/genhd.h
> @@ -194,6 +194,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;
> @@ -433,6 +434,7 @@ 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 bool get_user_ro(struct gendisk *disk, unsigned int partno);
>
> static inline int get_disk_ro(struct gendisk *disk)
> {
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2] scsi: sd: block: Fix regressions in read-only block device handling
2019-02-13 2:57 ` [PATCH v2] scsi: sd: block: Fix regressions in read-only block device handling Martin K. Petersen
@ 2019-02-19 1:36 ` Ming Lei
2019-02-16 3:02 ` Martin K. Petersen
` (2 subsequent siblings)
3 siblings, 0 replies; 17+ messages in thread
From: Ming Lei @ 2019-02-19 1:36 UTC (permalink / raw)
To: Martin K. Petersen, Alan Stern, Greg Kroah-Hartman
Cc: Linux SCSI List, linux-block, Jeremy Cline, Oleksii Kurochko,
stable, linux-usb
On Wed, Feb 13, 2019 at 5:01 PM Martin K. Petersen
<martin.petersen@oracle.com> 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.
Hi Martin & Oleksii,
From the Bugzilla, looks it is only reported on the "Kingston DT Ultimate G3"
USB flash drive.
If it is true, this particular issue might be addressed simply by applying one
quirk on this drive, such as, by adding one delay before calling
sd_spinup_disk()
in the 1st sd_revalidate_disk() to wait until it is ready to process
I/O requests.
Otherwise if there are many such devices, I think your approach is good.
Thanks,
Ming Lei
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2] scsi: sd: block: Fix regressions in read-only block device handling
@ 2019-02-19 1:36 ` Ming Lei
0 siblings, 0 replies; 17+ messages in thread
From: Ming Lei @ 2019-02-19 1:36 UTC (permalink / raw)
To: Martin K. Petersen, Alan Stern, Greg Kroah-Hartman
Cc: Linux SCSI List, linux-block, Jeremy Cline, Oleksii Kurochko,
stable, linux-usb
On Wed, Feb 13, 2019 at 5:01 PM Martin K. Petersen
<martin.petersen@oracle.com> 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.
Hi Martin & Oleksii,
>From the Bugzilla, looks it is only reported on the "Kingston DT Ultimate G3"
USB flash drive.
If it is true, this particular issue might be addressed simply by applying one
quirk on this drive, such as, by adding one delay before calling
sd_spinup_disk()
in the 1st sd_revalidate_disk() to wait until it is ready to process
I/O requests.
Otherwise if there are many such devices, I think your approach is good.
Thanks,
Ming Lei
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2] scsi: sd: block: Fix regressions in read-only block device handling
2019-02-19 1:36 ` Ming Lei
(?)
@ 2019-02-19 23:26 ` Martin K. Petersen
-1 siblings, 0 replies; 17+ messages in thread
From: Martin K. Petersen @ 2019-02-19 23:26 UTC (permalink / raw)
To: Ming Lei
Cc: Martin K. Petersen, Alan Stern, Greg Kroah-Hartman,
Linux SCSI List, linux-block, Jeremy Cline, Oleksii Kurochko,
stable, linux-usb
Hi Ming,
> From the Bugzilla, looks it is only reported on the "Kingston DT
> Ultimate G3" USB flash drive.
There are several bug reports as a result of the change. It's not at all
uncommon for a device to switch at runtime. Before Jeremy's change we
just didn't see it because the device setting always took precedence and
voided any user setting.
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] scsi: sd: block: Fix regressions in read-only block device handling
2019-02-13 2:57 ` [PATCH v2] scsi: sd: block: Fix regressions in read-only block device handling Martin K. Petersen
` (2 preceding siblings ...)
2019-02-19 1:36 ` Ming Lei
@ 2019-02-22 14:29 ` Christoph Hellwig
2019-02-27 4:19 ` [PATCH v3] " Martin K. Petersen
3 siblings, 1 reply; 17+ messages in thread
From: Christoph Hellwig @ 2019-02-22 14:29 UTC (permalink / raw)
To: Martin K. Petersen
Cc: linux-scsi, linux-block, Jeremy Cline, Oleksii Kurochko, stable
On Tue, Feb 12, 2019 at 09:57:17PM -0500, 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.
>
> Cc: Jeremy Cline <jeremy@jcline.org>
> Cc: Oleksii Kurochko <olkuroch@cisco.com>
> Cc: stable@vger.kernel.org # v4.16+
> Reported-by: Oleksii Kurochko <olkuroch@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.petersen@oracle.com>
>
> ---
>
> v2:
> - Track user read-only state in a bitmap
>
> - Work around the regression that caused us to drop user
> preferences on revalidate
> ---
> block/genhd.c | 22 +++++++++++++++++-----
> block/ioctl.c | 4 ++++
> block/partition-generic.c | 2 +-
> drivers/scsi/sd.c | 4 +---
> include/linux/genhd.h | 2 ++
> 5 files changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/block/genhd.c b/block/genhd.c
> index 1dd8fd6613b8..34667eb1d3cc 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -1544,19 +1544,31 @@ void set_device_ro(struct block_device *bdev, int flag)
>
> EXPORT_SYMBOL(set_device_ro);
>
> +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;
> +}
> +EXPORT_SYMBOL(get_user_ro);
No need to export this function.
> + p->policy = get_user_ro(disk, partno) ?: get_disk_ro(disk);
Can we avoid the obsfucating non-standard (GNU extension) use of ?: here?
Just use a local variable and a good old if.
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH v3] scsi: sd: block: Fix regressions in read-only block device handling
2019-02-22 14:29 ` Christoph Hellwig
@ 2019-02-27 4:19 ` Martin K. Petersen
2019-03-07 0:39 ` Martin K. Petersen
0 siblings, 1 reply; 17+ messages in thread
From: Martin K. Petersen @ 2019-02-27 4:19 UTC (permalink / raw)
To: linux-block, linux-scsi
Cc: Martin K. Petersen, Jeremy Cline, Oleksii Kurochko, stable
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 <jeremy@jcline.org>
Cc: Oleksii Kurochko <olkuroch@cisco.com>
Cc: stable@vger.kernel.org # v4.16+
Reported-by: Oleksii Kurochko <olkuroch@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.petersen@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 c78042975737..c28d7ee3a05e 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -790,7 +790,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 1dd8fd6613b8..c07037c0b03f 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1537,26 +1537,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);
}
@@ -1566,7 +1580,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 67cc439b86e4..5dfe37b08d3b 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2591,10 +2591,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;
@@ -2632,7 +2630,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);
--
2.19.2
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v3] scsi: sd: block: Fix regressions in read-only block device handling
2019-02-27 4:19 ` [PATCH v3] " Martin K. Petersen
@ 2019-03-07 0:39 ` Martin K. Petersen
0 siblings, 0 replies; 17+ messages in thread
From: Martin K. Petersen @ 2019-03-07 0:39 UTC (permalink / raw)
To: linux-block, linux-scsi
Somebody please review, need this regression fixed in stable!
> 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 <jeremy@jcline.org>
> Cc: Oleksii Kurochko <olkuroch@cisco.com>
> Cc: stable@vger.kernel.org # v4.16+
> Reported-by: Oleksii Kurochko <olkuroch@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.petersen@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 c78042975737..c28d7ee3a05e 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -790,7 +790,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 1dd8fd6613b8..c07037c0b03f 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -1537,26 +1537,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);
> }
>
> @@ -1566,7 +1580,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 67cc439b86e4..5dfe37b08d3b 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2591,10 +2591,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;
> @@ -2632,7 +2630,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);
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 17+ messages in thread