[dm-devel] enforce read-only state at the block layer

[dm-devel] enforce read-only state at the block layer

[Christoph Hellwig]

Hi all,

I've recently got a report where a file system can write to a read-only
block device, and while I've not found the root cause yet, it is very
clear that we should not prevents writes to read-only at all.

This did in fact get fixed 5 years ago, but Linus reverted it as older
lvm2 tools relying on this broken behavior.  This series tries to
restore it, although I'm still worried about thee older lvm2 tools
to be honest.  Question to the device mapper maintainers:  is the
any good way to work around that behavior in device mapper if needed
instead of leaving the core block layer and drivers exposed?

Diffstat:
 block/blk-core.c       |   20 ++++++++------------
 include/linux/blkdev.h |    1 -
 2 files changed, 8 insertions(+), 13 deletions(-)

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 1/3] block: remove a duplicate bdev_read_only declaration

[Christoph Hellwig]

bdev_read_only is already declared as in inline in blkdev.h, don't
add another declaration after it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/blkdev.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index d89c2da1469872..20bbce92a91c11 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1455,7 +1455,6 @@ static inline void bio_end_io_acct(struct bio *bio, unsigned long start_time)
 	return bio_end_io_acct_remapped(bio, start_time, bio->bi_bdev);
 }
 
-int bdev_read_only(struct block_device *bdev);
 int set_blocksize(struct block_device *bdev, int size);
 
 int lookup_bdev(const char *pathname, dev_t *dev);
-- 
2.39.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 2/3] block: simplify the check for flushes in bio_check_ro

[Christoph Hellwig]

The only writes without no sectors are pure flush requests, so drop
the extra op_is_flush check.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-core.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 2ae22bebeb3ee1..4ba243968e41eb 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -494,9 +494,8 @@ late_initcall(fail_make_request_debugfs);
 
 static inline void bio_check_ro(struct bio *bio)
 {
-	if (op_is_write(bio_op(bio)) && bdev_read_only(bio->bi_bdev)) {
-		if (op_is_flush(bio->bi_opf) && !bio_sectors(bio))
-			return;
+	if (op_is_write(bio_op(bio)) && bio_sectors(bio) &&
+	    bdev_read_only(bio->bi_bdev)) {
 		pr_warn("Trying to write to read-only block-device %pg\n",
 			bio->bi_bdev);
 		/* Older lvm-tools actually trigger this */
-- 
2.39.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 3/3] block: fail writes to read-only devices

[Christoph Hellwig]

Currently callers can happily submit writes to block devices that are
marked read-only, including to drivers that don't even support writes
and will crash when fed such bios.

While bio submitter should check for read-only devices, that's not a
very robust way of dealing with this.

Note that the last attempt to do this got reverted by Linus in commit
a32e236eb93e ("Partially revert "block: fail op_is_write() requests to
read-only partitions") because device mapper relyied on not enforcing
the read-only state when used together with older lvm-tools.

The lvm side got fixed in:

    https://sourceware.org/git/?p=lvm2.git;a=commit;h=a6fdb9d9d70f51c49ad11a87ab4243344e6701a3

but if people still have older lvm2 tools in use we probably need
to find a workaround for this in device mapper rather than lacking
the core block layer checks.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-core.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 4ba243968e41eb..ef41816bd0eade 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -492,16 +492,6 @@ static int __init fail_make_request_debugfs(void)
 late_initcall(fail_make_request_debugfs);
 #endif /* CONFIG_FAIL_MAKE_REQUEST */
 
-static inline void bio_check_ro(struct bio *bio)
-{
-	if (op_is_write(bio_op(bio)) && bio_sectors(bio) &&
-	    bdev_read_only(bio->bi_bdev)) {
-		pr_warn("Trying to write to read-only block-device %pg\n",
-			bio->bi_bdev);
-		/* Older lvm-tools actually trigger this */
-	}
-}
-
 static noinline int should_fail_bio(struct bio *bio)
 {
 	if (should_fail_request(bdev_whole(bio->bi_bdev), bio->bi_iter.bi_size))
@@ -735,7 +725,14 @@ void submit_bio_noacct(struct bio *bio)
 
 	if (should_fail_bio(bio))
 		goto end_io;
-	bio_check_ro(bio);
+
+	if (op_is_write(bio_op(bio)) && bio_sectors(bio) &&
+	    bdev_read_only(bdev)) {
+		pr_warn("Trying to write to read-only block-device %pg\n",
+			bdev);
+		goto end_io;
+	}
+
 	if (!bio_flagged(bio, BIO_REMAPPED)) {
 		if (unlikely(bio_check_eod(bio)))
 			goto end_io;
-- 
2.39.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 3/3] block: fail writes to read-only devices

[Linus Torvalds]

On Thu, Jun 1, 2023 at 3:28 AM Christoph Hellwig <hch@lst.de> wrote:
>
> Note that the last attempt to do this got reverted by Linus in commit
> a32e236eb93e ("Partially revert "block: fail op_is_write() requests to
> read-only partitions") because device mapper relyied on not enforcing
> the read-only state when used together with older lvm-tools.

I'm certainly happy to try again, but I have my doubts whether this will work.

Note that the problem case is for a device that *used* to be writable,
but became read-only later, and there's an existing writer that needs
that writability.

The lvm fix was not to stop writing, but to simply not mark it
read-only too early.

So I do think that the problem here is purely in the block layer.

The logic wrt "bdev_read_only()" is not necessarily a "right now it's
read-only", but more of a thing that should be checked purely when the
device is opened. Which is pretty much exactly what we do.

So honestly, that whole test for

+       if (op_is_write(bio_op(bio)) && bio_sectors(bio) &&
+           bdev_read_only(bdev)) {

may look "obviously correct", but it's also equally valid to view it
as "obviously garbage", simply because the test is being done at the
wrong point.

The same way you can write to a file that was opened for writing, but
has then become read-only afterwards, writing to a device with a bdev
that was writable when you *started* writing is not at all necessarily
wrong.

So please at least consider the possibility that that test - while it
looks obvious - is simply buggy.

                Linus

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 3/3] block: fail writes to read-only devices

[Christoph Hellwig]

[quoting your reply out of order, because I think it makes sense that
 way]

On Thu, Jun 01, 2023 at 09:02:25PM -0400, Linus Torvalds wrote:
> So honestly, that whole test for
> 
> +       if (op_is_write(bio_op(bio)) && bio_sectors(bio) &&
> +           bdev_read_only(bdev)) {
> 
> may look "obviously correct", but it's also equally valid to view it
> as "obviously garbage", simply because the test is being done at the
> wrong point.
> 
> The same way you can write to a file that was opened for writing, but
> has then become read-only afterwards, writing to a device with a bdev
> that was writable when you *started* writing is not at all necessarily
> wrong.

files, or more specifically file descriptors really are the wrong
analogy here.  A file descriptor allows you to keep writing to
a file that you were allowed to write to at open time.  And that's
fine (at least most of the time, people keep wanting a revoke and
keep implementing broken special cases of it, but I disgress).

The struct block_device is not such a handle, it's the underlying
object.  And the equivalent here is that we allow writes to inodes
that don't even implement a write method, or have the immutable
bit set.

> The logic wrt "bdev_read_only()" is not necessarily a "right now it's
> read-only", but more of a thing that should be checked purely when the
> device is opened. Which is pretty much exactly what we do.

Except the whole make a thing readonly just for fun is the corner case.
DM does it, and we have a sysfs file to allow it.  But the usual
case is that a block device has been read-only all the time, or has
been force to be read-only by the actual storage device, which
doesn't know anything about the file descriptor model, and will
not be happy.

So maybe a lazy read-only after the last writer goes away would be
nice (not that we actully track writers right now, but that whole
area is comletely fucked up and I'm looking into fixing it at the
moment).

And for extra fun blkdev_get_by_dev doesn't check for read-only
because we've historically allowed to open writable file descriptors
on read-only block devices for ioctls (in addition to the magic
(flags & O_ACCMODE) == 3 mode just ioctl). 

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 3/3] block: fail writes to read-only devices

[Linus Torvalds]

On Fri, Jun 2, 2023 at 11:41 AM Christoph Hellwig <hch@lst.de> wrote:
>
> Except the whole make a thing readonly just for fun is the corner case.
> DM does it, and we have a sysfs file to allow it.  But the usual
> case is that a block device has been read-only all the time, or has
> been force to be read-only by the actual storage device, which
> doesn't know anything about the file descriptor model, and will
> not be happy.

The "it's always been read-only" case is unambiguous.

So I do wonder if we should have two read-only bits: a "hard" and
"soft" bit, and make the open-time one the hard bit.

Anyway, I'm ok trying this simple thing once more, but if we end up
getting reports of breakage again, I think you may just need to accept
the fact that "somebody turned the device read-only after the fact"
may just be something the kernel will have to continue to deal with.

We might be able to squirrell the "read-only at time of open" bit away
in the file descriptor in the FMODE_CAN_WRITE bit or something like
that (although that would gives the wrong error for write(): -EINVAL
instead of -EROFS or whatever)

                 Linus

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 3/3] block: fail writes to read-only devices

[Mike Snitzer]

On Thu, Jun 01 2023 at  3:28P -0400,
Christoph Hellwig <hch@lst.de> wrote:

> Currently callers can happily submit writes to block devices that are
> marked read-only, including to drivers that don't even support writes
> and will crash when fed such bios.
> 
> While bio submitter should check for read-only devices, that's not a
> very robust way of dealing with this.
> 
> Note that the last attempt to do this got reverted by Linus in commit
> a32e236eb93e ("Partially revert "block: fail op_is_write() requests to
> read-only partitions") because device mapper relyied on not enforcing
> the read-only state when used together with older lvm-tools.
> 
> The lvm side got fixed in:
> 
>     https://sourceware.org/git/?p=lvm2.git;a=commit;h=a6fdb9d9d70f51c49ad11a87ab4243344e6701a3
> 
> but if people still have older lvm2 tools in use we probably need
> to find a workaround for this in device mapper rather than lacking
> the core block layer checks.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Mike Snitzer <snitzer@kernel.org>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] enforce read-only state at the block layer

[Mike Snitzer]

On Thu, Jun 01 2023 at  3:28P -0400,
Christoph Hellwig <hch@lst.de> wrote:

> Hi all,
> 
> I've recently got a report where a file system can write to a read-only
> block device, and while I've not found the root cause yet, it is very
> clear that we should not prevents writes to read-only at all.
> 
> This did in fact get fixed 5 years ago, but Linus reverted it as older
> lvm2 tools relying on this broken behavior.  This series tries to
> restore it, although I'm still worried about thee older lvm2 tools
> to be honest.  Question to the device mapper maintainers:  is the
> any good way to work around that behavior in device mapper if needed
> instead of leaving the core block layer and drivers exposed?

Given the block core change (in patch 3) _and_ old lvm2 code: it'll
obviously fail.

Not sure of a crafty hack to workaround. Hopefully 5 year old lvm2
remains tightly coupled to kernels of the same vintage and we get
lucky moving forward.

So I agree with Linus, worth trying this simple change again and
seeing if there is fallout. Revert/worry about it again as needed.

Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] enforce read-only state at the block layer

[Christoph Hellwig]

> Not sure of a crafty hack to workaround. Hopefully 5 year old lvm2
> remains tightly coupled to kernels of the same vintage and we get
> lucky moving forward.
> 
> So I agree with Linus, worth trying this simple change again and
> seeing if there is fallout. Revert/worry about it again as needed.

I'm actually looking into implementing Linus' suggestion now and
track in the block_device if it has ever been opened for writing.

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel