public inbox for linux-block@vger.kernel.org
 help / color / mirror / Atom feed
From: Jan Kara <jack@suse.cz>
To: Christoph Hellwig <hch@infradead.org>
Cc: Jan Kara <jack@suse.cz>, Jens Axboe <axboe@kernel.dk>,
	linux-block@vger.kernel.org, Dmitry Vyukov <dvyukov@google.com>,
	Ted Tso <tytso@mit.edu>, yebin <yebin@huaweicloud.com>,
	linux-fsdevel@vger.kernel.org, Kees Cook <keescook@google.com>,
	Alexander Popov <alex.popov@linux.com>,
	Eric Biggers <ebiggers@google.com>
Subject: Re: [PATCH] block: Add config option to not allow writing to mounted devices
Date: Tue, 20 Jun 2023 12:41:11 +0200	[thread overview]
Message-ID: <20230620104111.pnjoouegp2dx6pcp@quack3> (raw)
In-Reply-To: <ZIlqLJ6dFs1P4aj7@infradead.org>

Hello Christoph!

On Wed 14-06-23 00:20:12, Christoph Hellwig wrote:
> On Tue, Jun 13, 2023 at 10:56:14PM +0200, Jan Kara wrote:
> > Well, as I've mentioned in the changelog there are old setups (without
> > initrd) that run fsck on root filesystem mounted read-only and fsck
> > programs tend to open the device with O_RDWR. These would be broken by this
> > change (for the filesystems that would use BLK_OPEN_ flag).
> 
> But that's also a really broken setup that will corrupt data in many
> cases.  So yes, maybe we need a way to allow it, but it probably would
> have to be per-file system.

I was looking into implementing the write hardening support and I've come
across the following obstacle: Your patch series that is in linux-block.git
removes the 'mode' argument from blkdev_put() which makes it impossible to
track how many writers there are for the block device. This is needed so
that we can check whether the filesystem is safe when mounting the device.

I can see several solutions but since you've just reworked the code and I'm
not 100% certain about the motivation, I figured I'll ask you first before
spending significant time on something you won't like:

1) Just return the mode argument to blkdev_put().

2) Only pass to blkdev_put() whether we have write access or not as a
separate argument.

3) Don't track number of opens for writing, instead check whether writes
are blocked on each write access. I think this has a number of downsides
but I mention it for completeness. One problem is we have to add checks to
multiple places (buffered IO, direct IO) and existing mmap in particular
will be very hard to deal with (need to add page_mkwrite() handler). All
these checks add performance overhead. It is practically impossible
(without significant performance overhead or new percpu datastructures) to
properly synchronize open that wants to block writers against already
running writes.

So what would you prefer? Thanks in advance for your input.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

  reply	other threads:[~2023-06-20 10:41 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-06-12 16:16 [PATCH] block: Add config option to not allow writing to mounted devices Jan Kara
2023-06-12 16:25 ` Jan Kara
2023-06-12 17:39   ` Bart Van Assche
2023-06-12 17:47     ` Theodore Ts'o
2023-06-12 18:52     ` Colin Walters
2023-06-13 11:34       ` Jan Kara
2023-06-14  1:55         ` Darrick J. Wong
2023-06-14  7:14           ` Christoph Hellwig
2023-06-14  7:05         ` Christian Brauner
2023-06-14  7:07           ` Christoph Hellwig
2023-06-14  7:10         ` Christoph Hellwig
2023-06-14 10:12           ` Jan Kara
2023-06-14 14:30             ` Christoph Hellwig
2023-06-14 14:46             ` Matthew Wilcox
2023-06-13  4:56 ` kernel test robot
2023-06-13  5:10 ` Christoph Hellwig
2023-06-13  6:09   ` Dmitry Vyukov
2023-06-14  7:17     ` Christoph Hellwig
2023-06-14  8:18       ` Christian Brauner
2023-06-14 10:36         ` Jan Kara
2023-06-14 12:48           ` Christian Brauner
2023-06-15 14:39             ` Jan Kara
2023-06-14 14:31         ` Christoph Hellwig
2023-06-13 20:56   ` Jan Kara
2023-06-14  7:20     ` Christoph Hellwig
2023-06-20 10:41       ` Jan Kara [this message]
2023-06-20 11:29         ` Christoph Hellwig
2023-06-14  7:35     ` Christian Brauner
2023-06-13  6:49 ` Dmitry Vyukov
2023-06-13 19:22   ` Theodore Ts'o
2023-06-14  0:26   ` Dave Chinner
2023-06-14  2:04   ` Darrick J. Wong
2023-06-14  2:57     ` Theodore Ts'o
2023-06-14 12:27     ` Dmitry Vyukov
2023-06-14 23:38       ` Dave Chinner
2023-06-15  9:14         ` Dmitry Vyukov
2023-06-18 23:35           ` Dave Chinner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230620104111.pnjoouegp2dx6pcp@quack3 \
    --to=jack@suse.cz \
    --cc=alex.popov@linux.com \
    --cc=axboe@kernel.dk \
    --cc=dvyukov@google.com \
    --cc=ebiggers@google.com \
    --cc=hch@infradead.org \
    --cc=keescook@google.com \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=tytso@mit.edu \
    --cc=yebin@huaweicloud.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox