linux-xfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/7] add support for direct I/O with fscrypt using blk-crypto
@ 2020-07-20 23:37 Satya Tangirala
  2020-07-20 23:37 ` [PATCH v4 1/7] fscrypt: Add functions for direct I/O support Satya Tangirala
                   ` (7 more replies)
  0 siblings, 8 replies; 34+ messages in thread
From: Satya Tangirala @ 2020-07-20 23:37 UTC (permalink / raw)
  To: linux-fscrypt, linux-fsdevel, linux-f2fs-devel, linux-ext4
  Cc: linux-xfs, Satya Tangirala

This patch series adds support for direct I/O with fscrypt using
blk-crypto. It has been rebased on fscrypt/master.

Patch 1 adds two functions to fscrypt that need to be called to determine
if direct I/O is supported for a request.

Patches 2 and 3 wire up direct-io and iomap respectively with the functions
introduced in Patch 1 and set bio crypt contexts on bios when appropriate
by calling into fscrypt.

Patches 4 and 5 allow ext4 and f2fs direct I/O to support fscrypt without
falling back to buffered I/O.

Patches 6 and 7 update the fscrypt documentation for inline encryption
support and direct I/O. The documentation now notes the required conditions
for inline encryption and direct I/O on encrypted files.

This patch series was tested by running xfstests with test_dummy_encryption
with and without the 'inlinecrypt' mount option, and there were no
meaningful regressions. One regression was for generic/587 on ext4,
but that test isn't compatible with test_dummy_encryption in the first
place, and the test "incorrectly" passes without the 'inlinecrypt' mount
option - a patch will be sent out to exclude that test when
test_dummy_encryption is turned on with ext4 (like the other quota related
tests that use user visible quota files). The other regression was for
generic/252 on ext4, which does direct I/O with a buffer aligned to the
block device's blocksize, but not necessarily aligned to the filesystem's
block size, which direct I/O with fscrypt requires.

Changes v3 => v4:
 - Fix bug in iomap_dio_bio_actor() where fscrypt_limit_io_pages() was
   being called too early (thanks Eric!)
 - Improve comments and fix formatting in documentation
 - iomap_dio_zero() is only called to zero out partial blocks, but
   direct I/O is only supported on encrypted files when I/O is
   blocksize aligned, so it doesn't need to set encryption contexts on
   bios. Replace setting the encryption context with a WARN_ON(). (Eric)

Changes v2 => v3:
 - add changelog to coverletter

Changes v1 => v2:
 - Fix bug in f2fs caused by replacing f2fs_post_read_required() with
   !fscrypt_dio_supported() since the latter doesn't check for
   compressed inodes unlike the former.
 - Add patches 6 and 7 for fscrypt documentation
 - cleanups and comments

Eric Biggers (5):
  fscrypt: Add functions for direct I/O support
  direct-io: add support for fscrypt using blk-crypto
  iomap: support direct I/O with fscrypt using blk-crypto
  ext4: support direct I/O with fscrypt using blk-crypto
  f2fs: support direct I/O with fscrypt using blk-crypto

Satya Tangirala (2):
  fscrypt: document inline encryption support
  fscrypt: update documentation for direct I/O support

 Documentation/filesystems/fscrypt.rst | 36 +++++++++++-
 fs/crypto/crypto.c                    |  8 +++
 fs/crypto/inline_crypt.c              | 82 +++++++++++++++++++++++++++
 fs/direct-io.c                        | 15 ++++-
 fs/ext4/file.c                        | 10 ++--
 fs/f2fs/f2fs.h                        |  6 +-
 fs/iomap/direct-io.c                  | 12 +++-
 include/linux/fscrypt.h               | 19 +++++++
 8 files changed, 178 insertions(+), 10 deletions(-)

-- 
2.28.0.rc0.105.gf9edc3c819-goog


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v4 1/7] fscrypt: Add functions for direct I/O support
  2020-07-20 23:37 [PATCH v4 0/7] add support for direct I/O with fscrypt using blk-crypto Satya Tangirala
@ 2020-07-20 23:37 ` 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
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 34+ messages in thread
From: Satya Tangirala @ 2020-07-20 23:37 UTC (permalink / raw)
  To: linux-fscrypt, linux-fsdevel, linux-f2fs-devel, linux-ext4
  Cc: linux-xfs, Eric Biggers, Satya Tangirala

From: Eric Biggers <ebiggers@google.com>

Introduce fscrypt_dio_supported() to check whether a direct I/O request
is unsupported due to encryption constraints.

Also introduce fscrypt_limit_io_pages() to limit how many pages can be
added to a bio being prepared for direct I/O. This is needed for the
iomap direct I/O implementation to avoid DUN wraparound in the middle of
a bio (which is possible with the IV_INO_LBLK_32 IV generation method).
Elsewhere fscrypt_mergeable_bio() is used for this, but iomap operates
on logical ranges directly and doesn't have a chance to call
fscrypt_mergeable_bio() on every block or page. So we need this function
which limits a logical range in one go.

Signed-off-by: Eric Biggers <ebiggers@google.com>
Co-developed-by: Satya Tangirala <satyat@google.com>
Signed-off-by: Satya Tangirala <satyat@google.com>
---
 fs/crypto/crypto.c       |  8 ++++
 fs/crypto/inline_crypt.c | 82 ++++++++++++++++++++++++++++++++++++++++
 include/linux/fscrypt.h  | 19 ++++++++++
 3 files changed, 109 insertions(+)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index a52cf32733ab..fb34364360b3 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -69,6 +69,14 @@ void fscrypt_free_bounce_page(struct page *bounce_page)
 }
 EXPORT_SYMBOL(fscrypt_free_bounce_page);
 
+/*
+ * Generate the IV for the given logical block number within the given file.
+ * For filenames encryption, lblk_num == 0.
+ *
+ * Keep this in sync with fscrypt_limit_io_pages().  fscrypt_limit_io_pages()
+ * needs to know about any IV generation methods where the low bits of IV don't
+ * simply contain the lblk_num (e.g., IV_INO_LBLK_32).
+ */
 void fscrypt_generate_iv(union fscrypt_iv *iv, u64 lblk_num,
 			 const struct fscrypt_info *ci)
 {
diff --git a/fs/crypto/inline_crypt.c b/fs/crypto/inline_crypt.c
index d7aecadf33c1..578739712e00 100644
--- a/fs/crypto/inline_crypt.c
+++ b/fs/crypto/inline_crypt.c
@@ -16,6 +16,7 @@
 #include <linux/blkdev.h>
 #include <linux/buffer_head.h>
 #include <linux/sched/mm.h>
+#include <linux/uio.h>
 
 #include "fscrypt_private.h"
 
@@ -362,3 +363,84 @@ bool fscrypt_mergeable_bio_bh(struct bio *bio,
 	return fscrypt_mergeable_bio(bio, inode, next_lblk);
 }
 EXPORT_SYMBOL_GPL(fscrypt_mergeable_bio_bh);
+
+/**
+ * fscrypt_dio_supported() - check whether a direct I/O request is unsupported
+ *			     due to encryption constraints
+ * @iocb: the file and position the I/O is targeting
+ * @iter: the I/O data segment(s)
+ *
+ * Return: true if direct I/O is supported
+ */
+bool fscrypt_dio_supported(struct kiocb *iocb, struct iov_iter *iter)
+{
+	const struct inode *inode = file_inode(iocb->ki_filp);
+	const unsigned int blocksize = i_blocksize(inode);
+
+	/* If the file is unencrypted, no veto from us. */
+	if (!fscrypt_needs_contents_encryption(inode))
+		return true;
+
+	/* We only support direct I/O with inline crypto, not fs-layer crypto */
+	if (!fscrypt_inode_uses_inline_crypto(inode))
+		return false;
+
+	/*
+	 * Since the granularity of encryption is filesystem blocks, the I/O
+	 * must be block aligned -- not just disk sector aligned.
+	 */
+	if (!IS_ALIGNED(iocb->ki_pos | iov_iter_alignment(iter), blocksize))
+		return false;
+
+	return true;
+}
+EXPORT_SYMBOL_GPL(fscrypt_dio_supported);
+
+/**
+ * fscrypt_limit_io_pages() - limit I/O pages to avoid discontiguous DUNs
+ * @inode: the file on which I/O is being done
+ * @pos: the file position (in bytes) at which the I/O is being done
+ * @nr_pages: the number of pages we want to submit starting at @pos
+ *
+ * Determine the limit to the number of pages that can be submitted in the bio
+ * targeting @pos without causing a data unit number (DUN) discontinuity.
+ *
+ * This is normally just @nr_pages, as normally the DUNs just increment along
+ * with the logical blocks.  (Or the file is not encrypted.)
+ *
+ * In rare cases, fscrypt can be using an IV generation method that allows the
+ * DUN to wrap around within logically continuous blocks, and that wraparound
+ * will occur.  If this happens, a value less than @nr_pages will be returned so
+ * that the wraparound doesn't occur in the middle of the bio.  Note that we
+ * only support block_size == PAGE_SIZE (and page-aligned DIO) in such cases.
+ *
+ * Return: the actual number of pages that can be submitted
+ */
+int fscrypt_limit_io_pages(const struct inode *inode, loff_t pos, int nr_pages)
+{
+	const struct fscrypt_info *ci = inode->i_crypt_info;
+	u32 dun;
+
+	if (!fscrypt_inode_uses_inline_crypto(inode))
+		return nr_pages;
+
+	if (nr_pages <= 1)
+		return nr_pages;
+
+	if (!(fscrypt_policy_flags(&ci->ci_policy) &
+	      FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32))
+		return nr_pages;
+
+	/*
+	 * fscrypt_select_encryption_impl() ensures that block_size == PAGE_SIZE
+	 * when using FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32.
+	 */
+	if (WARN_ON_ONCE(i_blocksize(inode) != PAGE_SIZE))
+		return 1;
+
+	/* With IV_INO_LBLK_32, the DUN can wrap around from U32_MAX to 0. */
+
+	dun = ci->ci_hashed_ino + (pos >> inode->i_blkbits);
+
+	return min_t(u64, nr_pages, (u64)U32_MAX + 1 - dun);
+}
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index bb257411365f..c205c214b35e 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -559,6 +559,11 @@ bool fscrypt_mergeable_bio(struct bio *bio, const struct inode *inode,
 bool fscrypt_mergeable_bio_bh(struct bio *bio,
 			      const struct buffer_head *next_bh);
 
+bool fscrypt_dio_supported(struct kiocb *iocb, struct iov_iter *iter);
+
+int fscrypt_limit_io_pages(const struct inode *inode, loff_t pos,
+			   int nr_pages);
+
 #else /* CONFIG_FS_ENCRYPTION_INLINE_CRYPT */
 
 static inline bool __fscrypt_inode_uses_inline_crypto(const struct inode *inode)
@@ -587,6 +592,20 @@ static inline bool fscrypt_mergeable_bio_bh(struct bio *bio,
 {
 	return true;
 }
+
+static inline bool fscrypt_dio_supported(struct kiocb *iocb,
+					 struct iov_iter *iter)
+{
+	const struct inode *inode = file_inode(iocb->ki_filp);
+
+	return !fscrypt_needs_contents_encryption(inode);
+}
+
+static inline int fscrypt_limit_io_pages(const struct inode *inode, loff_t pos,
+					 int nr_pages)
+{
+	return nr_pages;
+}
 #endif /* !CONFIG_FS_ENCRYPTION_INLINE_CRYPT */
 
 /**
-- 
2.28.0.rc0.105.gf9edc3c819-goog


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH v4 2/7] direct-io: add support for fscrypt using blk-crypto
  2020-07-20 23:37 [PATCH v4 0/7] add support for direct I/O with fscrypt using blk-crypto Satya Tangirala
  2020-07-20 23:37 ` [PATCH v4 1/7] fscrypt: Add functions for direct I/O support Satya Tangirala
@ 2020-07-20 23:37 ` 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
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 34+ messages in thread
From: Satya Tangirala @ 2020-07-20 23:37 UTC (permalink / raw)
  To: linux-fscrypt, linux-fsdevel, linux-f2fs-devel, linux-ext4
  Cc: linux-xfs, Eric Biggers, Satya Tangirala

From: Eric Biggers <ebiggers@google.com>

Set bio crypt contexts on bios by calling into fscrypt when required,
and explicitly check for DUN continuity when adding pages to the bio.
(While DUN continuity is usually implied by logical block contiguity,
this is not the case when using certain fscrypt IV generation methods
like IV_INO_LBLK_32).

Signed-off-by: Eric Biggers <ebiggers@google.com>
Co-developed-by: Satya Tangirala <satyat@google.com>
Signed-off-by: Satya Tangirala <satyat@google.com>
---
 fs/direct-io.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/fs/direct-io.c b/fs/direct-io.c
index 6d5370eac2a8..f27f7e3780ee 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -24,6 +24,7 @@
 #include <linux/module.h>
 #include <linux/types.h>
 #include <linux/fs.h>
+#include <linux/fscrypt.h>
 #include <linux/mm.h>
 #include <linux/slab.h>
 #include <linux/highmem.h>
@@ -411,6 +412,7 @@ dio_bio_alloc(struct dio *dio, struct dio_submit *sdio,
 	      sector_t first_sector, int nr_vecs)
 {
 	struct bio *bio;
+	struct inode *inode = dio->inode;
 
 	/*
 	 * bio_alloc() is guaranteed to return a bio when allowed to sleep and
@@ -418,6 +420,9 @@ dio_bio_alloc(struct dio *dio, struct dio_submit *sdio,
 	 */
 	bio = bio_alloc(GFP_KERNEL, nr_vecs);
 
+	fscrypt_set_bio_crypt_ctx(bio, inode,
+				  sdio->cur_page_fs_offset >> inode->i_blkbits,
+				  GFP_KERNEL);
 	bio_set_dev(bio, bdev);
 	bio->bi_iter.bi_sector = first_sector;
 	bio_set_op_attrs(bio, dio->op, dio->op_flags);
@@ -782,9 +787,17 @@ static inline int dio_send_cur_page(struct dio *dio, struct dio_submit *sdio,
 		 * current logical offset in the file does not equal what would
 		 * be the next logical offset in the bio, submit the bio we
 		 * have.
+		 *
+		 * When fscrypt inline encryption is used, data unit number
+		 * (DUN) contiguity is also required.  Normally that's implied
+		 * by logical contiguity.  However, certain IV generation
+		 * methods (e.g. IV_INO_LBLK_32) don't guarantee it.  So, we
+		 * must explicitly check fscrypt_mergeable_bio() too.
 		 */
 		if (sdio->final_block_in_bio != sdio->cur_page_block ||
-		    cur_offset != bio_next_offset)
+		    cur_offset != bio_next_offset ||
+		    !fscrypt_mergeable_bio(sdio->bio, dio->inode,
+					   cur_offset >> dio->inode->i_blkbits))
 			dio_bio_submit(dio, sdio);
 	}
 
-- 
2.28.0.rc0.105.gf9edc3c819-goog


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH v4 3/7] iomap: support direct I/O with fscrypt using blk-crypto
  2020-07-20 23:37 [PATCH v4 0/7] add support for direct I/O with fscrypt using blk-crypto Satya Tangirala
  2020-07-20 23:37 ` [PATCH v4 1/7] fscrypt: Add functions for direct I/O support Satya Tangirala
  2020-07-20 23:37 ` [PATCH v4 2/7] direct-io: add support for fscrypt using blk-crypto Satya Tangirala
@ 2020-07-20 23:37 ` Satya Tangirala
  2020-07-22 17:06   ` Jaegeuk Kim
  2020-07-22 21:16   ` Dave Chinner
  2020-07-20 23:37 ` [PATCH v4 4/7] ext4: " Satya Tangirala
                   ` (4 subsequent siblings)
  7 siblings, 2 replies; 34+ messages in thread
From: Satya Tangirala @ 2020-07-20 23:37 UTC (permalink / raw)
  To: linux-fscrypt, linux-fsdevel, linux-f2fs-devel, linux-ext4
  Cc: linux-xfs, Eric Biggers, Satya Tangirala

From: Eric Biggers <ebiggers@google.com>

Wire up iomap direct I/O with the fscrypt additions for direct I/O.
This allows ext4 to support direct I/O on encrypted files when inline
encryption is enabled.

This change consists of two parts:

- Set a bio_crypt_ctx on bios for encrypted files, so that the file
  contents get encrypted (or decrypted).

- Ensure that encryption data unit numbers (DUNs) are contiguous within
  each bio.  Use the new function fscrypt_limit_io_pages() for this,
  since the iomap code works directly with logical ranges and thus
  doesn't have a chance to call fscrypt_mergeable_bio() on each page.

Note that fscrypt_limit_io_pages() is normally a no-op, as normally the
DUNs simply increment along with the logical blocks.  But it's needed to
handle an edge case in one of the fscrypt IV generation methods.

Signed-off-by: Eric Biggers <ebiggers@google.com>
Co-developed-by: Satya Tangirala <satyat@google.com>
Signed-off-by: Satya Tangirala <satyat@google.com>
---
 fs/iomap/direct-io.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index ec7b78e6feca..12064daa3e3d 100644
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -6,6 +6,7 @@
 #include <linux/module.h>
 #include <linux/compiler.h>
 #include <linux/fs.h>
+#include <linux/fscrypt.h>
 #include <linux/iomap.h>
 #include <linux/backing-dev.h>
 #include <linux/uio.h>
@@ -183,11 +184,16 @@ static void
 iomap_dio_zero(struct iomap_dio *dio, struct iomap *iomap, loff_t pos,
 		unsigned len)
 {
+	struct inode *inode = file_inode(dio->iocb->ki_filp);
 	struct page *page = ZERO_PAGE(0);
 	int flags = REQ_SYNC | REQ_IDLE;
 	struct bio *bio;
 
 	bio = bio_alloc(GFP_KERNEL, 1);
+
+	/* encrypted direct I/O is guaranteed to be fs-block aligned */
+	WARN_ON_ONCE(fscrypt_needs_contents_encryption(inode));
+
 	bio_set_dev(bio, iomap->bdev);
 	bio->bi_iter.bi_sector = iomap_sector(iomap, pos);
 	bio->bi_private = dio;
@@ -253,6 +259,7 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
 		ret = nr_pages;
 		goto out;
 	}
+	nr_pages = fscrypt_limit_io_pages(inode, pos, nr_pages);
 
 	if (need_zeroout) {
 		/* zero out from the start of the block to the write offset */
@@ -270,6 +277,8 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
 		}
 
 		bio = bio_alloc(GFP_KERNEL, nr_pages);
+		fscrypt_set_bio_crypt_ctx(bio, inode, pos >> inode->i_blkbits,
+					  GFP_KERNEL);
 		bio_set_dev(bio, iomap->bdev);
 		bio->bi_iter.bi_sector = iomap_sector(iomap, pos);
 		bio->bi_write_hint = dio->iocb->ki_hint;
@@ -306,9 +315,10 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
 		dio->size += n;
 		copied += n;
 
-		nr_pages = iov_iter_npages(dio->submit.iter, BIO_MAX_PAGES);
 		iomap_dio_submit_bio(dio, iomap, bio, pos);
 		pos += n;
+		nr_pages = iov_iter_npages(dio->submit.iter, BIO_MAX_PAGES);
+		nr_pages = fscrypt_limit_io_pages(inode, pos, nr_pages);
 	} while (nr_pages);
 
 	/*
-- 
2.28.0.rc0.105.gf9edc3c819-goog


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH v4 4/7] ext4: support direct I/O with fscrypt using blk-crypto
  2020-07-20 23:37 [PATCH v4 0/7] add support for direct I/O with fscrypt using blk-crypto Satya Tangirala
                   ` (2 preceding siblings ...)
  2020-07-20 23:37 ` [PATCH v4 3/7] iomap: support direct I/O with " Satya Tangirala
@ 2020-07-20 23:37 ` Satya Tangirala
  2020-07-22 17:07   ` Jaegeuk Kim
  2020-07-20 23:37 ` [PATCH v4 5/7] f2fs: " Satya Tangirala
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 34+ messages in thread
From: Satya Tangirala @ 2020-07-20 23:37 UTC (permalink / raw)
  To: linux-fscrypt, linux-fsdevel, linux-f2fs-devel, linux-ext4
  Cc: linux-xfs, Eric Biggers, Satya Tangirala

From: Eric Biggers <ebiggers@google.com>

Wire up ext4 with fscrypt direct I/O support. direct I/O with fscrypt is
only supported through blk-crypto (i.e. CONFIG_BLK_INLINE_ENCRYPTION must
have been enabled, the 'inlinecrypt' mount option must have been specified,
and either hardware inline encryption support must be present or
CONFIG_BLK_INLINE_ENCYRPTION_FALLBACK must have been enabled). Further,
direct I/O on encrypted files is only supported when I/O is aligned
to the filesystem block size (which is *not* necessarily the same as the
block device's block size).

Signed-off-by: Eric Biggers <ebiggers@google.com>
Co-developed-by: Satya Tangirala <satyat@google.com>
Signed-off-by: Satya Tangirala <satyat@google.com>
---
 fs/ext4/file.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 2a01e31a032c..d534f72675d9 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -36,9 +36,11 @@
 #include "acl.h"
 #include "truncate.h"
 
-static bool ext4_dio_supported(struct inode *inode)
+static bool ext4_dio_supported(struct kiocb *iocb, struct iov_iter *iter)
 {
-	if (IS_ENABLED(CONFIG_FS_ENCRYPTION) && IS_ENCRYPTED(inode))
+	struct inode *inode = file_inode(iocb->ki_filp);
+
+	if (!fscrypt_dio_supported(iocb, iter))
 		return false;
 	if (fsverity_active(inode))
 		return false;
@@ -61,7 +63,7 @@ static ssize_t ext4_dio_read_iter(struct kiocb *iocb, struct iov_iter *to)
 		inode_lock_shared(inode);
 	}
 
-	if (!ext4_dio_supported(inode)) {
+	if (!ext4_dio_supported(iocb, to)) {
 		inode_unlock_shared(inode);
 		/*
 		 * Fallback to buffered I/O if the operation being performed on
@@ -490,7 +492,7 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	}
 
 	/* Fallback to buffered I/O if the inode does not support direct I/O. */
-	if (!ext4_dio_supported(inode)) {
+	if (!ext4_dio_supported(iocb, from)) {
 		if (ilock_shared)
 			inode_unlock_shared(inode);
 		else
-- 
2.28.0.rc0.105.gf9edc3c819-goog


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH v4 5/7] f2fs: support direct I/O with fscrypt using blk-crypto
  2020-07-20 23:37 [PATCH v4 0/7] add support for direct I/O with fscrypt using blk-crypto Satya Tangirala
                   ` (3 preceding siblings ...)
  2020-07-20 23:37 ` [PATCH v4 4/7] ext4: " Satya Tangirala
@ 2020-07-20 23:37 ` 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
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 34+ messages in thread
From: Satya Tangirala @ 2020-07-20 23:37 UTC (permalink / raw)
  To: linux-fscrypt, linux-fsdevel, linux-f2fs-devel, linux-ext4
  Cc: linux-xfs, Eric Biggers, Satya Tangirala

From: Eric Biggers <ebiggers@google.com>

Wire up f2fs with fscrypt direct I/O support. direct I/O with fscrypt is
only supported through blk-crypto (i.e. CONFIG_BLK_INLINE_ENCRYPTION must
have been enabled, the 'inlinecrypt' mount option must have been specified,
and either hardware inline encryption support must be present or
CONFIG_BLK_INLINE_ENCYRPTION_FALLBACK must have been enabled). Further,
direct I/O on encrypted files is only supported when I/O is aligned
to the filesystem block size (which is *not* necessarily the same as the
block device's block size).

Signed-off-by: Eric Biggers <ebiggers@google.com>
Co-developed-by: Satya Tangirala <satyat@google.com>
Signed-off-by: Satya Tangirala <satyat@google.com>
---
 fs/f2fs/f2fs.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index b35a50f4953c..978130b5a195 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -4082,7 +4082,11 @@ static inline bool f2fs_force_buffered_io(struct inode *inode,
 	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
 	int rw = iov_iter_rw(iter);
 
-	if (f2fs_post_read_required(inode))
+	if (!fscrypt_dio_supported(iocb, iter))
+		return true;
+	if (fsverity_active(inode))
+		return true;
+	if (f2fs_compressed_file(inode))
 		return true;
 	if (f2fs_is_multi_device(sbi))
 		return true;
-- 
2.28.0.rc0.105.gf9edc3c819-goog


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH v4 6/7] fscrypt: document inline encryption support
  2020-07-20 23:37 [PATCH v4 0/7] add support for direct I/O with fscrypt using blk-crypto Satya Tangirala
                   ` (4 preceding siblings ...)
  2020-07-20 23:37 ` [PATCH v4 5/7] f2fs: " Satya Tangirala
@ 2020-07-20 23:37 ` 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:56 ` [PATCH v4 0/7] add support for direct I/O with fscrypt using blk-crypto Eric Biggers
  7 siblings, 1 reply; 34+ messages in thread
From: Satya Tangirala @ 2020-07-20 23:37 UTC (permalink / raw)
  To: linux-fscrypt, linux-fsdevel, linux-f2fs-devel, linux-ext4
  Cc: linux-xfs, Satya Tangirala, Eric Biggers

Update the fscrypt documentation file for inline encryption support.

Signed-off-by: Satya Tangirala <satyat@google.com>
Reviewed-by: Eric Biggers <ebiggers@google.com>
---
 Documentation/filesystems/fscrypt.rst | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst
index f5d8b0303ddf..ec81598477fc 100644
--- a/Documentation/filesystems/fscrypt.rst
+++ b/Documentation/filesystems/fscrypt.rst
@@ -1204,6 +1204,18 @@ buffer.  Some filesystems, such as UBIFS, already use temporary
 buffers regardless of encryption.  Other filesystems, such as ext4 and
 F2FS, have to allocate bounce pages specially for encryption.
 
+Fscrypt is also able to use inline encryption hardware instead of the
+kernel crypto API for en/decryption of file contents.  When possible,
+and if directed to do so (by specifying the 'inlinecrypt' mount option
+for an ext4/F2FS filesystem), it adds encryption contexts to bios and
+uses blk-crypto to perform the en/decryption instead of making use of
+the above read/write path changes.  Of course, even if directed to
+make use of inline encryption, fscrypt will only be able to do so if
+either hardware inline encryption support is available for the
+selected encryption algorithm or CONFIG_BLK_INLINE_ENCRYPTION_FALLBACK
+is selected.  If neither is the case, fscrypt will fall back to using
+the above mentioned read/write path changes for en/decryption.
+
 Filename hashing and encoding
 -----------------------------
 
@@ -1250,7 +1262,9 @@ Tests
 
 To test fscrypt, use xfstests, which is Linux's de facto standard
 filesystem test suite.  First, run all the tests in the "encrypt"
-group on the relevant filesystem(s).  For example, to test ext4 and
+group on the relevant filesystem(s).  One can also run the tests
+with the 'inlinecrypt' mount option to test the implementation for
+inline encryption support.  For example, to test ext4 and
 f2fs encryption using `kvm-xfstests
 <https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quickstart.md>`_::
 
-- 
2.28.0.rc0.105.gf9edc3c819-goog


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH v4 7/7] fscrypt: update documentation for direct I/O support
  2020-07-20 23:37 [PATCH v4 0/7] add support for direct I/O with fscrypt using blk-crypto Satya Tangirala
                   ` (5 preceding siblings ...)
  2020-07-20 23:37 ` [PATCH v4 6/7] fscrypt: document inline encryption support Satya Tangirala
@ 2020-07-20 23:37 ` Satya Tangirala
  2020-07-21  0:47   ` Eric Biggers
  2020-07-21  0:56 ` [PATCH v4 0/7] add support for direct I/O with fscrypt using blk-crypto Eric Biggers
  7 siblings, 1 reply; 34+ messages in thread
From: Satya Tangirala @ 2020-07-20 23:37 UTC (permalink / raw)
  To: linux-fscrypt, linux-fsdevel, linux-f2fs-devel, linux-ext4
  Cc: linux-xfs, Satya Tangirala

Update fscrypt documentation to reflect the addition of direct I/O support
and document the necessary conditions for direct I/O on encrypted files.

Signed-off-by: Satya Tangirala <satyat@google.com>
---
 Documentation/filesystems/fscrypt.rst | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst
index ec81598477fc..5367c03b17bb 100644
--- a/Documentation/filesystems/fscrypt.rst
+++ b/Documentation/filesystems/fscrypt.rst
@@ -1049,8 +1049,10 @@ astute users may notice some differences in behavior:
   may be used to overwrite the source files but isn't guaranteed to be
   effective on all filesystems and storage devices.
 
-- Direct I/O is not supported on encrypted files.  Attempts to use
-  direct I/O on such files will fall back to buffered I/O.
+- Direct I/O is supported on encrypted files only under some
+  circumstances (see `Direct I/O support`_ for details). When these
+  circumstances are not met, attempts to use direct I/O on encrypted
+  files will fall back to buffered I/O.
 
 - The fallocate operations FALLOC_FL_COLLAPSE_RANGE and
   FALLOC_FL_INSERT_RANGE are not supported on encrypted files and will
@@ -1123,6 +1125,20 @@ It is not currently possible to backup and restore encrypted files
 without the encryption key.  This would require special APIs which
 have not yet been implemented.
 
+Direct I/O support
+==================
+
+Direct I/O on encrypted files is supported through blk-crypto. In
+particular, this means the kernel must have CONFIG_BLK_INLINE_ENCRYPTION
+enabled, the filesystem must have had the 'inlinecrypt' mount option
+specified, and either hardware inline encryption must be present, or
+CONFIG_BLK_INLINE_ENCRYPTION_FALLBACK must have been enabled. Further,
+any I/O must be aligned to the filesystem block size (*not* necessarily
+the same as the block device's block size) - in particular, any userspace
+buffer into which data is read/written from must also be aligned to the
+filesystem block size. If any of these conditions isn't met, attempts to do
+direct I/O on an encrypted file will fall back to buffered I/O.
+
 Encryption policy enforcement
 =============================
 
-- 
2.28.0.rc0.105.gf9edc3c819-goog


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 7/7] fscrypt: update documentation for direct I/O support
  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
  0 siblings, 1 reply; 34+ messages in thread
From: Eric Biggers @ 2020-07-21  0:47 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: linux-fscrypt, linux-fsdevel, linux-f2fs-devel, linux-ext4, linux-xfs

On Mon, Jul 20, 2020 at 11:37:39PM +0000, Satya Tangirala wrote:
> Update fscrypt documentation to reflect the addition of direct I/O support
> and document the necessary conditions for direct I/O on encrypted files.
> 
> Signed-off-by: Satya Tangirala <satyat@google.com>

Reviewed-by: Eric Biggers <ebiggers@google.com>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 0/7] add support for direct I/O with fscrypt using blk-crypto
  2020-07-20 23:37 [PATCH v4 0/7] add support for direct I/O with fscrypt using blk-crypto Satya Tangirala
                   ` (6 preceding siblings ...)
  2020-07-20 23:37 ` [PATCH v4 7/7] fscrypt: update documentation for direct I/O support Satya Tangirala
@ 2020-07-21  0:56 ` Eric Biggers
  7 siblings, 0 replies; 34+ messages in thread
From: Eric Biggers @ 2020-07-21  0:56 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: linux-fscrypt, linux-fsdevel, linux-f2fs-devel, linux-ext4, linux-xfs

On Mon, Jul 20, 2020 at 11:37:32PM +0000, Satya Tangirala wrote:
> This patch series adds support for direct I/O with fscrypt using
> blk-crypto. It has been rebased on fscrypt/master.
> 
> Patch 1 adds two functions to fscrypt that need to be called to determine
> if direct I/O is supported for a request.
> 
> Patches 2 and 3 wire up direct-io and iomap respectively with the functions
> introduced in Patch 1 and set bio crypt contexts on bios when appropriate
> by calling into fscrypt.
> 
> Patches 4 and 5 allow ext4 and f2fs direct I/O to support fscrypt without
> falling back to buffered I/O.
> 
> Patches 6 and 7 update the fscrypt documentation for inline encryption
> support and direct I/O. The documentation now notes the required conditions
> for inline encryption and direct I/O on encrypted files.
> 
> This patch series was tested by running xfstests with test_dummy_encryption
> with and without the 'inlinecrypt' mount option, and there were no
> meaningful regressions. One regression was for generic/587 on ext4,
> but that test isn't compatible with test_dummy_encryption in the first
> place, and the test "incorrectly" passes without the 'inlinecrypt' mount
> option - a patch will be sent out to exclude that test when
> test_dummy_encryption is turned on with ext4 (like the other quota related
> tests that use user visible quota files). The other regression was for
> generic/252 on ext4, which does direct I/O with a buffer aligned to the
> block device's blocksize, but not necessarily aligned to the filesystem's
> block size, which direct I/O with fscrypt requires.
> 

This patch series looks good to me now.  Can the ext4, f2fs, and iomap
maintainers take a look?

- Eric

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 5/7] f2fs: support direct I/O with fscrypt using blk-crypto
  2020-07-20 23:37 ` [PATCH v4 5/7] f2fs: " Satya Tangirala
@ 2020-07-21 20:11   ` Jaegeuk Kim
  0 siblings, 0 replies; 34+ messages in thread
From: Jaegeuk Kim @ 2020-07-21 20:11 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: linux-fscrypt, linux-fsdevel, linux-f2fs-devel, linux-ext4,
	linux-xfs, Eric Biggers

On 07/20, Satya Tangirala wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Wire up f2fs with fscrypt direct I/O support. direct I/O with fscrypt is
> only supported through blk-crypto (i.e. CONFIG_BLK_INLINE_ENCRYPTION must
> have been enabled, the 'inlinecrypt' mount option must have been specified,
> and either hardware inline encryption support must be present or
> CONFIG_BLK_INLINE_ENCYRPTION_FALLBACK must have been enabled). Further,
> direct I/O on encrypted files is only supported when I/O is aligned
> to the filesystem block size (which is *not* necessarily the same as the
> block device's block size).
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> Co-developed-by: Satya Tangirala <satyat@google.com>
> Signed-off-by: Satya Tangirala <satyat@google.com>

Acked-by: Jaegeuk Kim <jaegeuk@kernel.org>

> ---
>  fs/f2fs/f2fs.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index b35a50f4953c..978130b5a195 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -4082,7 +4082,11 @@ static inline bool f2fs_force_buffered_io(struct inode *inode,
>  	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>  	int rw = iov_iter_rw(iter);
>  
> -	if (f2fs_post_read_required(inode))
> +	if (!fscrypt_dio_supported(iocb, iter))
> +		return true;
> +	if (fsverity_active(inode))
> +		return true;
> +	if (f2fs_compressed_file(inode))
>  		return true;
>  	if (f2fs_is_multi_device(sbi))
>  		return true;
> -- 
> 2.28.0.rc0.105.gf9edc3c819-goog

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 7/7] fscrypt: update documentation for direct I/O support
  2020-07-21  0:47   ` Eric Biggers
@ 2020-07-22 16:57     ` Jaegeuk Kim
  0 siblings, 0 replies; 34+ messages in thread
From: Jaegeuk Kim @ 2020-07-22 16:57 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Satya Tangirala, linux-fscrypt, linux-fsdevel, linux-f2fs-devel,
	linux-ext4, linux-xfs

On 07/20, Eric Biggers wrote:
> On Mon, Jul 20, 2020 at 11:37:39PM +0000, Satya Tangirala wrote:
> > Update fscrypt documentation to reflect the addition of direct I/O support
> > and document the necessary conditions for direct I/O on encrypted files.
> > 
> > Signed-off-by: Satya Tangirala <satyat@google.com>
> 
> Reviewed-by: Eric Biggers <ebiggers@google.com>

Reviewed-by: Jaegeuk Kim <jaegeuk@kernel.org>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 6/7] fscrypt: document inline encryption support
  2020-07-20 23:37 ` [PATCH v4 6/7] fscrypt: document inline encryption support Satya Tangirala
@ 2020-07-22 17:01   ` Jaegeuk Kim
  0 siblings, 0 replies; 34+ messages in thread
From: Jaegeuk Kim @ 2020-07-22 17:01 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: linux-fscrypt, linux-fsdevel, linux-f2fs-devel, linux-ext4,
	linux-xfs, Eric Biggers

On 07/20, Satya Tangirala wrote:
> Update the fscrypt documentation file for inline encryption support.
> 
> Signed-off-by: Satya Tangirala <satyat@google.com>
> Reviewed-by: Eric Biggers <ebiggers@google.com>

Reviewed-by: Jaegeuk Kim <jaegeuk@kernel.org>

> ---
>  Documentation/filesystems/fscrypt.rst | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst
> index f5d8b0303ddf..ec81598477fc 100644
> --- a/Documentation/filesystems/fscrypt.rst
> +++ b/Documentation/filesystems/fscrypt.rst
> @@ -1204,6 +1204,18 @@ buffer.  Some filesystems, such as UBIFS, already use temporary
>  buffers regardless of encryption.  Other filesystems, such as ext4 and
>  F2FS, have to allocate bounce pages specially for encryption.
>  
> +Fscrypt is also able to use inline encryption hardware instead of the
> +kernel crypto API for en/decryption of file contents.  When possible,
> +and if directed to do so (by specifying the 'inlinecrypt' mount option
> +for an ext4/F2FS filesystem), it adds encryption contexts to bios and
> +uses blk-crypto to perform the en/decryption instead of making use of
> +the above read/write path changes.  Of course, even if directed to
> +make use of inline encryption, fscrypt will only be able to do so if
> +either hardware inline encryption support is available for the
> +selected encryption algorithm or CONFIG_BLK_INLINE_ENCRYPTION_FALLBACK
> +is selected.  If neither is the case, fscrypt will fall back to using
> +the above mentioned read/write path changes for en/decryption.
> +
>  Filename hashing and encoding
>  -----------------------------
>  
> @@ -1250,7 +1262,9 @@ Tests
>  
>  To test fscrypt, use xfstests, which is Linux's de facto standard
>  filesystem test suite.  First, run all the tests in the "encrypt"
> -group on the relevant filesystem(s).  For example, to test ext4 and
> +group on the relevant filesystem(s).  One can also run the tests
> +with the 'inlinecrypt' mount option to test the implementation for
> +inline encryption support.  For example, to test ext4 and
>  f2fs encryption using `kvm-xfstests
>  <https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quickstart.md>`_::
>  
> -- 
> 2.28.0.rc0.105.gf9edc3c819-goog

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 1/7] fscrypt: Add functions for direct I/O support
  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
  0 siblings, 0 replies; 34+ messages in thread
From: Jaegeuk Kim @ 2020-07-22 17:04 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: linux-fscrypt, linux-fsdevel, linux-f2fs-devel, linux-ext4,
	linux-xfs, Eric Biggers

On 07/20, Satya Tangirala wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Introduce fscrypt_dio_supported() to check whether a direct I/O request
> is unsupported due to encryption constraints.
> 
> Also introduce fscrypt_limit_io_pages() to limit how many pages can be
> added to a bio being prepared for direct I/O. This is needed for the
> iomap direct I/O implementation to avoid DUN wraparound in the middle of
> a bio (which is possible with the IV_INO_LBLK_32 IV generation method).
> Elsewhere fscrypt_mergeable_bio() is used for this, but iomap operates
> on logical ranges directly and doesn't have a chance to call
> fscrypt_mergeable_bio() on every block or page. So we need this function
> which limits a logical range in one go.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> Co-developed-by: Satya Tangirala <satyat@google.com>
> Signed-off-by: Satya Tangirala <satyat@google.com>

Reviewed-by: Jaegeuk Kim <jaegeuk@kernel.org>

> ---
>  fs/crypto/crypto.c       |  8 ++++
>  fs/crypto/inline_crypt.c | 82 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/fscrypt.h  | 19 ++++++++++
>  3 files changed, 109 insertions(+)
> 
> diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
> index a52cf32733ab..fb34364360b3 100644
> --- a/fs/crypto/crypto.c
> +++ b/fs/crypto/crypto.c
> @@ -69,6 +69,14 @@ void fscrypt_free_bounce_page(struct page *bounce_page)
>  }
>  EXPORT_SYMBOL(fscrypt_free_bounce_page);
>  
> +/*
> + * Generate the IV for the given logical block number within the given file.
> + * For filenames encryption, lblk_num == 0.
> + *
> + * Keep this in sync with fscrypt_limit_io_pages().  fscrypt_limit_io_pages()
> + * needs to know about any IV generation methods where the low bits of IV don't
> + * simply contain the lblk_num (e.g., IV_INO_LBLK_32).
> + */
>  void fscrypt_generate_iv(union fscrypt_iv *iv, u64 lblk_num,
>  			 const struct fscrypt_info *ci)
>  {
> diff --git a/fs/crypto/inline_crypt.c b/fs/crypto/inline_crypt.c
> index d7aecadf33c1..578739712e00 100644
> --- a/fs/crypto/inline_crypt.c
> +++ b/fs/crypto/inline_crypt.c
> @@ -16,6 +16,7 @@
>  #include <linux/blkdev.h>
>  #include <linux/buffer_head.h>
>  #include <linux/sched/mm.h>
> +#include <linux/uio.h>
>  
>  #include "fscrypt_private.h"
>  
> @@ -362,3 +363,84 @@ bool fscrypt_mergeable_bio_bh(struct bio *bio,
>  	return fscrypt_mergeable_bio(bio, inode, next_lblk);
>  }
>  EXPORT_SYMBOL_GPL(fscrypt_mergeable_bio_bh);
> +
> +/**
> + * fscrypt_dio_supported() - check whether a direct I/O request is unsupported
> + *			     due to encryption constraints
> + * @iocb: the file and position the I/O is targeting
> + * @iter: the I/O data segment(s)
> + *
> + * Return: true if direct I/O is supported
> + */
> +bool fscrypt_dio_supported(struct kiocb *iocb, struct iov_iter *iter)
> +{
> +	const struct inode *inode = file_inode(iocb->ki_filp);
> +	const unsigned int blocksize = i_blocksize(inode);
> +
> +	/* If the file is unencrypted, no veto from us. */
> +	if (!fscrypt_needs_contents_encryption(inode))
> +		return true;
> +
> +	/* We only support direct I/O with inline crypto, not fs-layer crypto */
> +	if (!fscrypt_inode_uses_inline_crypto(inode))
> +		return false;
> +
> +	/*
> +	 * Since the granularity of encryption is filesystem blocks, the I/O
> +	 * must be block aligned -- not just disk sector aligned.
> +	 */
> +	if (!IS_ALIGNED(iocb->ki_pos | iov_iter_alignment(iter), blocksize))
> +		return false;
> +
> +	return true;
> +}
> +EXPORT_SYMBOL_GPL(fscrypt_dio_supported);
> +
> +/**
> + * fscrypt_limit_io_pages() - limit I/O pages to avoid discontiguous DUNs
> + * @inode: the file on which I/O is being done
> + * @pos: the file position (in bytes) at which the I/O is being done
> + * @nr_pages: the number of pages we want to submit starting at @pos
> + *
> + * Determine the limit to the number of pages that can be submitted in the bio
> + * targeting @pos without causing a data unit number (DUN) discontinuity.
> + *
> + * This is normally just @nr_pages, as normally the DUNs just increment along
> + * with the logical blocks.  (Or the file is not encrypted.)
> + *
> + * In rare cases, fscrypt can be using an IV generation method that allows the
> + * DUN to wrap around within logically continuous blocks, and that wraparound
> + * will occur.  If this happens, a value less than @nr_pages will be returned so
> + * that the wraparound doesn't occur in the middle of the bio.  Note that we
> + * only support block_size == PAGE_SIZE (and page-aligned DIO) in such cases.
> + *
> + * Return: the actual number of pages that can be submitted
> + */
> +int fscrypt_limit_io_pages(const struct inode *inode, loff_t pos, int nr_pages)
> +{
> +	const struct fscrypt_info *ci = inode->i_crypt_info;
> +	u32 dun;
> +
> +	if (!fscrypt_inode_uses_inline_crypto(inode))
> +		return nr_pages;
> +
> +	if (nr_pages <= 1)
> +		return nr_pages;
> +
> +	if (!(fscrypt_policy_flags(&ci->ci_policy) &
> +	      FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32))
> +		return nr_pages;
> +
> +	/*
> +	 * fscrypt_select_encryption_impl() ensures that block_size == PAGE_SIZE
> +	 * when using FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32.
> +	 */
> +	if (WARN_ON_ONCE(i_blocksize(inode) != PAGE_SIZE))
> +		return 1;
> +
> +	/* With IV_INO_LBLK_32, the DUN can wrap around from U32_MAX to 0. */
> +
> +	dun = ci->ci_hashed_ino + (pos >> inode->i_blkbits);
> +
> +	return min_t(u64, nr_pages, (u64)U32_MAX + 1 - dun);
> +}
> diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
> index bb257411365f..c205c214b35e 100644
> --- a/include/linux/fscrypt.h
> +++ b/include/linux/fscrypt.h
> @@ -559,6 +559,11 @@ bool fscrypt_mergeable_bio(struct bio *bio, const struct inode *inode,
>  bool fscrypt_mergeable_bio_bh(struct bio *bio,
>  			      const struct buffer_head *next_bh);
>  
> +bool fscrypt_dio_supported(struct kiocb *iocb, struct iov_iter *iter);
> +
> +int fscrypt_limit_io_pages(const struct inode *inode, loff_t pos,
> +			   int nr_pages);
> +
>  #else /* CONFIG_FS_ENCRYPTION_INLINE_CRYPT */
>  
>  static inline bool __fscrypt_inode_uses_inline_crypto(const struct inode *inode)
> @@ -587,6 +592,20 @@ static inline bool fscrypt_mergeable_bio_bh(struct bio *bio,
>  {
>  	return true;
>  }
> +
> +static inline bool fscrypt_dio_supported(struct kiocb *iocb,
> +					 struct iov_iter *iter)
> +{
> +	const struct inode *inode = file_inode(iocb->ki_filp);
> +
> +	return !fscrypt_needs_contents_encryption(inode);
> +}
> +
> +static inline int fscrypt_limit_io_pages(const struct inode *inode, loff_t pos,
> +					 int nr_pages)
> +{
> +	return nr_pages;
> +}
>  #endif /* !CONFIG_FS_ENCRYPTION_INLINE_CRYPT */
>  
>  /**
> -- 
> 2.28.0.rc0.105.gf9edc3c819-goog

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 2/7] direct-io: add support for fscrypt using blk-crypto
  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
  0 siblings, 0 replies; 34+ messages in thread
From: Jaegeuk Kim @ 2020-07-22 17:05 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: linux-fscrypt, linux-fsdevel, linux-f2fs-devel, linux-ext4,
	linux-xfs, Eric Biggers

On 07/20, Satya Tangirala wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Set bio crypt contexts on bios by calling into fscrypt when required,
> and explicitly check for DUN continuity when adding pages to the bio.
> (While DUN continuity is usually implied by logical block contiguity,
> this is not the case when using certain fscrypt IV generation methods
> like IV_INO_LBLK_32).
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> Co-developed-by: Satya Tangirala <satyat@google.com>
> Signed-off-by: Satya Tangirala <satyat@google.com>

Reviwed-by: Jaegeuk Kim <jaegeuk@kernel.org>

> ---
>  fs/direct-io.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/direct-io.c b/fs/direct-io.c
> index 6d5370eac2a8..f27f7e3780ee 100644
> --- a/fs/direct-io.c
> +++ b/fs/direct-io.c
> @@ -24,6 +24,7 @@
>  #include <linux/module.h>
>  #include <linux/types.h>
>  #include <linux/fs.h>
> +#include <linux/fscrypt.h>
>  #include <linux/mm.h>
>  #include <linux/slab.h>
>  #include <linux/highmem.h>
> @@ -411,6 +412,7 @@ dio_bio_alloc(struct dio *dio, struct dio_submit *sdio,
>  	      sector_t first_sector, int nr_vecs)
>  {
>  	struct bio *bio;
> +	struct inode *inode = dio->inode;
>  
>  	/*
>  	 * bio_alloc() is guaranteed to return a bio when allowed to sleep and
> @@ -418,6 +420,9 @@ dio_bio_alloc(struct dio *dio, struct dio_submit *sdio,
>  	 */
>  	bio = bio_alloc(GFP_KERNEL, nr_vecs);
>  
> +	fscrypt_set_bio_crypt_ctx(bio, inode,
> +				  sdio->cur_page_fs_offset >> inode->i_blkbits,
> +				  GFP_KERNEL);
>  	bio_set_dev(bio, bdev);
>  	bio->bi_iter.bi_sector = first_sector;
>  	bio_set_op_attrs(bio, dio->op, dio->op_flags);
> @@ -782,9 +787,17 @@ static inline int dio_send_cur_page(struct dio *dio, struct dio_submit *sdio,
>  		 * current logical offset in the file does not equal what would
>  		 * be the next logical offset in the bio, submit the bio we
>  		 * have.
> +		 *
> +		 * When fscrypt inline encryption is used, data unit number
> +		 * (DUN) contiguity is also required.  Normally that's implied
> +		 * by logical contiguity.  However, certain IV generation
> +		 * methods (e.g. IV_INO_LBLK_32) don't guarantee it.  So, we
> +		 * must explicitly check fscrypt_mergeable_bio() too.
>  		 */
>  		if (sdio->final_block_in_bio != sdio->cur_page_block ||
> -		    cur_offset != bio_next_offset)
> +		    cur_offset != bio_next_offset ||
> +		    !fscrypt_mergeable_bio(sdio->bio, dio->inode,
> +					   cur_offset >> dio->inode->i_blkbits))
>  			dio_bio_submit(dio, sdio);
>  	}
>  
> -- 
> 2.28.0.rc0.105.gf9edc3c819-goog

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 3/7] iomap: support direct I/O with fscrypt using blk-crypto
  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
  1 sibling, 0 replies; 34+ messages in thread
From: Jaegeuk Kim @ 2020-07-22 17:06 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: linux-fscrypt, linux-fsdevel, linux-f2fs-devel, linux-ext4,
	linux-xfs, Eric Biggers

On 07/20, Satya Tangirala wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Wire up iomap direct I/O with the fscrypt additions for direct I/O.
> This allows ext4 to support direct I/O on encrypted files when inline
> encryption is enabled.
> 
> This change consists of two parts:
> 
> - Set a bio_crypt_ctx on bios for encrypted files, so that the file
>   contents get encrypted (or decrypted).
> 
> - Ensure that encryption data unit numbers (DUNs) are contiguous within
>   each bio.  Use the new function fscrypt_limit_io_pages() for this,
>   since the iomap code works directly with logical ranges and thus
>   doesn't have a chance to call fscrypt_mergeable_bio() on each page.
> 
> Note that fscrypt_limit_io_pages() is normally a no-op, as normally the
> DUNs simply increment along with the logical blocks.  But it's needed to
> handle an edge case in one of the fscrypt IV generation methods.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> Co-developed-by: Satya Tangirala <satyat@google.com>
> Signed-off-by: Satya Tangirala <satyat@google.com>

Reviewed-by: Jaegeuk Kim <jaegeuk@kernel.org>

> ---
>  fs/iomap/direct-io.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> index ec7b78e6feca..12064daa3e3d 100644
> --- a/fs/iomap/direct-io.c
> +++ b/fs/iomap/direct-io.c
> @@ -6,6 +6,7 @@
>  #include <linux/module.h>
>  #include <linux/compiler.h>
>  #include <linux/fs.h>
> +#include <linux/fscrypt.h>
>  #include <linux/iomap.h>
>  #include <linux/backing-dev.h>
>  #include <linux/uio.h>
> @@ -183,11 +184,16 @@ static void
>  iomap_dio_zero(struct iomap_dio *dio, struct iomap *iomap, loff_t pos,
>  		unsigned len)
>  {
> +	struct inode *inode = file_inode(dio->iocb->ki_filp);
>  	struct page *page = ZERO_PAGE(0);
>  	int flags = REQ_SYNC | REQ_IDLE;
>  	struct bio *bio;
>  
>  	bio = bio_alloc(GFP_KERNEL, 1);
> +
> +	/* encrypted direct I/O is guaranteed to be fs-block aligned */
> +	WARN_ON_ONCE(fscrypt_needs_contents_encryption(inode));
> +
>  	bio_set_dev(bio, iomap->bdev);
>  	bio->bi_iter.bi_sector = iomap_sector(iomap, pos);
>  	bio->bi_private = dio;
> @@ -253,6 +259,7 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
>  		ret = nr_pages;
>  		goto out;
>  	}
> +	nr_pages = fscrypt_limit_io_pages(inode, pos, nr_pages);
>  
>  	if (need_zeroout) {
>  		/* zero out from the start of the block to the write offset */
> @@ -270,6 +277,8 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
>  		}
>  
>  		bio = bio_alloc(GFP_KERNEL, nr_pages);
> +		fscrypt_set_bio_crypt_ctx(bio, inode, pos >> inode->i_blkbits,
> +					  GFP_KERNEL);
>  		bio_set_dev(bio, iomap->bdev);
>  		bio->bi_iter.bi_sector = iomap_sector(iomap, pos);
>  		bio->bi_write_hint = dio->iocb->ki_hint;
> @@ -306,9 +315,10 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
>  		dio->size += n;
>  		copied += n;
>  
> -		nr_pages = iov_iter_npages(dio->submit.iter, BIO_MAX_PAGES);
>  		iomap_dio_submit_bio(dio, iomap, bio, pos);
>  		pos += n;
> +		nr_pages = iov_iter_npages(dio->submit.iter, BIO_MAX_PAGES);
> +		nr_pages = fscrypt_limit_io_pages(inode, pos, nr_pages);
>  	} while (nr_pages);
>  
>  	/*
> -- 
> 2.28.0.rc0.105.gf9edc3c819-goog

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 4/7] ext4: support direct I/O with fscrypt using blk-crypto
  2020-07-20 23:37 ` [PATCH v4 4/7] ext4: " Satya Tangirala
@ 2020-07-22 17:07   ` Jaegeuk Kim
  0 siblings, 0 replies; 34+ messages in thread
From: Jaegeuk Kim @ 2020-07-22 17:07 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: linux-fscrypt, linux-fsdevel, linux-f2fs-devel, linux-ext4,
	linux-xfs, Eric Biggers

On 07/20, Satya Tangirala wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Wire up ext4 with fscrypt direct I/O support. direct I/O with fscrypt is
> only supported through blk-crypto (i.e. CONFIG_BLK_INLINE_ENCRYPTION must
> have been enabled, the 'inlinecrypt' mount option must have been specified,
> and either hardware inline encryption support must be present or
> CONFIG_BLK_INLINE_ENCYRPTION_FALLBACK must have been enabled). Further,
> direct I/O on encrypted files is only supported when I/O is aligned
> to the filesystem block size (which is *not* necessarily the same as the
> block device's block size).
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> Co-developed-by: Satya Tangirala <satyat@google.com>
> Signed-off-by: Satya Tangirala <satyat@google.com>

Reviewed-by: Jaegeuk Kim <jaegeuk@kernel.org>

> ---
>  fs/ext4/file.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 2a01e31a032c..d534f72675d9 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -36,9 +36,11 @@
>  #include "acl.h"
>  #include "truncate.h"
>  
> -static bool ext4_dio_supported(struct inode *inode)
> +static bool ext4_dio_supported(struct kiocb *iocb, struct iov_iter *iter)
>  {
> -	if (IS_ENABLED(CONFIG_FS_ENCRYPTION) && IS_ENCRYPTED(inode))
> +	struct inode *inode = file_inode(iocb->ki_filp);
> +
> +	if (!fscrypt_dio_supported(iocb, iter))
>  		return false;
>  	if (fsverity_active(inode))
>  		return false;
> @@ -61,7 +63,7 @@ static ssize_t ext4_dio_read_iter(struct kiocb *iocb, struct iov_iter *to)
>  		inode_lock_shared(inode);
>  	}
>  
> -	if (!ext4_dio_supported(inode)) {
> +	if (!ext4_dio_supported(iocb, to)) {
>  		inode_unlock_shared(inode);
>  		/*
>  		 * Fallback to buffered I/O if the operation being performed on
> @@ -490,7 +492,7 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)
>  	}
>  
>  	/* Fallback to buffered I/O if the inode does not support direct I/O. */
> -	if (!ext4_dio_supported(inode)) {
> +	if (!ext4_dio_supported(iocb, from)) {
>  		if (ilock_shared)
>  			inode_unlock_shared(inode);
>  		else
> -- 
> 2.28.0.rc0.105.gf9edc3c819-goog

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 3/7] iomap: support direct I/O with fscrypt using blk-crypto
  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
  1 sibling, 1 reply; 34+ messages in thread
From: Dave Chinner @ 2020-07-22 21:16 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: linux-fscrypt, linux-fsdevel, linux-f2fs-devel, linux-ext4,
	linux-xfs, Eric Biggers

On Mon, Jul 20, 2020 at 11:37:35PM +0000, Satya Tangirala wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Wire up iomap direct I/O with the fscrypt additions for direct I/O.
> This allows ext4 to support direct I/O on encrypted files when inline
> encryption is enabled.
> 
> This change consists of two parts:
> 
> - Set a bio_crypt_ctx on bios for encrypted files, so that the file
>   contents get encrypted (or decrypted).
> 
> - Ensure that encryption data unit numbers (DUNs) are contiguous within
>   each bio.  Use the new function fscrypt_limit_io_pages() for this,
>   since the iomap code works directly with logical ranges and thus
>   doesn't have a chance to call fscrypt_mergeable_bio() on each page.
> 
> Note that fscrypt_limit_io_pages() is normally a no-op, as normally the
> DUNs simply increment along with the logical blocks.  But it's needed to
> handle an edge case in one of the fscrypt IV generation methods.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> Co-developed-by: Satya Tangirala <satyat@google.com>
> Signed-off-by: Satya Tangirala <satyat@google.com>
> ---
>  fs/iomap/direct-io.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> index ec7b78e6feca..12064daa3e3d 100644
> --- a/fs/iomap/direct-io.c
> +++ b/fs/iomap/direct-io.c
> @@ -6,6 +6,7 @@
>  #include <linux/module.h>
>  #include <linux/compiler.h>
>  #include <linux/fs.h>
> +#include <linux/fscrypt.h>
>  #include <linux/iomap.h>
>  #include <linux/backing-dev.h>
>  #include <linux/uio.h>
> @@ -183,11 +184,16 @@ static void
>  iomap_dio_zero(struct iomap_dio *dio, struct iomap *iomap, loff_t pos,
>  		unsigned len)
>  {
> +	struct inode *inode = file_inode(dio->iocb->ki_filp);
>  	struct page *page = ZERO_PAGE(0);
>  	int flags = REQ_SYNC | REQ_IDLE;
>  	struct bio *bio;
>  
>  	bio = bio_alloc(GFP_KERNEL, 1);
> +
> +	/* encrypted direct I/O is guaranteed to be fs-block aligned */
> +	WARN_ON_ONCE(fscrypt_needs_contents_encryption(inode));

Which means you are now placing a new constraint on this code in
that we cannot ever, in future, zero entire blocks here.

This code can issue arbitrary sized zeroing bios - multiple entire fs blocks
blocks if necessary - so I think constraining it to only support
partial block zeroing by adding a warning like this is no correct.

> @@ -253,6 +259,7 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
>  		ret = nr_pages;
>  		goto out;
>  	}
> +	nr_pages = fscrypt_limit_io_pages(inode, pos, nr_pages);

So if "pos" overlaps a 2^32 offset when a certain mode is used, we
have to break up the IO?

If true, I'm not sure that this belongs here. Limiting the size of
the IOs because of filesytem contraints belongs in the filesystem
extent mapping code. That's the point where the IO is broken up into
maximally sized chunks the filesystem can issue as a contiguous
range. If the fscrypt code is breaking that "contiguous IO range"
because of the mode being used, the fs mapping code should break
the mapping at the boundery where the IO needs to be broken.

Hence the dio mapping code here will never build IOs that cross this
-filesystem- encryption limitation, and we don't need this fscrypt
code in the direct IO path at all.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 3/7] iomap: support direct I/O with fscrypt using blk-crypto
  2020-07-22 21:16   ` Dave Chinner
@ 2020-07-22 22:34     ` Eric Biggers
  2020-07-22 22:44       ` Matthew Wilcox
                         ` (2 more replies)
  0 siblings, 3 replies; 34+ messages in thread
From: Eric Biggers @ 2020-07-22 22:34 UTC (permalink / raw)
  To: Dave Chinner
  Cc: Satya Tangirala, linux-fscrypt, linux-fsdevel, linux-f2fs-devel,
	linux-ext4, linux-xfs

On Thu, Jul 23, 2020 at 07:16:29AM +1000, Dave Chinner wrote:
> On Mon, Jul 20, 2020 at 11:37:35PM +0000, Satya Tangirala wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > Wire up iomap direct I/O with the fscrypt additions for direct I/O.
> > This allows ext4 to support direct I/O on encrypted files when inline
> > encryption is enabled.
> > 
> > This change consists of two parts:
> > 
> > - Set a bio_crypt_ctx on bios for encrypted files, so that the file
> >   contents get encrypted (or decrypted).
> > 
> > - Ensure that encryption data unit numbers (DUNs) are contiguous within
> >   each bio.  Use the new function fscrypt_limit_io_pages() for this,
> >   since the iomap code works directly with logical ranges and thus
> >   doesn't have a chance to call fscrypt_mergeable_bio() on each page.
> > 
> > Note that fscrypt_limit_io_pages() is normally a no-op, as normally the
> > DUNs simply increment along with the logical blocks.  But it's needed to
> > handle an edge case in one of the fscrypt IV generation methods.
> > 
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > Co-developed-by: Satya Tangirala <satyat@google.com>
> > Signed-off-by: Satya Tangirala <satyat@google.com>
> > ---
> >  fs/iomap/direct-io.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> > index ec7b78e6feca..12064daa3e3d 100644
> > --- a/fs/iomap/direct-io.c
> > +++ b/fs/iomap/direct-io.c
> > @@ -6,6 +6,7 @@
> >  #include <linux/module.h>
> >  #include <linux/compiler.h>
> >  #include <linux/fs.h>
> > +#include <linux/fscrypt.h>
> >  #include <linux/iomap.h>
> >  #include <linux/backing-dev.h>
> >  #include <linux/uio.h>
> > @@ -183,11 +184,16 @@ static void
> >  iomap_dio_zero(struct iomap_dio *dio, struct iomap *iomap, loff_t pos,
> >  		unsigned len)
> >  {
> > +	struct inode *inode = file_inode(dio->iocb->ki_filp);
> >  	struct page *page = ZERO_PAGE(0);
> >  	int flags = REQ_SYNC | REQ_IDLE;
> >  	struct bio *bio;
> >  
> >  	bio = bio_alloc(GFP_KERNEL, 1);
> > +
> > +	/* encrypted direct I/O is guaranteed to be fs-block aligned */
> > +	WARN_ON_ONCE(fscrypt_needs_contents_encryption(inode));
> 
> Which means you are now placing a new constraint on this code in
> that we cannot ever, in future, zero entire blocks here.
> 
> This code can issue arbitrary sized zeroing bios - multiple entire fs blocks
> blocks if necessary - so I think constraining it to only support
> partial block zeroing by adding a warning like this is no correct.

In v3 and earlier this instead had the code to set an encryption context:

	fscrypt_set_bio_crypt_ctx(bio, inode, pos >> inode->i_blkbits,
				  GFP_KERNEL);

Would you prefer that, even though the call to fscrypt_set_bio_crypt_ctx() would
always be a no-op currently (since for now, iomap_dio_zero() will never be
called with an encrypted file) and thus wouldn't be properly tested?

BTW, iomap_dio_zero() is actually limited to one page, so it's not quite
"arbitrary sizes".
	
> 
> > @@ -253,6 +259,7 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
> >  		ret = nr_pages;
> >  		goto out;
> >  	}
> > +	nr_pages = fscrypt_limit_io_pages(inode, pos, nr_pages);
> 
> So if "pos" overlaps a 2^32 offset when a certain mode is used, we
> have to break up the IO?

More or less.  It's actually when 'hashed_ino + (pos >> i_blkbits)' overlaps a
2^32 offset.  But yes, we have to break up the I/O when it happens.

> 
> If true, I'm not sure that this belongs here. Limiting the size of
> the IOs because of filesytem contraints belongs in the filesystem
> extent mapping code. That's the point where the IO is broken up into
> maximally sized chunks the filesystem can issue as a contiguous
> range. If the fscrypt code is breaking that "contiguous IO range"
> because of the mode being used, the fs mapping code should break
> the mapping at the boundery where the IO needs to be broken.
> 
> Hence the dio mapping code here will never build IOs that cross this
> -filesystem- encryption limitation, and we don't need this fscrypt
> code in the direct IO path at all.
> 

I think that would work.

iomap is used for other filesystem operations too, so we need to consider when
to actually do the limiting.  I don't think we should break up the extents
returned FS_IOC_FIEMAP, for example.  FIEMAP already has a defined behavior.
Also, it would be weird for the list of extents that FIEMAP returns to change
depending on whether the filesystem is mounted with '-o inlinecrypt' or not.

So, something like this:

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 44bad4bb8831..2816194db46c 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3437,6 +3437,15 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
 	map.m_len = min_t(loff_t, (offset + length - 1) >> blkbits,
 			  EXT4_MAX_LOGICAL_BLOCK) - map.m_lblk + 1;
 
+	/*
+	 * When inline encryption is enabled, sometimes I/O to an encrypted file
+	 * has to be broken up to guarantee DUN contiguity.  Handle this by
+	 * limiting the length of the mapping returned.
+	 */
+	if (!(flags & IOMAP_REPORT))
+		map.m_len = fscrypt_limit_io_blocks(inode, map.m_lblk,
+						    map.m_len);
+
 	if (flags & IOMAP_WRITE)
 		ret = ext4_iomap_alloc(inode, &map, flags);
 	else


That also avoids any confusion between pages and blocks, which is nice.

- Eric

^ permalink raw reply related	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 3/7] iomap: support direct I/O with fscrypt using blk-crypto
  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-23 22:07       ` Dave Chinner
  2 siblings, 1 reply; 34+ messages in thread
From: Matthew Wilcox @ 2020-07-22 22:44 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Dave Chinner, Satya Tangirala, linux-fscrypt, linux-fsdevel,
	linux-f2fs-devel, linux-ext4, linux-xfs

On Wed, Jul 22, 2020 at 03:34:04PM -0700, Eric Biggers wrote:
> > Which means you are now placing a new constraint on this code in
> > that we cannot ever, in future, zero entire blocks here.
> > 
> > This code can issue arbitrary sized zeroing bios - multiple entire fs blocks
> > blocks if necessary - so I think constraining it to only support
> > partial block zeroing by adding a warning like this is no correct.
> 
> In v3 and earlier this instead had the code to set an encryption context:
> 
> 	fscrypt_set_bio_crypt_ctx(bio, inode, pos >> inode->i_blkbits,
> 				  GFP_KERNEL);
> 
> Would you prefer that, even though the call to fscrypt_set_bio_crypt_ctx() would
> always be a no-op currently (since for now, iomap_dio_zero() will never be
> called with an encrypted file) and thus wouldn't be properly tested?
> 
> BTW, iomap_dio_zero() is actually limited to one page, so it's not quite
> "arbitrary sizes".

I have a patch for that

http://git.infradead.org/users/willy/pagecache.git/commitdiff/1a4d72a890ca9c2ea3d244a6153511ae674ce1d8

It's not going to cause a problem for crossing a 2^32 boundary because
pages are naturally aligned and don't get that big.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 3/7] iomap: support direct I/O with fscrypt using blk-crypto
  2020-07-22 22:44       ` Matthew Wilcox
@ 2020-07-22 23:12         ` Eric Biggers
  0 siblings, 0 replies; 34+ messages in thread
From: Eric Biggers @ 2020-07-22 23:12 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Dave Chinner, Satya Tangirala, linux-fscrypt, linux-fsdevel,
	linux-f2fs-devel, linux-ext4, linux-xfs

On Wed, Jul 22, 2020 at 11:44:07PM +0100, Matthew Wilcox wrote:
> On Wed, Jul 22, 2020 at 03:34:04PM -0700, Eric Biggers wrote:
> > > Which means you are now placing a new constraint on this code in
> > > that we cannot ever, in future, zero entire blocks here.
> > > 
> > > This code can issue arbitrary sized zeroing bios - multiple entire fs blocks
> > > blocks if necessary - so I think constraining it to only support
> > > partial block zeroing by adding a warning like this is no correct.
> > 
> > In v3 and earlier this instead had the code to set an encryption context:
> > 
> > 	fscrypt_set_bio_crypt_ctx(bio, inode, pos >> inode->i_blkbits,
> > 				  GFP_KERNEL);
> > 
> > Would you prefer that, even though the call to fscrypt_set_bio_crypt_ctx() would
> > always be a no-op currently (since for now, iomap_dio_zero() will never be
> > called with an encrypted file) and thus wouldn't be properly tested?
> > 
> > BTW, iomap_dio_zero() is actually limited to one page, so it's not quite
> > "arbitrary sizes".
> 
> I have a patch for that
> 
> http://git.infradead.org/users/willy/pagecache.git/commitdiff/1a4d72a890ca9c2ea3d244a6153511ae674ce1d8

No you don't :-)  Your patch is for iomap_zero_range() in
fs/iomap/buffered-io.c.  It doesn't touch fs/iomap/direct-io.c which is what
we're talking about here.

> It's not going to cause a problem for crossing a 2^32 boundary because
> pages are naturally aligned and don't get that big.

Well, the boundary can actually occur at any block.  But it's not relevant here
because (a) fs/iomap/buffered-io.c doesn't yet support encryption anyway, since
neither ext4 nor f2fs use it; and (b) iomap_zero_range() just writes to the
pagecache, and the bios aren't actually issued until ->writepages().

- Eric

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 3/7] iomap: support direct I/O with fscrypt using blk-crypto
  2020-07-22 22:34     ` Eric Biggers
  2020-07-22 22:44       ` Matthew Wilcox
@ 2020-07-22 23:26       ` Eric Biggers
  2020-07-22 23:32         ` Darrick J. Wong
  2020-07-23 22:07       ` Dave Chinner
  2 siblings, 1 reply; 34+ messages in thread
From: Eric Biggers @ 2020-07-22 23:26 UTC (permalink / raw)
  To: Dave Chinner
  Cc: Satya Tangirala, linux-fscrypt, linux-fsdevel, linux-f2fs-devel,
	linux-ext4, linux-xfs

On Wed, Jul 22, 2020 at 03:34:04PM -0700, Eric Biggers wrote:
> So, something like this:
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 44bad4bb8831..2816194db46c 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -3437,6 +3437,15 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
>  	map.m_len = min_t(loff_t, (offset + length - 1) >> blkbits,
>  			  EXT4_MAX_LOGICAL_BLOCK) - map.m_lblk + 1;
>  
> +	/*
> +	 * When inline encryption is enabled, sometimes I/O to an encrypted file
> +	 * has to be broken up to guarantee DUN contiguity.  Handle this by
> +	 * limiting the length of the mapping returned.
> +	 */
> +	if (!(flags & IOMAP_REPORT))
> +		map.m_len = fscrypt_limit_io_blocks(inode, map.m_lblk,
> +						    map.m_len);
> +
>  	if (flags & IOMAP_WRITE)
>  		ret = ext4_iomap_alloc(inode, &map, flags);
>  	else
> 
> 
> That also avoids any confusion between pages and blocks, which is nice.

Correction: for fiemap, ext4 actually uses ext4_iomap_begin_report() instead of
ext4_iomap_begin().  So we don't need to check for !IOMAP_REPORT.

Also it could make sense to limit map.m_len after ext4_iomap_alloc() rather than
before, so that we don't limit the length of the extent that gets allocated but
rather just the length that gets returned to iomap.

- Eric

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 3/7] iomap: support direct I/O with fscrypt using blk-crypto
  2020-07-22 23:26       ` Eric Biggers
@ 2020-07-22 23:32         ` Darrick J. Wong
  2020-07-22 23:43           ` Eric Biggers
  0 siblings, 1 reply; 34+ messages in thread
From: Darrick J. Wong @ 2020-07-22 23:32 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Dave Chinner, Satya Tangirala, linux-fscrypt, linux-fsdevel,
	linux-f2fs-devel, linux-ext4, linux-xfs

On Wed, Jul 22, 2020 at 04:26:25PM -0700, Eric Biggers wrote:
> On Wed, Jul 22, 2020 at 03:34:04PM -0700, Eric Biggers wrote:
> > So, something like this:
> > 
> > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> > index 44bad4bb8831..2816194db46c 100644
> > --- a/fs/ext4/inode.c
> > +++ b/fs/ext4/inode.c
> > @@ -3437,6 +3437,15 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
> >  	map.m_len = min_t(loff_t, (offset + length - 1) >> blkbits,
> >  			  EXT4_MAX_LOGICAL_BLOCK) - map.m_lblk + 1;
> >  
> > +	/*
> > +	 * When inline encryption is enabled, sometimes I/O to an encrypted file
> > +	 * has to be broken up to guarantee DUN contiguity.  Handle this by
> > +	 * limiting the length of the mapping returned.
> > +	 */
> > +	if (!(flags & IOMAP_REPORT))
> > +		map.m_len = fscrypt_limit_io_blocks(inode, map.m_lblk,
> > +						    map.m_len);
> > +
> >  	if (flags & IOMAP_WRITE)
> >  		ret = ext4_iomap_alloc(inode, &map, flags);
> >  	else
> > 
> > 
> > That also avoids any confusion between pages and blocks, which is nice.
> 
> Correction: for fiemap, ext4 actually uses ext4_iomap_begin_report() instead of
> ext4_iomap_begin().  So we don't need to check for !IOMAP_REPORT.
> 
> Also it could make sense to limit map.m_len after ext4_iomap_alloc() rather than
> before, so that we don't limit the length of the extent that gets allocated but
> rather just the length that gets returned to iomap.

Naïve question here -- if the decision to truncate the bio depends on
the file block offset, can you achieve the same thing by capping the
length of the iovec prior to iomap_dio_rw?

Granted that probably only makes sense if the LBLK IV thing is only
supposed to be used infrequently, and having to opencode a silly loop
might be more hassle than it's worth...

--D

> - Eric

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 3/7] iomap: support direct I/O with fscrypt using blk-crypto
  2020-07-22 23:32         ` Darrick J. Wong
@ 2020-07-22 23:43           ` Eric Biggers
  0 siblings, 0 replies; 34+ messages in thread
From: Eric Biggers @ 2020-07-22 23:43 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: Dave Chinner, Satya Tangirala, linux-fscrypt, linux-fsdevel,
	linux-f2fs-devel, linux-ext4, linux-xfs

On Wed, Jul 22, 2020 at 04:32:47PM -0700, Darrick J. Wong wrote:
> On Wed, Jul 22, 2020 at 04:26:25PM -0700, Eric Biggers wrote:
> > On Wed, Jul 22, 2020 at 03:34:04PM -0700, Eric Biggers wrote:
> > > So, something like this:
> > > 
> > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> > > index 44bad4bb8831..2816194db46c 100644
> > > --- a/fs/ext4/inode.c
> > > +++ b/fs/ext4/inode.c
> > > @@ -3437,6 +3437,15 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
> > >  	map.m_len = min_t(loff_t, (offset + length - 1) >> blkbits,
> > >  			  EXT4_MAX_LOGICAL_BLOCK) - map.m_lblk + 1;
> > >  
> > > +	/*
> > > +	 * When inline encryption is enabled, sometimes I/O to an encrypted file
> > > +	 * has to be broken up to guarantee DUN contiguity.  Handle this by
> > > +	 * limiting the length of the mapping returned.
> > > +	 */
> > > +	if (!(flags & IOMAP_REPORT))
> > > +		map.m_len = fscrypt_limit_io_blocks(inode, map.m_lblk,
> > > +						    map.m_len);
> > > +
> > >  	if (flags & IOMAP_WRITE)
> > >  		ret = ext4_iomap_alloc(inode, &map, flags);
> > >  	else
> > > 
> > > 
> > > That also avoids any confusion between pages and blocks, which is nice.
> > 
> > Correction: for fiemap, ext4 actually uses ext4_iomap_begin_report() instead of
> > ext4_iomap_begin().  So we don't need to check for !IOMAP_REPORT.
> > 
> > Also it could make sense to limit map.m_len after ext4_iomap_alloc() rather than
> > before, so that we don't limit the length of the extent that gets allocated but
> > rather just the length that gets returned to iomap.
> 
> Naïve question here -- if the decision to truncate the bio depends on
> the file block offset, can you achieve the same thing by capping the
> length of the iovec prior to iomap_dio_rw?
> 
> Granted that probably only makes sense if the LBLK IV thing is only
> supposed to be used infrequently, and having to opencode a silly loop
> might be more hassle than it's worth...
> 

We *could* do the truncation there, but that would truncate the actual read() or
write().  So, userspace would see a short read or write.  And I understand that
while applications are *supposed* to handle short reads and writes, many don't.

I think Dave's suggestion makes more sense, since it would make this case be
treated just like normal fragmentation of the file.

- Eric

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 3/7] iomap: support direct I/O with fscrypt using blk-crypto
  2020-07-22 22:34     ` Eric Biggers
  2020-07-22 22:44       ` Matthew Wilcox
  2020-07-22 23:26       ` Eric Biggers
@ 2020-07-23 22:07       ` Dave Chinner
  2020-07-23 23:03         ` Eric Biggers
  2 siblings, 1 reply; 34+ messages in thread
From: Dave Chinner @ 2020-07-23 22:07 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Satya Tangirala, linux-fscrypt, linux-fsdevel, linux-f2fs-devel,
	linux-ext4, linux-xfs

On Wed, Jul 22, 2020 at 03:34:04PM -0700, Eric Biggers wrote:
> On Thu, Jul 23, 2020 at 07:16:29AM +1000, Dave Chinner wrote:
> > On Mon, Jul 20, 2020 at 11:37:35PM +0000, Satya Tangirala wrote:
> > > From: Eric Biggers <ebiggers@google.com>
> > > 
> > > Wire up iomap direct I/O with the fscrypt additions for direct I/O.
> > > This allows ext4 to support direct I/O on encrypted files when inline
> > > encryption is enabled.
> > > 
> > > This change consists of two parts:
> > > 
> > > - Set a bio_crypt_ctx on bios for encrypted files, so that the file
> > >   contents get encrypted (or decrypted).
> > > 
> > > - Ensure that encryption data unit numbers (DUNs) are contiguous within
> > >   each bio.  Use the new function fscrypt_limit_io_pages() for this,
> > >   since the iomap code works directly with logical ranges and thus
> > >   doesn't have a chance to call fscrypt_mergeable_bio() on each page.
> > > 
> > > Note that fscrypt_limit_io_pages() is normally a no-op, as normally the
> > > DUNs simply increment along with the logical blocks.  But it's needed to
> > > handle an edge case in one of the fscrypt IV generation methods.
> > > 
> > > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > > Co-developed-by: Satya Tangirala <satyat@google.com>
> > > Signed-off-by: Satya Tangirala <satyat@google.com>
> > > ---
> > >  fs/iomap/direct-io.c | 12 +++++++++++-
> > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> > > index ec7b78e6feca..12064daa3e3d 100644
> > > --- a/fs/iomap/direct-io.c
> > > +++ b/fs/iomap/direct-io.c
> > > @@ -6,6 +6,7 @@
> > >  #include <linux/module.h>
> > >  #include <linux/compiler.h>
> > >  #include <linux/fs.h>
> > > +#include <linux/fscrypt.h>
> > >  #include <linux/iomap.h>
> > >  #include <linux/backing-dev.h>
> > >  #include <linux/uio.h>
> > > @@ -183,11 +184,16 @@ static void
> > >  iomap_dio_zero(struct iomap_dio *dio, struct iomap *iomap, loff_t pos,
> > >  		unsigned len)
> > >  {
> > > +	struct inode *inode = file_inode(dio->iocb->ki_filp);
> > >  	struct page *page = ZERO_PAGE(0);
> > >  	int flags = REQ_SYNC | REQ_IDLE;
> > >  	struct bio *bio;
> > >  
> > >  	bio = bio_alloc(GFP_KERNEL, 1);
> > > +
> > > +	/* encrypted direct I/O is guaranteed to be fs-block aligned */
> > > +	WARN_ON_ONCE(fscrypt_needs_contents_encryption(inode));
> > 
> > Which means you are now placing a new constraint on this code in
> > that we cannot ever, in future, zero entire blocks here.
> > 
> > This code can issue arbitrary sized zeroing bios - multiple entire fs blocks
> > blocks if necessary - so I think constraining it to only support
> > partial block zeroing by adding a warning like this is no correct.
> 
> In v3 and earlier this instead had the code to set an encryption context:
> 
> 	fscrypt_set_bio_crypt_ctx(bio, inode, pos >> inode->i_blkbits,
> 				  GFP_KERNEL);
> 
> Would you prefer that, even though the call to fscrypt_set_bio_crypt_ctx() would

Actually, I have no idea what that function does. It's not in a
5.8-rc6 kernel, and it's not in this patchset....

> always be a no-op currently (since for now, iomap_dio_zero() will never be
> called with an encrypted file) and thus wouldn't be properly tested?

Same can be said for this WARN_ON_ONCE() code :)

But, in the interests of not leaving landmines, if a fscrypt context
is needed to be attached to the bio for data IO in direct IO, it
should be attached to all bios that are allocated in the dio path
rather than leave a landmine for people in future to trip over.

> BTW, iomap_dio_zero() is actually limited to one page, so it's not quite
> "arbitrary sizes".

Yup, but that's an implentation detail, not a design constraint.
i.e. I typically review/talk about how stuff functions at a
design/architecture level, not how it's been implemented in the
code.

e.g. block size > page size patches in progress make use of the
"arbitrary length" capability of the design:

https://lore.kernel.org/linux-xfs/20181107063127.3902-7-david@fromorbit.com/

> iomap is used for other filesystem operations too, so we need to consider when
> to actually do the limiting.  I don't think we should break up the extents
> returned FS_IOC_FIEMAP, for example.  FIEMAP already has a defined behavior.
> Also, it would be weird for the list of extents that FIEMAP returns to change
> depending on whether the filesystem is mounted with '-o inlinecrypt' or not.

We don't need to care about that in the iomap code. The caller
controls the behaviour of the mapping callbacks themselves via
the iomap_ops structure they pass into high level iomap functions.

> That also avoids any confusion between pages and blocks, which is nice.

FWIW, the latest version of the above patchset (which,
co-incidentally, I was bring up to date yesterday) abstracts away
page and block sizes. It introduces the concept of "chunk size"
which is calculated from the combination of the current page's size
and the current inode's block size.

i.e. in the near future we are going to have both variable page
sizes (on a per-page basis via Willy's current work) and per-inode
blocks sizes smaller, the same and larger than the size of the
current pager. Hence we need to get rid of any assumptions about
page sizes and block sizes in the iomap code, not introduce new
ones.

Hence if there is any limitation of filesystem functionality based
on block size vs page size, it is going to be up to the filesystem
to detect and enforce those restrictions, not the iomap
infrastructure.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 3/7] iomap: support direct I/O with fscrypt using blk-crypto
  2020-07-23 22:07       ` Dave Chinner
@ 2020-07-23 23:03         ` Eric Biggers
  2020-07-24  1:39           ` Dave Chinner
  0 siblings, 1 reply; 34+ messages in thread
From: Eric Biggers @ 2020-07-23 23:03 UTC (permalink / raw)
  To: Dave Chinner
  Cc: Satya Tangirala, linux-fscrypt, linux-fsdevel, linux-f2fs-devel,
	linux-ext4, linux-xfs

Hi Dave,

On Fri, Jul 24, 2020 at 08:07:52AM +1000, Dave Chinner wrote:
> > > > @@ -183,11 +184,16 @@ static void
> > > >  iomap_dio_zero(struct iomap_dio *dio, struct iomap *iomap, loff_t pos,
> > > >  		unsigned len)
> > > >  {
> > > > +	struct inode *inode = file_inode(dio->iocb->ki_filp);
> > > >  	struct page *page = ZERO_PAGE(0);
> > > >  	int flags = REQ_SYNC | REQ_IDLE;
> > > >  	struct bio *bio;
> > > >  
> > > >  	bio = bio_alloc(GFP_KERNEL, 1);
> > > > +
> > > > +	/* encrypted direct I/O is guaranteed to be fs-block aligned */
> > > > +	WARN_ON_ONCE(fscrypt_needs_contents_encryption(inode));
> > > 
> > > Which means you are now placing a new constraint on this code in
> > > that we cannot ever, in future, zero entire blocks here.
> > > 
> > > This code can issue arbitrary sized zeroing bios - multiple entire fs blocks
> > > blocks if necessary - so I think constraining it to only support
> > > partial block zeroing by adding a warning like this is no correct.
> > 
> > In v3 and earlier this instead had the code to set an encryption context:
> > 
> > 	fscrypt_set_bio_crypt_ctx(bio, inode, pos >> inode->i_blkbits,
> > 				  GFP_KERNEL);
> > 
> > Would you prefer that, even though the call to fscrypt_set_bio_crypt_ctx() would
> 
> Actually, I have no idea what that function does. It's not in a
> 5.8-rc6 kernel, and it's not in this patchset....

The cover letter mentions that this patchset is based on fscrypt/master.

That is, "master" of https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git

fscrypt_set_bio_crypt_ctx() was introduced by
"fscrypt: add inline encryption support" on that branch.

> 
> > always be a no-op currently (since for now, iomap_dio_zero() will never be
> > called with an encrypted file) and thus wouldn't be properly tested?
> 
> Same can be said for this WARN_ON_ONCE() code :)
> 
> But, in the interests of not leaving landmines, if a fscrypt context
> is needed to be attached to the bio for data IO in direct IO, it
> should be attached to all bios that are allocated in the dio path
> rather than leave a landmine for people in future to trip over.

My concern is that if we were to pass the wrong 'lblk' to
fscrypt_set_bio_crypt_ctx(), we wouldn't catch it because it's not tested.
Passing the wrong 'lblk' would cause the data to be encrypted/decrypted
incorrectly.

It's not a big deal though, as it's "obviously correct".  So we can just go
with that if you prefer it.

> 
> > BTW, iomap_dio_zero() is actually limited to one page, so it's not quite
> > "arbitrary sizes".
> 
> Yup, but that's an implentation detail, not a design constraint.
> i.e. I typically review/talk about how stuff functions at a
> design/architecture level, not how it's been implemented in the
> code.
> 
> e.g. block size > page size patches in progress make use of the
> "arbitrary length" capability of the design:
> 
> https://lore.kernel.org/linux-xfs/20181107063127.3902-7-david@fromorbit.com/
> 
> > iomap is used for other filesystem operations too, so we need to consider when
> > to actually do the limiting.  I don't think we should break up the extents
> > returned FS_IOC_FIEMAP, for example.  FIEMAP already has a defined behavior.
> > Also, it would be weird for the list of extents that FIEMAP returns to change
> > depending on whether the filesystem is mounted with '-o inlinecrypt' or not.
> 
> We don't need to care about that in the iomap code. The caller
> controls the behaviour of the mapping callbacks themselves via
> the iomap_ops structure they pass into high level iomap functions.

Sure, I wasn't saying we need to.  I was talking about what we need to do in
ext4.

> 
> > That also avoids any confusion between pages and blocks, which is nice.
> 
> FWIW, the latest version of the above patchset (which,
> co-incidentally, I was bring up to date yesterday) abstracts away
> page and block sizes. It introduces the concept of "chunk size"
> which is calculated from the combination of the current page's size
> and the current inode's block size.
> 
> i.e. in the near future we are going to have both variable page
> sizes (on a per-page basis via Willy's current work) and per-inode
> blocks sizes smaller, the same and larger than the size of the
> current pager. Hence we need to get rid of any assumptions about
> page sizes and block sizes in the iomap code, not introduce new
> ones.
> 
> Hence if there is any limitation of filesystem functionality based
> on block size vs page size, it is going to be up to the filesystem
> to detect and enforce those restrictions, not the iomap
> infrastructure.

Sure, again I was talking about what we'll be doing in ext4, since with the
proposed change, it will be ext4 that does fscrypt_limit_io_blocks().  The limit
is based on blocks, not pages, so "fscrypt_limit_io_pages()" was a bit weird.

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.

However we currently don't support blocksize > PAGE_SIZE in ext4, f2fs, or
fs/crypto/ at all, so I don't think we should add extra logic to fs/iomap/ to
try to handle that case for encrypted files when we'd have no way to test it.

- Eric

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 3/7] iomap: support direct I/O with fscrypt using blk-crypto
  2020-07-23 23:03         ` Eric Biggers
@ 2020-07-24  1:39           ` Dave Chinner
  2020-07-24  3:46             ` Eric Biggers
  0 siblings, 1 reply; 34+ messages in thread
From: Dave Chinner @ 2020-07-24  1:39 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Satya Tangirala, linux-fscrypt, linux-fsdevel, linux-f2fs-devel,
	linux-ext4, linux-xfs

On Thu, Jul 23, 2020 at 04:03:45PM -0700, Eric Biggers wrote:
> Hi Dave,
> 
> On Fri, Jul 24, 2020 at 08:07:52AM +1000, Dave Chinner wrote:
> > > > > @@ -183,11 +184,16 @@ static void
> > > > >  iomap_dio_zero(struct iomap_dio *dio, struct iomap *iomap, loff_t pos,
> > > > >  		unsigned len)
> > > > >  {
> > > > > +	struct inode *inode = file_inode(dio->iocb->ki_filp);
> > > > >  	struct page *page = ZERO_PAGE(0);
> > > > >  	int flags = REQ_SYNC | REQ_IDLE;
> > > > >  	struct bio *bio;
> > > > >  
> > > > >  	bio = bio_alloc(GFP_KERNEL, 1);
> > > > > +
> > > > > +	/* encrypted direct I/O is guaranteed to be fs-block aligned */
> > > > > +	WARN_ON_ONCE(fscrypt_needs_contents_encryption(inode));
> > > > 
> > > > Which means you are now placing a new constraint on this code in
> > > > that we cannot ever, in future, zero entire blocks here.
> > > > 
> > > > This code can issue arbitrary sized zeroing bios - multiple entire fs blocks
> > > > blocks if necessary - so I think constraining it to only support
> > > > partial block zeroing by adding a warning like this is no correct.
> > > 
> > > In v3 and earlier this instead had the code to set an encryption context:
> > > 
> > > 	fscrypt_set_bio_crypt_ctx(bio, inode, pos >> inode->i_blkbits,
> > > 				  GFP_KERNEL);
> > > 
> > > Would you prefer that, even though the call to fscrypt_set_bio_crypt_ctx() would
> > 
> > Actually, I have no idea what that function does. It's not in a
> > 5.8-rc6 kernel, and it's not in this patchset....
> 
> The cover letter mentions that this patchset is based on fscrypt/master.

Which the reader is left to work out where it exists. If you are
going to say "based on", then a pointer to the actual tree like
this:

> That is, "master" of https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git

is somewhat helpful.

> fscrypt_set_bio_crypt_ctx() was introduced by
> "fscrypt: add inline encryption support" on that branch.

Way too much static inline function abstraction.

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.

Further, I note that the local variable for ci is fetched before
fscrypt_inode_uses_inline_crypto() is run, so leaving a landmine for
people who try to access ci before checking if inline crypto is
enabled. Or, indeed, if S_ENCRYPTED is set and the crypt_info has
not been set up, fscrypt_set_bio_crypt_ctx() will oops in the DIO
path.

> > > always be a no-op currently (since for now, iomap_dio_zero() will never be
> > > called with an encrypted file) and thus wouldn't be properly tested?
> > 
> > Same can be said for this WARN_ON_ONCE() code :)
> > 
> > But, in the interests of not leaving landmines, if a fscrypt context
> > is needed to be attached to the bio for data IO in direct IO, it
> > should be attached to all bios that are allocated in the dio path
> > rather than leave a landmine for people in future to trip over.
> 
> My concern is that if we were to pass the wrong 'lblk' to
> fscrypt_set_bio_crypt_ctx(), we wouldn't catch it because it's not tested.
> Passing the wrong 'lblk' would cause the data to be encrypted/decrypted
> incorrectly.

When you implement sub-block DIO alignment for fscrypt enabled
filesystems, fsx will tell you pretty quickly if you screwed up....

> 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...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 3/7] iomap: support direct I/O with fscrypt using blk-crypto
  2020-07-24  1:39           ` Dave Chinner
@ 2020-07-24  3:46             ` Eric Biggers
  2020-07-24  5:31               ` Dave Chinner
  0 siblings, 1 reply; 34+ messages in thread
From: Eric Biggers @ 2020-07-24  3:46 UTC (permalink / raw)
  To: Dave Chinner
  Cc: Satya Tangirala, linux-fscrypt, linux-fsdevel, linux-f2fs-devel,
	linux-ext4, linux-xfs

On Fri, Jul 24, 2020 at 11:39:10AM +1000, Dave Chinner wrote:
> > fscrypt_set_bio_crypt_ctx() was introduced by
> > "fscrypt: add inline encryption support" on that branch.
> 
> Way too much static inline function abstraction.

Well, this is mostly because we're trying to be a good kernel citizen and
minimize the overhead when our feature isn't enabled or isn't being used.

Eventually we might remove CONFIG_FS_ENCRYPTION_INLINE_CRYPT and make
CONFIG_FS_ENCRYPTION always offer inline crypto support, which would simplify
things.  But for now we'd like to keep the separate option.

Also, previously people have complained about hardcoded S_ISREG() to decide
whether a file needs contents encryption, and said they prefer a helper function
fscrypt_needs_contents_encryption().  (Which is what we have now.)

So we can't make everyone happy...

> 
> 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.

If we're talking about handling a hypothetical bug where it does nevertheless
happen, there are three options:

(a) Crash (current behavior)
(b) Silently leave the file unencrypted, i.e. silently introduce a critical
    security vulnerability.
(c) Return an error, which would add a lot of code complexity because we'd have
    start returning an error from places like fscrypt_set_bio_crypt_ctx() that
    currently can't fail.  This would be dead code that's not tested.

I very much prefer (a), since it's the simplest option, and it also makes the
bug most likely to be reported and fixed, without leaving the possibility for
data to silently be left unencrypted.  Crashing isn't good (obviously), but the
other options are worse.

> Further, I note that the local variable for ci is fetched before
> fscrypt_inode_uses_inline_crypto() is run, so leaving a landmine for
> people who try to access ci before checking if inline crypto is
> enabled. Or, indeed, if S_ENCRYPTED is set and the crypt_info has
> not been set up, fscrypt_set_bio_crypt_ctx() will oops in the DIO
> path.

Sure, this is harmless currently, but we can move the assignment to 'ci'.

> > > > always be a no-op currently (since for now, iomap_dio_zero() will never be
> > > > called with an encrypted file) and thus wouldn't be properly tested?
> > > 
> > > Same can be said for this WARN_ON_ONCE() code :)
> > > 
> > > But, in the interests of not leaving landmines, if a fscrypt context
> > > is needed to be attached to the bio for data IO in direct IO, it
> > > should be attached to all bios that are allocated in the dio path
> > > rather than leave a landmine for people in future to trip over.
> > 
> > My concern is that if we were to pass the wrong 'lblk' to
> > fscrypt_set_bio_crypt_ctx(), we wouldn't catch it because it's not tested.
> > Passing the wrong 'lblk' would cause the data to be encrypted/decrypted
> > incorrectly.
> 
> When you implement sub-block DIO alignment for fscrypt enabled
> filesystems, fsx will tell you pretty quickly if you screwed up....
> 
> > 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.

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.  I was just pointing out that some changes might be needed to
maintain this property in the blocksize > PAGE_SIZE case (which again, for
encrypted files is totally unsupported at the moment anyway).

(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, and there would be a
significant performance hit when using a smaller size, so there hasn't been a
need to add the extra layer of complexity.)

- Eric

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 3/7] iomap: support direct I/O with fscrypt using blk-crypto
  2020-07-24  3:46             ` Eric Biggers
@ 2020-07-24  5:31               ` Dave Chinner
  2020-07-24 17:41                 ` Eric Biggers
  0 siblings, 1 reply; 34+ messages in thread
From: Dave Chinner @ 2020-07-24  5:31 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Satya Tangirala, linux-fscrypt, linux-fsdevel, linux-f2fs-devel,
	linux-ext4, linux-xfs

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 3/7] iomap: support direct I/O with fscrypt using blk-crypto
  2020-07-24  5:31               ` Dave Chinner
@ 2020-07-24 17:41                 ` Eric Biggers
  2020-07-25 23:47                   ` Dave Chinner
  0 siblings, 1 reply; 34+ messages in thread
From: Eric Biggers @ 2020-07-24 17:41 UTC (permalink / raw)
  To: Dave Chinner
  Cc: Satya Tangirala, linux-fscrypt, linux-fsdevel, linux-f2fs-devel,
	linux-ext4, linux-xfs

On Fri, Jul 24, 2020 at 03:31:30PM +1000, Dave Chinner wrote:
> 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.

That way of thinking about things doesn't entirely work for inline encryption.

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.)

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.

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.

Is it your opinion that inline encryption should only be supported when
data_unit_size <= logical_block_size?  The problems with that are

    (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).

    (b) fscrypt currently only supports data_unit_size == filesystem_block_size.
	(OFC, filesystem_block_size may be greater than logical_block_size.)

    (c) Filesystem images would be less portable, unless the minimum
	data_unit_size were used everywhere which would degrade performance.

(We could address (b) by allowing users to specify data_unit_size when
encrypting a directory.  That would add complexity, but it's possible.)

But again, as far as I can tell, fs/iomap/direct-io.c currently *does* guarantee
that *if* the input is fully filesystem-block-aligned and if blocksize <=
PAGE_SIZE, then the issued I/O is also filesystem-block-aligned.

So as far as I can tell, there isn't really any problem there, at least not now.
I just want to make sure we're on the same page...

> 
> > (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.
> 

It's already supported (on ext4 since Linux v5.5).

What's not supported (yet) is data_unit_size < filesystem_block_size.

- Eric

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 3/7] iomap: support direct I/O with fscrypt using blk-crypto
  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
  0 siblings, 2 replies; 34+ messages in thread
From: Dave Chinner @ 2020-07-25 23:47 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Satya Tangirala, linux-fscrypt, linux-fsdevel, linux-f2fs-devel,
	linux-ext4, linux-xfs

On Fri, Jul 24, 2020 at 10:41:32AM -0700, Eric Biggers wrote:
> On Fri, Jul 24, 2020 at 03:31:30PM +1000, Dave Chinner wrote:
> > 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.
> 
> 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.

> 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.

>     (b) fscrypt currently only supports data_unit_size == filesystem_block_size.
> 	(OFC, filesystem_block_size may be greater than logical_block_size.)

Which is just fine if FSB == logical block size.

The existing constraint on filesystems is that FSB >= block device
logical sector size as the filesystem has to be able to do single
block IOs.  For inline encryption, this turns into a constraint on
fscrypt that DUN <= logical block size because it requires IOs to be
aligned to DUNs, not filesystem blocks..

And because of the -implementation limitation- of fscrypt that DUN
== FSB, that means the only valid configuration right now is DUN =
FSB = logical sector size.

>     (c) Filesystem images would be less portable, unless the minimum
> 	data_unit_size were used everywhere which would degrade performance.

Not my problem. If the hardware and/or kernel cannot support the
requirements of the encryption used within the filesystem image,
then it -should error out-.

> (We could address (b) by allowing users to specify data_unit_size when
> encrypting a directory.  That would add complexity, but it's possible.)
> 
> But again, as far as I can tell, fs/iomap/direct-io.c currently *does* guarantee
> that *if* the input is fully filesystem-block-aligned and if blocksize <=
> PAGE_SIZE, then the issued I/O is also filesystem-block-aligned.

Please listen to what I'm saying, Eric.

The -current iomap implementation- may provide that behaviour. That
doesn't mean we guarantee that behaviour. i.e. the iomap -design-
does not guaranteee that behaviour, and we don't guarantee such
behaviour into the future. And we won't guarantee this behaviour -
even though the current implementation may provide it - because the
rest of the IO stack below iomap does not provide iomap with that
guarantee.

Hence if iomap cannot get a guarantee that IO it issues won't get
split at some arbitrary boundary, it cannot provide filesystems with
that guarantee.

> So as far as I can tell, there isn't really any problem there, at least not now.
> I just want to make sure we're on the same page...

If that page is "fscrypt+inline encryption is making fundamentally
flawed assumptions about IO stack behaviour" then, yes, we're on the
same page.

And that code that has fundamentally flawed assumptions is grounds
for a NACK, yes?

-Dave.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 3/7] iomap: support direct I/O with fscrypt using blk-crypto
  2020-07-25 23:47                   ` Dave Chinner
@ 2020-07-25 23:59                     ` Dave Chinner
  2020-07-26  2:42                     ` Eric Biggers
  1 sibling, 0 replies; 34+ messages in thread
From: Dave Chinner @ 2020-07-25 23:59 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Satya Tangirala, linux-fscrypt, linux-fsdevel, linux-f2fs-devel,
	linux-ext4, linux-xfs

On Sun, Jul 26, 2020 at 09:47:51AM +1000, Dave Chinner wrote:
> On Fri, Jul 24, 2020 at 10:41:32AM -0700, Eric Biggers wrote:
> > But again, as far as I can tell, fs/iomap/direct-io.c currently *does* guarantee
> > that *if* the input is fully filesystem-block-aligned and if blocksize <=
> > PAGE_SIZE, then the issued I/O is also filesystem-block-aligned.
> 
> Please listen to what I'm saying, Eric.
> 
> The -current iomap implementation- may provide that behaviour. That
> doesn't mean we guarantee that behaviour. i.e. the iomap -design-
> does not guaranteee that behaviour, and we don't guarantee such
> behaviour into the future. And we won't guarantee this behaviour -
> even though the current implementation may provide it - because the
> rest of the IO stack below iomap does not provide iomap with that
> guarantee.
> 
> Hence if iomap cannot get a guarantee that IO it issues won't get
> split at some arbitrary boundary, it cannot provide filesystems with
> that guarantee.

BTW, if you want iomap_dio_rw() to provide an arbitrary bio
alignment guarantee at the iomap layer, then it should be returned
in the iomap along with the extent mapping. That could then be used
instead of the bdev logical block size. That won't guarantee the
behaviour of the rest of the stack, but it would provide a defined
IO submission behaviour that iomap would have to guarantee into the
future...

That would also remove the need to duplicate the alignment checks
in the filesystem for fscrypt DIO...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 3/7] iomap: support direct I/O with fscrypt using blk-crypto
  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
  1 sibling, 1 reply; 34+ messages in thread
From: Eric Biggers @ 2020-07-26  2:42 UTC (permalink / raw)
  To: Dave Chinner
  Cc: Satya Tangirala, linux-fscrypt, linux-fsdevel, linux-f2fs-devel,
	linux-ext4, linux-xfs

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 3/7] iomap: support direct I/O with fscrypt using blk-crypto
  2020-07-26  2:42                     ` Eric Biggers
@ 2020-07-27 17:16                       ` Eric Biggers
  0 siblings, 0 replies; 34+ messages in thread
From: Eric Biggers @ 2020-07-27 17:16 UTC (permalink / raw)
  To: Dave Chinner
  Cc: Satya Tangirala, linux-fscrypt, linux-fsdevel, linux-f2fs-devel,
	linux-ext4, linux-xfs

On Sat, Jul 25, 2020 at 07:42:11PM -0700, Eric Biggers wrote:
> > 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.

I found get_max_io_size() in block/blk-merge.c.  I'll check if that needs to be
updated.

Let me know if you have any objection to the fscrypt inline encryption patches
*without direct I/O support* going into 5.9.  Note that fscrypt doesn't directly
care about this block layer stuff at all; instead it uses
blk_crypto_config_supported() to check whether inline encryption with the
specified (crypto_mode, data_unit_size, dun_bytes) combination is supported on
the filesystem's device(s).  Only then will fscrypt use inline encryption
instead of the traditional filesystem-layer encryption.

So if blk_crypto_config_supported() is saying that some crypto configuration is
supported when it isn't, then that's a bug in the blk-crypto patches that went
into the block layer in 5.8, which we need to fix there.  (Ideally by fixing any
cases where encrypted I/O may be split in the middle of a data unit.  But in the
worst case, we could easily make blk_crypto_config_supported() return false when
'data_unit_size > logical_block_size' for now.)

- Eric

^ permalink raw reply	[flat|nested] 34+ messages in thread

end of thread, other threads:[~2020-07-27 17:16 UTC | newest]

Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-20 23:37 [PATCH v4 0/7] add support for direct I/O with fscrypt using blk-crypto 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
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

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).