* [PATCH v2 0/2] block: enforce ioctl(BLKROSET) and set_disk_ro()
@ 2018-01-10 16:18 Ilya Dryomov
2018-01-10 16:18 ` [PATCH v2 1/2] block: fail op_is_write() requests to read-only partitions Ilya Dryomov
2018-01-10 16:18 ` [PATCH v2 2/2] block: add bdev_read_only() checks to common helpers Ilya Dryomov
0 siblings, 2 replies; 6+ messages in thread
From: Ilya Dryomov @ 2018-01-10 16:18 UTC (permalink / raw)
To: linux-block
Cc: Jens Axboe, Christoph Hellwig, Tejun Heo, David Disseldorp,
Sagi Grimberg
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
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v2 1/2] block: fail op_is_write() requests to read-only partitions 2018-01-10 16:18 [PATCH v2 0/2] block: enforce ioctl(BLKROSET) and set_disk_ro() Ilya Dryomov @ 2018-01-10 16:18 ` Ilya Dryomov 2018-01-10 16:34 ` Jens Axboe 2018-01-10 16:18 ` [PATCH v2 2/2] block: add bdev_read_only() checks to common helpers Ilya Dryomov 1 sibling, 1 reply; 6+ messages in thread From: Ilya Dryomov @ 2018-01-10 16:18 UTC (permalink / raw) To: linux-block Cc: Jens Axboe, Christoph Hellwig, Tejun Heo, David Disseldorp, Sagi Grimberg 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] block: fail op_is_write() requests to read-only partitions 2018-01-10 16:18 ` [PATCH v2 1/2] block: fail op_is_write() requests to read-only partitions Ilya Dryomov @ 2018-01-10 16:34 ` Jens Axboe 2018-01-10 16:49 ` Ilya Dryomov 2018-01-10 16:55 ` Christoph Hellwig 0 siblings, 2 replies; 6+ messages in thread From: Jens Axboe @ 2018-01-10 16:34 UTC (permalink / raw) To: Ilya Dryomov, linux-block Cc: Christoph Hellwig, Tejun Heo, David Disseldorp, Sagi Grimberg 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] block: fail op_is_write() requests to read-only partitions 2018-01-10 16:34 ` Jens Axboe @ 2018-01-10 16:49 ` Ilya Dryomov 2018-01-10 16:55 ` Christoph Hellwig 1 sibling, 0 replies; 6+ messages in thread From: Ilya Dryomov @ 2018-01-10 16:49 UTC (permalink / raw) To: Jens Axboe Cc: linux-block, Christoph Hellwig, Tejun Heo, David Disseldorp, Sagi Grimberg 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] block: fail op_is_write() requests to read-only partitions 2018-01-10 16:34 ` Jens Axboe 2018-01-10 16:49 ` Ilya Dryomov @ 2018-01-10 16:55 ` Christoph Hellwig 1 sibling, 0 replies; 6+ messages in thread From: Christoph Hellwig @ 2018-01-10 16:55 UTC (permalink / raw) To: Jens Axboe Cc: Ilya Dryomov, linux-block, Christoph Hellwig, Tejun Heo, David Disseldorp, Sagi Grimberg 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] block: add bdev_read_only() checks to common helpers 2018-01-10 16:18 [PATCH v2 0/2] block: enforce ioctl(BLKROSET) and set_disk_ro() Ilya Dryomov 2018-01-10 16:18 ` [PATCH v2 1/2] block: fail op_is_write() requests to read-only partitions Ilya Dryomov @ 2018-01-10 16:18 ` Ilya Dryomov 1 sibling, 0 replies; 6+ messages in thread From: Ilya Dryomov @ 2018-01-10 16:18 UTC (permalink / raw) To: linux-block Cc: Jens Axboe, Christoph Hellwig, Tejun Heo, David Disseldorp, Sagi Grimberg 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-01-10 16:55 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-01-10 16:18 [PATCH v2 0/2] block: enforce ioctl(BLKROSET) and set_disk_ro() Ilya Dryomov 2018-01-10 16:18 ` [PATCH v2 1/2] block: fail op_is_write() requests to read-only partitions Ilya Dryomov 2018-01-10 16:34 ` Jens Axboe 2018-01-10 16:49 ` Ilya Dryomov 2018-01-10 16:55 ` Christoph Hellwig 2018-01-10 16:18 ` [PATCH v2 2/2] block: add bdev_read_only() checks to common helpers Ilya Dryomov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).