Linux-f2fs-devel Archive on lore.kernel.org
 help / color / Atom feed
From: Dave Chinner <david@fromorbit.com>
To: Eric Biggers <ebiggers@kernel.org>
Cc: Satya Tangirala <satyat@google.com>,
	linux-f2fs-devel@lists.sourceforge.net,
	linux-xfs@vger.kernel.org, linux-fscrypt@vger.kernel.org,
	linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org
Subject: Re: [f2fs-dev] [PATCH v4 3/7] iomap: support direct I/O with fscrypt using blk-crypto
Date: Fri, 24 Jul 2020 15:31:30 +1000
Message-ID: <20200724053130.GO2005@dread.disaster.area> (raw)
In-Reply-To: <20200724034628.GC870@sol.localdomain>

On Thu, Jul 23, 2020 at 08:46:28PM -0700, Eric Biggers wrote:
> On Fri, Jul 24, 2020 at 11:39:10AM +1000, Dave Chinner wrote:
> > fscrypt_inode_uses_inline_crypto() ends up being:
> > 
> > 	if (IS_ENCRYPTED(inode) && S_ISREG(inode->i_mode) &&
> > 	    inode->i_crypt_info->ci_inlinecrypt)
> > 
> > I note there are no checks for inode->i_crypt_info being non-null,
> > and I note that S_ENCRYPTED is set on the inode when the on-disk
> > encrypted flag is encountered, not when inode->i_crypt_info is set.
> > 
> 
> ->i_crypt_info is set when the file is opened, so it's guaranteed to be set for
> any I/O.  So the case you're concerned about just doesn't happen.

Ok. The connection is not obvious to someone who doesn't know the
fscrypt code inside out.

> > > Note that currently, I don't think iomap_dio_bio_actor() would handle an
> > > encrypted file with blocksize > PAGE_SIZE correctly, as the I/O could be split
> > > in the middle of a filesystem block (even after the filesystem ensures that
> > > direct I/O on encrypted files is fully filesystem-block-aligned, which we do ---
> > > see the rest of this patchset), which isn't allowed on encrypted files.
> > 
> > That can already happen unless you've specifically restricted DIO
> > alignments in the filesystem code. i.e. Direct IO already supports
> > sub-block ranges and alignment, and we can already do user DIO on
> > sub-block, sector aligned ranges just fine. And the filesystem can
> > already split the iomap on sub-block alignments and ranges if it
> > needs to because the iomap uses byte range addressing, not sector or
> > block based addressing.
> > 
> > So either you already have a situation where the 2^32 offset can
> > land *inside* a filesystem block, or the offset is guaranteed to be
> > filesystem block aligned and so you'll never get this "break an IO
> > on sub-block alignment" problem regardless of the filesystem block
> > size...
> > 
> > Either way, it's not an iomap problem - it's a filesystem mapping
> > problem...
> > 
> 
> 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.

> (It's possible that in the future we'll support other encryption data unit
> sizes, perhaps powers of 2 from 512 to filesystem block size.  But for now the
> filesystem block size has been good enough for everyone,

Not the case. fscrypt use in enterprise environments needs support
for block size < page size so that it can be deployed on 64kB page
size machines without requiring 64kB filesystem block sizes.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

  reply index

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-20 23:37 [f2fs-dev] [PATCH v4 0/7] add support for " Satya Tangirala via Linux-f2fs-devel
2020-07-20 23:37 ` [f2fs-dev] [PATCH v4 1/7] fscrypt: Add functions for direct I/O support Satya Tangirala via Linux-f2fs-devel
2020-07-22 17:04   ` Jaegeuk Kim
2020-07-20 23:37 ` [f2fs-dev] [PATCH v4 2/7] direct-io: add support for fscrypt using blk-crypto Satya Tangirala via Linux-f2fs-devel
2020-07-22 17:05   ` Jaegeuk Kim
2020-07-20 23:37 ` [f2fs-dev] [PATCH v4 3/7] iomap: support direct I/O with " Satya Tangirala via Linux-f2fs-devel
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 [this message]
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
2020-07-27 17:16                       ` Eric Biggers
2020-07-20 23:37 ` [f2fs-dev] [PATCH v4 4/7] ext4: " Satya Tangirala via Linux-f2fs-devel
2020-07-22 17:07   ` Jaegeuk Kim
2020-07-20 23:37 ` [f2fs-dev] [PATCH v4 5/7] f2fs: " Satya Tangirala via Linux-f2fs-devel
2020-07-21 20:11   ` Jaegeuk Kim
2020-07-20 23:37 ` [f2fs-dev] [PATCH v4 6/7] fscrypt: document inline encryption support Satya Tangirala via Linux-f2fs-devel
2020-07-22 17:01   ` Jaegeuk Kim
2020-07-20 23:37 ` [f2fs-dev] [PATCH v4 7/7] fscrypt: update documentation for direct I/O support Satya Tangirala via Linux-f2fs-devel
2020-07-21  0:47   ` Eric Biggers
2020-07-22 16:57     ` Jaegeuk Kim
2020-07-21  0:56 ` [f2fs-dev] [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=20200724053130.GO2005@dread.disaster.area \
    --to=david@fromorbit.com \
    --cc=ebiggers@kernel.org \
    --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-f2fs-devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-f2fs-devel/0 linux-f2fs-devel/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-f2fs-devel linux-f2fs-devel/ https://lore.kernel.org/linux-f2fs-devel \
		linux-f2fs-devel@lists.sourceforge.net
	public-inbox-index linux-f2fs-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/net.sourceforge.lists.linux-f2fs-devel


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