[PATCH v2 0/2] block: enforce ioctl(BLKROSET) and set_disk_ro()

[PATCH v2 0/2] block: enforce ioctl(BLKROSET) and set_disk_ro()

[Ilya Dryomov]

Hello,

I was doing some cleanup work on rbd BLKROSET handler and discovered
that we ignore partition rw/ro setting (hd_struct->policy) for pretty
much everything but straight writes.

David (CCed) has blktests patches standing by.

(Another aspect of this is that we don't enforce open(2) mode.  Tejun
took a stab at this a few years ago, but his patch had to be reverted:

  75f1dc0d076d ("block: check bdev_read_only() from blkdev_get()")
  e51900f7d38c ("block: revert block_dev read-only check")

It is a separate issue and refusing writes to read-only devices is
obviously more important, but perhaps it's time to revisit that as
well?)

v1 -> v2:
- added unlikely() per Sagi's suggestion

Thanks,

                Ilya


Ilya Dryomov (2):
  block: fail op_is_write() requests to read-only partitions
  block: add bdev_read_only() checks to common helpers

 block/blk-core.c | 23 ++++++++++++++++++++++-
 block/blk-lib.c  | 12 ++++++++++++
 2 files changed, 34 insertions(+), 1 deletion(-)

-- 
2.4.3

[PATCH v2 1/2] block: fail op_is_write() requests to read-only partitions

[Ilya Dryomov]

Regular block device writes go through blkdev_write_iter(), which does
bdev_read_only(), while zeroout/discard/etc requests are never checked,
both userspace- and kernel-triggered.  Add a generic catch-all check to
generic_make_request_checks() to actually enforce ioctl(BLKROSET) and
set_disk_ro(), which is used by quite a few drivers for things like
snapshots, read-only backing files/images, etc.

Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
---
 block/blk-core.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index f843ae4f858d..d132bec4a266 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2123,6 +2123,20 @@ static inline int bio_check_eod(struct bio *bio, unsigned int nr_sectors)
 	return 0;
 }
 
+static inline bool bio_check_ro(struct bio *bio)
+{
+	struct hd_struct *p;
+	bool ret = false;
+
+	rcu_read_lock();
+	p = __disk_get_part(bio->bi_disk, bio->bi_partno);
+	if (!p || (p->policy && op_is_write(bio_op(bio))))
+		ret = true;
+	rcu_read_unlock();
+
+	return ret;
+}
+
 static noinline_for_stack bool
 generic_make_request_checks(struct bio *bio)
 {
@@ -2145,11 +2159,18 @@ generic_make_request_checks(struct bio *bio)
 		goto end_io;
 	}
 
+	if (unlikely(bio_check_ro(bio))) {
+		printk(KERN_ERR
+		       "generic_make_request: Trying to write "
+			"to read-only block-device %s (partno %d)\n",
+			bio_devname(bio, b), bio->bi_partno);
+		goto end_io;
+	}
+
 	/*
 	 * For a REQ_NOWAIT based request, return -EOPNOTSUPP
 	 * if queue is not a request based queue.
 	 */
-
 	if ((bio->bi_opf & REQ_NOWAIT) && !queue_is_rq_based(q))
 		goto not_supported;
 
-- 
2.4.3

[PATCH v2 2/2] block: add bdev_read_only() checks to common helpers

[Ilya Dryomov]

Similar to blkdev_write_iter(), return -EPERM if the partition is
read-only.  This covers ioctl(), fallocate() and most in-kernel users
but isn't meant to be exhaustive -- everything else will be caught in
generic_make_request_checks(), fail with -EIO and can be fixed later.

Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
---
 block/blk-lib.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index 2bc544ce3d2e..a676084d4740 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -37,6 +37,9 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 	if (!q)
 		return -ENXIO;
 
+	if (bdev_read_only(bdev))
+		return -EPERM;
+
 	if (flags & BLKDEV_DISCARD_SECURE) {
 		if (!blk_queue_secure_erase(q))
 			return -EOPNOTSUPP;
@@ -156,6 +159,9 @@ static int __blkdev_issue_write_same(struct block_device *bdev, sector_t sector,
 	if (!q)
 		return -ENXIO;
 
+	if (bdev_read_only(bdev))
+		return -EPERM;
+
 	bs_mask = (bdev_logical_block_size(bdev) >> 9) - 1;
 	if ((sector | nr_sects) & bs_mask)
 		return -EINVAL;
@@ -233,6 +239,9 @@ static int __blkdev_issue_write_zeroes(struct block_device *bdev,
 	if (!q)
 		return -ENXIO;
 
+	if (bdev_read_only(bdev))
+		return -EPERM;
+
 	/* Ensure that max_write_zeroes_sectors doesn't overflow bi_size */
 	max_write_zeroes_sectors = bdev_write_zeroes_sectors(bdev);
 
@@ -287,6 +296,9 @@ static int __blkdev_issue_zero_pages(struct block_device *bdev,
 	if (!q)
 		return -ENXIO;
 
+	if (bdev_read_only(bdev))
+		return -EPERM;
+
 	while (nr_sects != 0) {
 		bio = next_bio(bio, __blkdev_sectors_to_bio_pages(nr_sects),
 			       gfp_mask);
-- 
2.4.3

Re: [PATCH v2 1/2] block: fail op_is_write() requests to read-only partitions

[Jens Axboe]

On 1/10/18 9:18 AM, Ilya Dryomov wrote:
> Regular block device writes go through blkdev_write_iter(), which does
> bdev_read_only(), while zeroout/discard/etc requests are never checked,
> both userspace- and kernel-triggered.  Add a generic catch-all check to
> generic_make_request_checks() to actually enforce ioctl(BLKROSET) and
> set_disk_ro(), which is used by quite a few drivers for things like
> snapshots, read-only backing files/images, etc.
> 
> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>  block/blk-core.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index f843ae4f858d..d132bec4a266 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -2123,6 +2123,20 @@ static inline int bio_check_eod(struct bio *bio, unsigned int nr_sectors)
>  	return 0;
>  }
>  
> +static inline bool bio_check_ro(struct bio *bio)
> +{
> +	struct hd_struct *p;
> +	bool ret = false;
> +
> +	rcu_read_lock();
> +	p = __disk_get_part(bio->bi_disk, bio->bi_partno);
> +	if (!p || (p->policy && op_is_write(bio_op(bio))))
> +		ret = true;
> +	rcu_read_unlock();
> +
> +	return ret;
> +}> +
>  static noinline_for_stack bool
>  generic_make_request_checks(struct bio *bio)
>  {
> @@ -2145,11 +2159,18 @@ generic_make_request_checks(struct bio *bio)
>  		goto end_io;
>  	}
>  
> +	if (unlikely(bio_check_ro(bio))) {
> +		printk(KERN_ERR
> +		       "generic_make_request: Trying to write "
> +			"to read-only block-device %s (partno %d)\n",
> +			bio_devname(bio, b), bio->bi_partno);
> +		goto end_io;
> +	}

It's yet another check that adds part lookup and rcu lock/unlock in that
path. Can we combine some of them? Make this part of the remap?  This
overhead impacts every IO, let's not bloat it more than absolutely
necessary.

-- 
Jens Axboe


-- 
Jens Axboe

Re: [PATCH v2 1/2] block: fail op_is_write() requests to read-only partitions

[Ilya Dryomov]

On Wed, Jan 10, 2018 at 5:34 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On 1/10/18 9:18 AM, Ilya Dryomov wrote:
>> Regular block device writes go through blkdev_write_iter(), which does
>> bdev_read_only(), while zeroout/discard/etc requests are never checked,
>> both userspace- and kernel-triggered.  Add a generic catch-all check to
>> generic_make_request_checks() to actually enforce ioctl(BLKROSET) and
>> set_disk_ro(), which is used by quite a few drivers for things like
>> snapshots, read-only backing files/images, etc.
>>
>> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
>> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
>> ---
>>  block/blk-core.c | 23 ++++++++++++++++++++++-
>>  1 file changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index f843ae4f858d..d132bec4a266 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -2123,6 +2123,20 @@ static inline int bio_check_eod(struct bio *bio, unsigned int nr_sectors)
>>       return 0;
>>  }
>>
>> +static inline bool bio_check_ro(struct bio *bio)
>> +{
>> +     struct hd_struct *p;
>> +     bool ret = false;
>> +
>> +     rcu_read_lock();
>> +     p = __disk_get_part(bio->bi_disk, bio->bi_partno);
>> +     if (!p || (p->policy && op_is_write(bio_op(bio))))
>> +             ret = true;
>> +     rcu_read_unlock();
>> +
>> +     return ret;
>> +}> +
>>  static noinline_for_stack bool
>>  generic_make_request_checks(struct bio *bio)
>>  {
>> @@ -2145,11 +2159,18 @@ generic_make_request_checks(struct bio *bio)
>>               goto end_io;
>>       }
>>
>> +     if (unlikely(bio_check_ro(bio))) {
>> +             printk(KERN_ERR
>> +                    "generic_make_request: Trying to write "
>> +                     "to read-only block-device %s (partno %d)\n",
>> +                     bio_devname(bio, b), bio->bi_partno);
>> +             goto end_io;
>> +     }
>
> It's yet another check that adds part lookup and rcu lock/unlock in that
> path. Can we combine some of them? Make this part of the remap?  This
> overhead impacts every IO, let's not bloat it more than absolutely
> necessary.

Yes, combining with should_fail_request check in remap should be easy
enough.  I considered it, but opted for the less invasive patch.  I'll
re-spin.

Thanks,

                Ilya

Re: [PATCH v2 1/2] block: fail op_is_write() requests to read-only partitions

[Christoph Hellwig]

On Wed, Jan 10, 2018 at 09:34:02AM -0700, Jens Axboe wrote:
> It's yet another check that adds part lookup and rcu lock/unlock in that
> path. Can we combine some of them? Make this part of the remap?  This
> overhead impacts every IO, let's not bloat it more than absolutely
> necessary.

Yes, we should be able to integrate this with the partition remap.