Linux-FSCrypt Archive on lore.kernel.org
 help / color / Atom feed
From: Eric Biggers <ebiggers@kernel.org>
To: Dave Chinner <david@fromorbit.com>
Cc: Satya Tangirala <satyat@google.com>,
	linux-fscrypt@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-f2fs-devel@lists.sourceforge.net,
	linux-ext4@vger.kernel.org, linux-xfs@vger.kernel.org
Subject: Re: [PATCH v4 3/7] iomap: support direct I/O with fscrypt using blk-crypto
Date: Sat, 25 Jul 2020 19:42:11 -0700
Message-ID: <20200726024211.GA14321@sol.localdomain> (raw)
In-Reply-To: <20200725234751.GR2005@dread.disaster.area>

On Sun, Jul 26, 2020 at 09:47:51AM +1000, Dave Chinner wrote:
> > > > I think you're missing the point here.  Currently, the granularity of encryption
> > > > (a.k.a. "data unit size") is always filesystem blocks, so that's the minimum we
> > > > can directly read or write to an encrypted file.  This has nothing to do with
> > > > the IV wraparound case also being discussed.
> > > 
> > > So when you change the subject, please make it *really obvious* so
> > > that people don't think you are still talking about the same issue.
> > > 
> > > > For example, changing a single bit in the plaintext of a filesystem block may
> > > > result in the entire block's ciphertext changing.  (The exact behavior depends
> > > > on the cryptographic algorithm that is used.)
> > > > 
> > > > That's why this patchset makes ext4 only allow direct I/O on encrypted files if
> > > > the I/O is fully filesystem-block-aligned.  Note that this might be a more
> > > > strict alignment requirement than the bdev_logical_block_size().
> > > > 
> > > > As long as the iomap code only issues filesystem-block-aligned bios, *given
> > > > fully filesystem-block-aligned inputs*, we're fine.  That appears to be the case
> > > > currently.
> > > 
> > > The actual size and shape of the bios issued by direct IO (both old
> > > code and newer iomap code) is determined by the user supplied iov,
> > > the size of the biovec array allocated in the bio, and the IO
> > > constraints of the underlying hardware.  Hence direct IO does not
> > > guarantee alignment to anything larger than the underlying block
> > > device logical sector size because there's no guarantee when or
> > > where a bio will fill up.
> > > 
> > > To guarantee alignment of what ends up at the hardware, you have to
> > > set the block device parameters (e.g. logical sector size)
> > > appropriately all the way down the stack. You also need to ensure
> > > that the filesystem is correctly aligned on the block device so that
> > > filesystem blocks don't overlap things like RAID stripe boundaires,
> > > linear concat boundaries, etc.
> > > 
> > > IOWs, to constrain alignment in the IO path, you need to configure
> > > you system correct so that the information provided to iomap for IO
> > > alignment matches your requirements. This is not somethign iomap can
> > > do itself; everything from above needs to be constrained by the
> > > filesystem using iomap, everything sent below by iomap is
> > > constrained by the block device config.
> > 
> > That way of thinking about things doesn't entirely work for inline encryption.
> 
> Then the inline encryption design is flawed. Block devices tell the
> layers above what the minimum unit of atomic IO is via the logical
> block size of the device is. Everything above the block device
> assumes that it can align and size IO to this size, and the IO will
> succeed.
> 
> > Hardware can support multiple encryption "data unit sizes", some of which may be
> > larger than the logical block size.  (The data unit size is the granularity of
> > encryption.  E.g. if software selects data_unit_size=4096, then each invocation
> > of the encryption/decryption algorithm is passed 4096 bytes.  You can't then
> > later encrypt/decrypt just part of that; that's not how the algorithms work.)
> 
> I know what a DUN is. The problem here is that it's the unit of
> atomic IO the hardware supports when encryption is enabled....
> 
> > For example hardware might *in general* support addressing 512-byte sectors and
> > thus have logical_block_size=512.  But it could also support encryption data
> > unit sizes [512, 1024, 2048, 4096].  Encrypted I/O has to be aligned to the data
> > unit size, not just to the logical block size.  The data unit size to use, and
> > whether to use encryption or not, is decided on a per-I/O basis.
> 
> And that is the fundamental problem here: DUN > logical block size
> of the underlying device. i.e. The storage stack does not guarantee
> atomicity of such IOs.
> 
> If inline encryption increases the size of the atomic unit of IO,
> then the logical block size of the device must increase to match it.
> If you do that, then the iomap and storage layers will guarantee
> that IOs are *always* aligned to DUN boundaries.
> 
> > So in this case technically it's the filesystem (and later the
> > bio::bi_crypt_context which the filesystem sets) that knows about the alignment
> > needed -- *not* the request_queue.
> 
> Exactly my point. Requiring infrastructure and storage layers to
> obey completely new, undefined, undiscoverable, opaque and variable
> definition of the block devices' "atomic unit of IO", then that's
> simply a non-starter. That requires a complete re-architecture of
> the block layers and how things interface and transmit information
> through them. At minimum, high level IO alignment constraints must
> be generic and not be hidden in context specific crypto structures.

Do you have any specific examples in mind of where *encrypted* I/O may broken at
only a logical_block_size boundary?  Remember that encrypted I/O with a
particular data_unit_size is only issued if the request_queue has declared that
it supports encryption with that data_unit_size.  In the case of a layered
device, that means that every layer would have to opt-into supporting encryption
as well as the specific data_unit_size.

Also, the alignment requirement is already passed down the stack as part of the
bio_crypt_ctx.  If there do turn out to be places that need to use it, we could
easily define generic helper functions:

unsigned int bio_required_alignment(struct bio *bio)
{
        unsigned int alignmask = queue_logical_block_size(bio->bi_disk->queue) - 1;

#ifdef CONFIG_BLK_INLINE_ENCRYPTION
        if (bio->bi_crypt_context)
                alignmask |= bio->bi_crypt_context->bc_key->crypto_cfg.data_unit_size - 1;
#endif

        return alignmask + 1;
}

unsigned int rq_required_alignment(struct request *rq)
{
        unsigned int alignmask = queue_logical_block_size(rq->q) - 1;

#ifdef CONFIG_BLK_INLINE_ENCRYPTION
        if (rq->crypt_ctx)
                alignmask |= rq->crypt_ctx->bc_key->crypto_cfg.data_unit_size - 1;
#endif

        return alignmask + 1;
}

Sure, we could also add a new alignment_required field to struct bio and struct
request, but it would be unnecessary since all the information is already there.

> > Is it your opinion that inline encryption should only be supported when
> > data_unit_size <= logical_block_size?  The problems with that are
> 
> Pretty much.
> 
> >     (a) Using an unnecessarily small data_unit_size degrades performance a
> > 	lot -- for *all* I/O, not just direct I/O.  This is because there are a
> > 	lot more separate encryptions/decryptions to do, and there's a fixed
> > 	overhead to each one (much of which is intrinsic in the crypto
> > 	algorithms themselves, i.e. this isn't simply an implementation quirk).
> 
> Performance is irrelevant if correctness is not possible.
> 

As far as I know, data_unit_size > logical_block_size is working for everyone
who has used it so far.

So again, I'm curious if you have any specific examples in mind.  Is this a
real-world problem, or just a theoretical case where (in the future) someone
could declare support for some data_unit_size in their 'struct request_queue'
(possibly for a layered device) without correctly handling alignment in all
cases?

I do see that logical_block_size is used for discard, write_same, and zeroout.
But that isn't encrypted I/O.

BTW, there might very well be hardware that *only* supports
data_unit_size > logical_block_size.

- Eric

  parent reply index

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-20 23:37 [PATCH v4 0/7] add support for " Satya Tangirala
2020-07-20 23:37 ` [PATCH v4 1/7] fscrypt: Add functions for direct I/O support Satya Tangirala
2020-07-22 17:04   ` Jaegeuk Kim
2020-07-20 23:37 ` [PATCH v4 2/7] direct-io: add support for fscrypt using blk-crypto Satya Tangirala
2020-07-22 17:05   ` Jaegeuk Kim
2020-07-20 23:37 ` [PATCH v4 3/7] iomap: support direct I/O with " Satya Tangirala
2020-07-22 17:06   ` Jaegeuk Kim
2020-07-22 21:16   ` Dave Chinner
2020-07-22 22:34     ` Eric Biggers
2020-07-22 22:44       ` Matthew Wilcox
2020-07-22 23:12         ` Eric Biggers
2020-07-22 23:26       ` Eric Biggers
2020-07-22 23:32         ` Darrick J. Wong
2020-07-22 23:43           ` Eric Biggers
2020-07-23 22:07       ` Dave Chinner
2020-07-23 23:03         ` Eric Biggers
2020-07-24  1:39           ` Dave Chinner
2020-07-24  3:46             ` Eric Biggers
2020-07-24  5:31               ` Dave Chinner
2020-07-24 17:41                 ` Eric Biggers
2020-07-25 23:47                   ` Dave Chinner
2020-07-25 23:59                     ` Dave Chinner
2020-07-26  2:42                     ` Eric Biggers [this message]
2020-07-27 17:16                       ` Eric Biggers
2020-07-20 23:37 ` [PATCH v4 4/7] ext4: " Satya Tangirala
2020-07-22 17:07   ` Jaegeuk Kim
2020-07-20 23:37 ` [PATCH v4 5/7] f2fs: " Satya Tangirala
2020-07-21 20:11   ` Jaegeuk Kim
2020-07-20 23:37 ` [PATCH v4 6/7] fscrypt: document inline encryption support Satya Tangirala
2020-07-22 17:01   ` Jaegeuk Kim
2020-07-20 23:37 ` [PATCH v4 7/7] fscrypt: update documentation for direct I/O support Satya Tangirala
2020-07-21  0:47   ` Eric Biggers
2020-07-22 16:57     ` Jaegeuk Kim
2020-07-21  0:56 ` [PATCH v4 0/7] add support for direct I/O with fscrypt using blk-crypto Eric Biggers

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=20200726024211.GA14321@sol.localdomain \
    --to=ebiggers@kernel.org \
    --cc=david@fromorbit.com \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-f2fs-devel@lists.sourceforge.net \
    --cc=linux-fscrypt@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-xfs@vger.kernel.org \
    --cc=satyat@google.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

Linux-FSCrypt Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fscrypt/0 linux-fscrypt/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fscrypt linux-fscrypt/ https://lore.kernel.org/linux-fscrypt \
		linux-fscrypt@vger.kernel.org
	public-inbox-index linux-fscrypt

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fscrypt


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git