linux-fscrypt.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v9 0/9] add support for direct I/O with fscrypt using blk-crypto
@ 2021-06-04 21:08 Satya Tangirala
  2021-06-04 21:09 ` [PATCH v9 1/9] block: blk-crypto-fallback: handle data unit split across multiple bvecs Satya Tangirala
                   ` (9 more replies)
  0 siblings, 10 replies; 18+ messages in thread
From: Satya Tangirala @ 2021-06-04 21:08 UTC (permalink / raw)
  To: Theodore Y . Ts'o, Jaegeuk Kim, Eric Biggers, Chao Yu,
	Jens Axboe, Darrick J . Wong
  Cc: linux-kernel, linux-fscrypt, linux-f2fs-devel, linux-xfs,
	linux-block, linux-ext4, Satya Tangirala

This patch series adds support for direct I/O with fscrypt using
blk-crypto. Note that this patch relies on another patchset
("ensure bios aren't split in middle of crypto data unit" found at
https://lore.kernel.org/linux-block/20210604195900.2096121-1-satyat@google.com/
)

Till now, the blk-crypto-fallback expected the offset and length of each
bvec in a bio to be aligned to the crypto data unit size. This in turn
would mean that any user buffer used to read/write encrypted data using the
blk-crypto framework would need to be aligned to the crypto data unit size.
Patch 1 enables blk-crypto-fallback to work without this requirement. It
also relaxes the alignment requirement that blk-crypto checks for - now,
blk-crypto only requires that the length of the I/O is aligned to the
crypto data unit size. This allows direct I/O support introduced in the
later patches in this series to require extra alignment restrictions on
user buffers.

Patch 2 relaxes the alignment check that blk-crypto performs on bios.
blk-crypto would check that the offset and length of each bvec in a bio is
aligned to the data unit size, since the blk-crypto-fallback required it.
As this is no longer the case, blk-crypto now only checks that the total
length of the bio is data unit size aligned.

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

Patches 4 and 6 modify direct-io and iomap respectively to set bio crypt
contexts on bios when appropriate by calling into fscrypt.

Patch 5 makes bio_iov_iter_get_pages() respect
bio_required_sector_alignment() which is now necessary since Patch 6 makes
it possible for iomap (which uses bio_iov_iter_get_pages()) construct bios
that have crypt contexts.

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

Patch 9 updates the fscrypt documentation for direct I/O support.
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. Without any modification, xfstests skip any
direct I/O test when using ext4/encrypt and f2fs/encrypt, so I modified
xfstests not to skip those tests.

Among those tests, generic/465 fails with ext4/encrypt because a bio ends
up being split in the middle of a crypto data unit.  Patch 1 from v7 (which
has been sent out as a separate patch series) fixes this.

Note that the blk-crypto-fallback changes (Patch 1 in v8 in this series)
were also tested through xfstests by using this series along with the patch
series that ensures bios aren't split in the middle of a data unit (Patch 1
from v7) - Some tests (such as generic/465 again) result in bvecs that
don't contain a complete data unit (so a data unit is split across multiple
bvecs), and only pass with this patch.

Changes v8 => v9:
 - Introduce patch 5 to fix bug with iomap_dio_bio_actor() which
   constructed bios that had incomplete crypto data units (fixes xfstests
   generic/465 with ext4)

Changes v7 => v8:
 - Patch 1 from v7 (which ensured that bios aren't split in the middle of
   a data unit) has been sent out in a separate patch series, as it's
   required even without this patch series. That patch series can now
   be found at
   https://lore.kernel.org/linux-block/20210604195900.2096121-1-satyat@google.com/
 - Patch 2 from v7 has been split into 2 patches (Patch 1 and 2 in v8).
 - Update docs

Changes v6 => v7:
 - add patches 1 and 2 to allow blk-crypto to work with user buffers not
   aligned to crypto data unit size, so that direct I/O doesn't require
   that alignment either.
 - some cleanups

Changes v5 => v6:
 - fix bug with fscrypt_limit_io_blocks() and make it ready for 64 bit
   block numbers.
 - remove Reviewed-by for Patch 1 due to significant changes from when
   the Reviewed-by was given.

Changes v4 => v5:
 - replace fscrypt_limit_io_pages() with fscrypt_limit_io_block(), which
   is now called by individual filesystems (currently only ext4) instead
   of the iomap code. This new function serves the same end purpose as
   the one it replaces (ensuring that DUNs within a bio are contiguous)
   but operates purely with blocks instead of with pages.
 - make iomap_dio_zero() set bio_crypt_ctx's again, instead of just a
   WARN_ON() since some folks prefer that instead.
 - add Reviewed-by's

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 (4):
  block: blk-crypto-fallback: handle data unit split across multiple
    bvecs
  block: blk-crypto: relax alignment requirements for bvecs in bios
  block: Make bio_iov_iter_get_pages() respect
    bio_required_sector_alignment()
  fscrypt: update documentation for direct I/O support

 Documentation/filesystems/fscrypt.rst |  21 ++-
 block/bio.c                           |  13 +-
 block/blk-crypto-fallback.c           | 203 ++++++++++++++++++++------
 block/blk-crypto.c                    |  19 +--
 fs/crypto/crypto.c                    |   8 +
 fs/crypto/inline_crypt.c              |  75 ++++++++++
 fs/direct-io.c                        |  15 +-
 fs/ext4/file.c                        |  10 +-
 fs/ext4/inode.c                       |   7 +
 fs/f2fs/f2fs.h                        |   6 +-
 fs/iomap/direct-io.c                  |   6 +
 include/linux/fscrypt.h               |  18 +++
 12 files changed, 328 insertions(+), 73 deletions(-)

-- 
2.32.0.rc1.229.g3e70b5a671-goog


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

* [PATCH v9 1/9] block: blk-crypto-fallback: handle data unit split across multiple bvecs
  2021-06-04 21:08 [PATCH v9 0/9] add support for direct I/O with fscrypt using blk-crypto Satya Tangirala
@ 2021-06-04 21:09 ` Satya Tangirala
  2021-06-04 21:09 ` [PATCH v9 2/9] block: blk-crypto: relax alignment requirements for bvecs in bios Satya Tangirala
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: Satya Tangirala @ 2021-06-04 21:09 UTC (permalink / raw)
  To: Theodore Y . Ts'o, Jaegeuk Kim, Eric Biggers, Chao Yu,
	Jens Axboe, Darrick J . Wong
  Cc: linux-kernel, linux-fscrypt, linux-f2fs-devel, linux-xfs,
	linux-block, linux-ext4, Satya Tangirala, Eric Biggers

Till now, the blk-crypto-fallback required each crypto data unit to be
contained within a single bvec. It also required the starting offset
of each bvec to be aligned to the data unit size. This patch removes
both restrictions, so that blk-crypto-fallback can handle crypto data
units split across multiple bvecs. blk-crypto-fallback now only requires
that the total size of the bio be aligned to the crypto data unit size.
The buffer that is being read/written to no longer needs to be data unit
size aligned.

This is useful for making the alignment requirements for direct I/O on
encrypted files similar to those for direct I/O on unencrypted files.

Co-developed-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: Satya Tangirala <satyat@google.com>
---
 block/blk-crypto-fallback.c | 203 +++++++++++++++++++++++++++---------
 1 file changed, 156 insertions(+), 47 deletions(-)

diff --git a/block/blk-crypto-fallback.c b/block/blk-crypto-fallback.c
index 85c813ef670b..658ff7deadf1 100644
--- a/block/blk-crypto-fallback.c
+++ b/block/blk-crypto-fallback.c
@@ -256,6 +256,65 @@ static void blk_crypto_dun_to_iv(const u64 dun[BLK_CRYPTO_DUN_ARRAY_SIZE],
 		iv->dun[i] = cpu_to_le64(dun[i]);
 }
 
+/*
+ * If the length of any bio segment isn't a multiple of data_unit_size
+ * (which can happen if data_unit_size > logical_block_size), then each
+ * encryption/decryption might need to be passed multiple scatterlist elements.
+ * If that will be the case, this function allocates and initializes src and dst
+ * scatterlists (or a combined src/dst scatterlist) with the needed length.
+ *
+ * If 1 element is guaranteed to be enough (which is usually the case, and is
+ * guaranteed when data_unit_size <= logical_block_size), then this function
+ * just initializes the on-stack scatterlist(s).
+ */
+static bool blk_crypto_alloc_sglists(struct bio *bio,
+				     const struct bvec_iter *start_iter,
+				     unsigned int data_unit_size,
+				     struct scatterlist **src_p,
+				     struct scatterlist **dst_p)
+{
+	struct bio_vec bv;
+	struct bvec_iter iter;
+	bool aligned = true;
+	unsigned int count = 0;
+
+	__bio_for_each_segment(bv, bio, iter, *start_iter) {
+		count++;
+		aligned &= IS_ALIGNED(bv.bv_len, data_unit_size);
+	}
+	if (aligned) {
+		count = 1;
+	} else {
+		/*
+		 * We can't need more elements than bio segments, and we can't
+		 * need more than the number of sectors per data unit.  This may
+		 * overestimate the required length by a bit, but that's okay.
+		 */
+		count = min(count, data_unit_size >> SECTOR_SHIFT);
+	}
+
+	if (count > 1) {
+		*src_p = kmalloc_array(count, sizeof(struct scatterlist),
+				       GFP_NOIO);
+		if (!*src_p)
+			return false;
+		if (dst_p) {
+			*dst_p = kmalloc_array(count,
+					       sizeof(struct scatterlist),
+					       GFP_NOIO);
+			if (!*dst_p) {
+				kfree(*src_p);
+				*src_p = NULL;
+				return false;
+			}
+		}
+	}
+	sg_init_table(*src_p, count);
+	if (dst_p)
+		sg_init_table(*dst_p, count);
+	return true;
+}
+
 /*
  * The crypto API fallback's encryption routine.
  * Allocate a bounce bio for encryption, encrypt the input bio using crypto API,
@@ -272,9 +331,12 @@ static bool blk_crypto_fallback_encrypt_bio(struct bio **bio_ptr)
 	struct skcipher_request *ciph_req = NULL;
 	DECLARE_CRYPTO_WAIT(wait);
 	u64 curr_dun[BLK_CRYPTO_DUN_ARRAY_SIZE];
-	struct scatterlist src, dst;
+	struct scatterlist _src, *src = &_src;
+	struct scatterlist _dst, *dst = &_dst;
 	union blk_crypto_iv iv;
-	unsigned int i, j;
+	unsigned int i;
+	unsigned int sg_idx = 0;
+	unsigned int du_filled = 0;
 	bool ret = false;
 	blk_status_t blk_st;
 
@@ -286,11 +348,18 @@ static bool blk_crypto_fallback_encrypt_bio(struct bio **bio_ptr)
 	bc = src_bio->bi_crypt_context;
 	data_unit_size = bc->bc_key->crypto_cfg.data_unit_size;
 
+	/* Allocate scatterlists if needed */
+	if (!blk_crypto_alloc_sglists(src_bio, &src_bio->bi_iter,
+				      data_unit_size, &src, &dst)) {
+		src_bio->bi_status = BLK_STS_RESOURCE;
+		return false;
+	}
+
 	/* Allocate bounce bio for encryption */
 	enc_bio = blk_crypto_clone_bio(src_bio);
 	if (!enc_bio) {
 		src_bio->bi_status = BLK_STS_RESOURCE;
-		return false;
+		goto out_free_sglists;
 	}
 
 	/*
@@ -310,45 +379,58 @@ static bool blk_crypto_fallback_encrypt_bio(struct bio **bio_ptr)
 	}
 
 	memcpy(curr_dun, bc->bc_dun, sizeof(curr_dun));
-	sg_init_table(&src, 1);
-	sg_init_table(&dst, 1);
 
-	skcipher_request_set_crypt(ciph_req, &src, &dst, data_unit_size,
+	skcipher_request_set_crypt(ciph_req, src, dst, data_unit_size,
 				   iv.bytes);
 
-	/* Encrypt each page in the bounce bio */
+	/*
+	 * Encrypt each data unit in the bounce bio.
+	 *
+	 * Take care to handle the case where a data unit spans bio segments.
+	 * This can happen when data_unit_size > logical_block_size.
+	 */
 	for (i = 0; i < enc_bio->bi_vcnt; i++) {
-		struct bio_vec *enc_bvec = &enc_bio->bi_io_vec[i];
-		struct page *plaintext_page = enc_bvec->bv_page;
+		struct bio_vec *bv = &enc_bio->bi_io_vec[i];
+		struct page *plaintext_page = bv->bv_page;
 		struct page *ciphertext_page =
 			mempool_alloc(blk_crypto_bounce_page_pool, GFP_NOIO);
+		unsigned int offset_in_bv = 0;
 
-		enc_bvec->bv_page = ciphertext_page;
+		bv->bv_page = ciphertext_page;
 
 		if (!ciphertext_page) {
 			src_bio->bi_status = BLK_STS_RESOURCE;
 			goto out_free_bounce_pages;
 		}
 
-		sg_set_page(&src, plaintext_page, data_unit_size,
-			    enc_bvec->bv_offset);
-		sg_set_page(&dst, ciphertext_page, data_unit_size,
-			    enc_bvec->bv_offset);
-
-		/* Encrypt each data unit in this page */
-		for (j = 0; j < enc_bvec->bv_len; j += data_unit_size) {
-			blk_crypto_dun_to_iv(curr_dun, &iv);
-			if (crypto_wait_req(crypto_skcipher_encrypt(ciph_req),
-					    &wait)) {
-				i++;
-				src_bio->bi_status = BLK_STS_IOERR;
-				goto out_free_bounce_pages;
+		while (offset_in_bv < bv->bv_len) {
+			unsigned int n = min(bv->bv_len - offset_in_bv,
+					     data_unit_size - du_filled);
+			sg_set_page(&src[sg_idx], plaintext_page, n,
+				    bv->bv_offset + offset_in_bv);
+			sg_set_page(&dst[sg_idx], ciphertext_page, n,
+				    bv->bv_offset + offset_in_bv);
+			sg_idx++;
+			offset_in_bv += n;
+			du_filled += n;
+			if (du_filled == data_unit_size) {
+				blk_crypto_dun_to_iv(curr_dun, &iv);
+				if (crypto_wait_req(crypto_skcipher_encrypt(ciph_req),
+						    &wait)) {
+					src_bio->bi_status = BLK_STS_IOERR;
+					i++;
+					goto out_free_bounce_pages;
+				}
+				bio_crypt_dun_increment(curr_dun, 1);
+				sg_idx = 0;
+				du_filled = 0;
 			}
-			bio_crypt_dun_increment(curr_dun, 1);
-			src.offset += data_unit_size;
-			dst.offset += data_unit_size;
 		}
 	}
+	if (WARN_ON_ONCE(du_filled != 0)) {
+		src_bio->bi_status = BLK_STS_IOERR;
+		goto out_free_bounce_pages;
+	}
 
 	enc_bio->bi_private = src_bio;
 	enc_bio->bi_end_io = blk_crypto_fallback_encrypt_endio;
@@ -369,7 +451,11 @@ static bool blk_crypto_fallback_encrypt_bio(struct bio **bio_ptr)
 out_put_enc_bio:
 	if (enc_bio)
 		bio_put(enc_bio);
-
+out_free_sglists:
+	if (src != &_src)
+		kfree(src);
+	if (dst != &_dst)
+		kfree(dst);
 	return ret;
 }
 
@@ -388,13 +474,21 @@ static void blk_crypto_fallback_decrypt_bio(struct work_struct *work)
 	DECLARE_CRYPTO_WAIT(wait);
 	u64 curr_dun[BLK_CRYPTO_DUN_ARRAY_SIZE];
 	union blk_crypto_iv iv;
-	struct scatterlist sg;
+	struct scatterlist _sg, *sg = &_sg;
 	struct bio_vec bv;
 	struct bvec_iter iter;
 	const int data_unit_size = bc->bc_key->crypto_cfg.data_unit_size;
-	unsigned int i;
+	unsigned int sg_idx = 0;
+	unsigned int du_filled = 0;
 	blk_status_t blk_st;
 
+	/* Allocate scatterlist if needed */
+	if (!blk_crypto_alloc_sglists(bio, &f_ctx->crypt_iter, data_unit_size,
+				      &sg, NULL)) {
+		bio->bi_status = BLK_STS_RESOURCE;
+		goto out_no_sglists;
+	}
+
 	/*
 	 * Use the crypto API fallback keyslot manager to get a crypto_skcipher
 	 * for the algorithm and key specified for this bio.
@@ -412,33 +506,48 @@ static void blk_crypto_fallback_decrypt_bio(struct work_struct *work)
 	}
 
 	memcpy(curr_dun, bc->bc_dun, sizeof(curr_dun));
-	sg_init_table(&sg, 1);
-	skcipher_request_set_crypt(ciph_req, &sg, &sg, data_unit_size,
-				   iv.bytes);
+	skcipher_request_set_crypt(ciph_req, sg, sg, data_unit_size, iv.bytes);
 
-	/* Decrypt each segment in the bio */
+	/*
+	 * Decrypt each data unit in the bio.
+	 *
+	 * Take care to handle the case where a data unit spans bio segments.
+	 * This can happen when data_unit_size > logical_block_size.
+	 */
 	__bio_for_each_segment(bv, bio, iter, f_ctx->crypt_iter) {
-		struct page *page = bv.bv_page;
-
-		sg_set_page(&sg, page, data_unit_size, bv.bv_offset);
-
-		/* Decrypt each data unit in the segment */
-		for (i = 0; i < bv.bv_len; i += data_unit_size) {
-			blk_crypto_dun_to_iv(curr_dun, &iv);
-			if (crypto_wait_req(crypto_skcipher_decrypt(ciph_req),
-					    &wait)) {
-				bio->bi_status = BLK_STS_IOERR;
-				goto out;
+		unsigned int offset_in_bv = 0;
+
+		while (offset_in_bv < bv.bv_len) {
+			unsigned int n = min(bv.bv_len - offset_in_bv,
+					     data_unit_size - du_filled);
+			sg_set_page(&sg[sg_idx++], bv.bv_page, n,
+				    bv.bv_offset + offset_in_bv);
+			offset_in_bv += n;
+			du_filled += n;
+			if (du_filled == data_unit_size) {
+				blk_crypto_dun_to_iv(curr_dun, &iv);
+				if (crypto_wait_req(crypto_skcipher_decrypt(ciph_req),
+						    &wait)) {
+					bio->bi_status = BLK_STS_IOERR;
+					goto out;
+				}
+				bio_crypt_dun_increment(curr_dun, 1);
+				sg_idx = 0;
+				du_filled = 0;
 			}
-			bio_crypt_dun_increment(curr_dun, 1);
-			sg.offset += data_unit_size;
 		}
 	}
-
+	if (WARN_ON_ONCE(du_filled != 0)) {
+		bio->bi_status = BLK_STS_IOERR;
+		goto out;
+	}
 out:
 	skcipher_request_free(ciph_req);
 	blk_ksm_put_slot(slot);
 out_no_keyslot:
+	if (sg != &_sg)
+		kfree(sg);
+out_no_sglists:
 	mempool_free(f_ctx, bio_fallback_crypt_ctx_pool);
 	bio_endio(bio);
 }
-- 
2.32.0.rc1.229.g3e70b5a671-goog


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

* [PATCH v9 2/9] block: blk-crypto: relax alignment requirements for bvecs in bios
  2021-06-04 21:08 [PATCH v9 0/9] add support for direct I/O with fscrypt using blk-crypto Satya Tangirala
  2021-06-04 21:09 ` [PATCH v9 1/9] block: blk-crypto-fallback: handle data unit split across multiple bvecs Satya Tangirala
@ 2021-06-04 21:09 ` Satya Tangirala
  2021-06-04 21:09 ` [PATCH v9 3/9] fscrypt: add functions for direct I/O support Satya Tangirala
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: Satya Tangirala @ 2021-06-04 21:09 UTC (permalink / raw)
  To: Theodore Y . Ts'o, Jaegeuk Kim, Eric Biggers, Chao Yu,
	Jens Axboe, Darrick J . Wong
  Cc: linux-kernel, linux-fscrypt, linux-f2fs-devel, linux-xfs,
	linux-block, linux-ext4, Satya Tangirala, Eric Biggers

blk-crypto only accepted bios whose bvecs' offsets and lengths were aligned
to the crypto data unit size, since blk-crypto-fallback required that to
work correctly.

Now that the blk-crypto-fallback has been updated to work without that
assumption, we relax the alignment requirement - blk-crypto now only needs
the total size of the bio to be aligned to the crypto data unit size.

Co-developed-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: Satya Tangirala <satyat@google.com>
---
 block/blk-crypto.c | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

diff --git a/block/blk-crypto.c b/block/blk-crypto.c
index c5bdaafffa29..06f81e64151d 100644
--- a/block/blk-crypto.c
+++ b/block/blk-crypto.c
@@ -200,22 +200,6 @@ bool bio_crypt_ctx_mergeable(struct bio_crypt_ctx *bc1, unsigned int bc1_bytes,
 	return !bc1 || bio_crypt_dun_is_contiguous(bc1, bc1_bytes, bc2->bc_dun);
 }
 
-/* Check that all I/O segments are data unit aligned. */
-static bool bio_crypt_check_alignment(struct bio *bio)
-{
-	const unsigned int data_unit_size =
-		bio->bi_crypt_context->bc_key->crypto_cfg.data_unit_size;
-	struct bvec_iter iter;
-	struct bio_vec bv;
-
-	bio_for_each_segment(bv, bio, iter) {
-		if (!IS_ALIGNED(bv.bv_len | bv.bv_offset, data_unit_size))
-			return false;
-	}
-
-	return true;
-}
-
 blk_status_t __blk_crypto_init_request(struct request *rq)
 {
 	return blk_ksm_get_slot_for_key(rq->q->ksm, rq->crypt_ctx->bc_key,
@@ -271,7 +255,8 @@ bool __blk_crypto_bio_prep(struct bio **bio_ptr)
 		goto fail;
 	}
 
-	if (!bio_crypt_check_alignment(bio)) {
+	if (!IS_ALIGNED(bio->bi_iter.bi_size,
+			bc_key->crypto_cfg.data_unit_size)) {
 		bio->bi_status = BLK_STS_IOERR;
 		goto fail;
 	}
-- 
2.32.0.rc1.229.g3e70b5a671-goog


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

* [PATCH v9 3/9] fscrypt: add functions for direct I/O support
  2021-06-04 21:08 [PATCH v9 0/9] add support for direct I/O with fscrypt using blk-crypto Satya Tangirala
  2021-06-04 21:09 ` [PATCH v9 1/9] block: blk-crypto-fallback: handle data unit split across multiple bvecs Satya Tangirala
  2021-06-04 21:09 ` [PATCH v9 2/9] block: blk-crypto: relax alignment requirements for bvecs in bios Satya Tangirala
@ 2021-06-04 21:09 ` Satya Tangirala
  2021-07-23 20:42   ` Eric Biggers
  2021-06-04 21:09 ` [PATCH v9 4/9] direct-io: add support for fscrypt using blk-crypto Satya Tangirala
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 18+ messages in thread
From: Satya Tangirala @ 2021-06-04 21:09 UTC (permalink / raw)
  To: Theodore Y . Ts'o, Jaegeuk Kim, Eric Biggers, Chao Yu,
	Jens Axboe, Darrick J . Wong
  Cc: linux-kernel, linux-fscrypt, linux-f2fs-devel, linux-xfs,
	linux-block, linux-ext4, 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_blocks() to limit how many blocks can be
added to a bio being prepared for direct I/O. This is needed for
filesystems that use 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, so
filesystems using iomap won't have a chance to call fscrypt_mergeable_bio()
on every block added to a bio. 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 | 75 ++++++++++++++++++++++++++++++++++++++++
 include/linux/fscrypt.h  | 18 ++++++++++
 3 files changed, 101 insertions(+)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 4ef3f714046a..4fcca79f39ae 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_blocks().  fscrypt_limit_io_blocks()
+ * 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 c57bebfa48fe..a7ce650fbd3b 100644
--- a/fs/crypto/inline_crypt.c
+++ b/fs/crypto/inline_crypt.c
@@ -17,6 +17,7 @@
 #include <linux/buffer_head.h>
 #include <linux/sched/mm.h>
 #include <linux/slab.h>
+#include <linux/uio.h>
 
 #include "fscrypt_private.h"
 
@@ -363,3 +364,77 @@ 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_count(iter), blocksize))
+		return false;
+
+	return true;
+}
+EXPORT_SYMBOL_GPL(fscrypt_dio_supported);
+
+/**
+ * fscrypt_limit_io_blocks() - limit I/O blocks to avoid discontiguous DUNs
+ * @inode: the file on which I/O is being done
+ * @lblk: the block at which the I/O is being started from
+ * @nr_blocks: the number of blocks we want to submit starting at @lblk
+ *
+ * Determine the limit to the number of blocks that can be submitted in the bio
+ * targeting @lblk without causing a data unit number (DUN) discontinuity.
+ *
+ * This is normally just @nr_blocks, 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_blocks will be returned
+ * so that the wraparound doesn't occur in the middle of the bio.
+ *
+ * Return: the actual number of blocks that can be submitted
+ */
+u64 fscrypt_limit_io_blocks(const struct inode *inode, u64 lblk, u64 nr_blocks)
+{
+	const struct fscrypt_info *ci = inode->i_crypt_info;
+	u32 dun;
+
+	if (!fscrypt_inode_uses_inline_crypto(inode))
+		return nr_blocks;
+
+	if (nr_blocks <= 1)
+		return nr_blocks;
+
+	if (!(fscrypt_policy_flags(&ci->ci_policy) &
+	      FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32))
+		return nr_blocks;
+
+	/* With IV_INO_LBLK_32, the DUN can wrap around from U32_MAX to 0. */
+
+	dun = ci->ci_hashed_ino + lblk;
+
+	return min_t(u64, nr_blocks, (u64)U32_MAX + 1 - dun);
+}
+EXPORT_SYMBOL_GPL(fscrypt_limit_io_blocks);
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 2ea1387bb497..d8dde02aee82 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -609,6 +609,10 @@ 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);
+
+u64 fscrypt_limit_io_blocks(const struct inode *inode, u64 lblk, u64 nr_blocks);
+
 #else /* CONFIG_FS_ENCRYPTION_INLINE_CRYPT */
 
 static inline bool __fscrypt_inode_uses_inline_crypto(const struct inode *inode)
@@ -637,6 +641,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 u64 fscrypt_limit_io_blocks(const struct inode *inode, u64 lblk,
+					  u64 nr_blocks)
+{
+	return nr_blocks;
+}
 #endif /* !CONFIG_FS_ENCRYPTION_INLINE_CRYPT */
 
 /**
-- 
2.32.0.rc1.229.g3e70b5a671-goog


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

* [PATCH v9 4/9] direct-io: add support for fscrypt using blk-crypto
  2021-06-04 21:08 [PATCH v9 0/9] add support for direct I/O with fscrypt using blk-crypto Satya Tangirala
                   ` (2 preceding siblings ...)
  2021-06-04 21:09 ` [PATCH v9 3/9] fscrypt: add functions for direct I/O support Satya Tangirala
@ 2021-06-04 21:09 ` Satya Tangirala
  2021-06-04 21:09 ` [PATCH v9 5/9] block: Make bio_iov_iter_get_pages() respect bio_required_sector_alignment() Satya Tangirala
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: Satya Tangirala @ 2021-06-04 21:09 UTC (permalink / raw)
  To: Theodore Y . Ts'o, Jaegeuk Kim, Eric Biggers, Chao Yu,
	Jens Axboe, Darrick J . Wong
  Cc: linux-kernel, linux-fscrypt, linux-f2fs-devel, linux-xfs,
	linux-block, linux-ext4, 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>
Reviewed-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 b2e86e739d7a..328ed7ac0094 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>
@@ -392,6 +393,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
@@ -399,6 +401,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);
@@ -765,9 +770,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.32.0.rc1.229.g3e70b5a671-goog


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

* [PATCH v9 5/9] block: Make bio_iov_iter_get_pages() respect bio_required_sector_alignment()
  2021-06-04 21:08 [PATCH v9 0/9] add support for direct I/O with fscrypt using blk-crypto Satya Tangirala
                   ` (3 preceding siblings ...)
  2021-06-04 21:09 ` [PATCH v9 4/9] direct-io: add support for fscrypt using blk-crypto Satya Tangirala
@ 2021-06-04 21:09 ` Satya Tangirala
  2021-06-04 22:51   ` kernel test robot
                     ` (2 more replies)
  2021-06-04 21:09 ` [PATCH v9 6/9] iomap: support direct I/O with fscrypt using blk-crypto Satya Tangirala
                   ` (4 subsequent siblings)
  9 siblings, 3 replies; 18+ messages in thread
From: Satya Tangirala @ 2021-06-04 21:09 UTC (permalink / raw)
  To: Theodore Y . Ts'o, Jaegeuk Kim, Eric Biggers, Chao Yu,
	Jens Axboe, Darrick J . Wong
  Cc: linux-kernel, linux-fscrypt, linux-f2fs-devel, linux-xfs,
	linux-block, linux-ext4, Satya Tangirala

Previously, bio_iov_iter_get_pages() wasn't used with bios that could have
an encryption context. However, direct I/O support using blk-crypto
introduces this possibility, so this function must now respect
bio_required_sector_alignment() (otherwise, xfstests like generic/465 with
ext4 will fail).

Signed-off-by: Satya Tangirala <satyat@google.com>
---
 block/bio.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/block/bio.c b/block/bio.c
index 32f75f31bb5c..99c510f706e2 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1099,7 +1099,8 @@ static int __bio_iov_append_get_pages(struct bio *bio, struct iov_iter *iter)
  * The function tries, but does not guarantee, to pin as many pages as
  * fit into the bio, or are requested in @iter, whatever is smaller. If
  * MM encounters an error pinning the requested pages, it stops. Error
- * is returned only if 0 pages could be pinned.
+ * is returned only if 0 pages could be pinned. It also ensures that the number
+ * of sectors added to the bio is aligned to bio_required_sector_alignment().
  *
  * It's intended for direct IO, so doesn't do PSI tracking, the caller is
  * responsible for setting BIO_WORKINGSET if necessary.
@@ -1107,6 +1108,7 @@ static int __bio_iov_append_get_pages(struct bio *bio, struct iov_iter *iter)
 int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
 {
 	int ret = 0;
+	unsigned int aligned_sectors;
 
 	if (iov_iter_is_bvec(iter)) {
 		if (bio_op(bio) == REQ_OP_ZONE_APPEND)
@@ -1121,6 +1123,15 @@ int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
 			ret = __bio_iov_iter_get_pages(bio, iter);
 	} while (!ret && iov_iter_count(iter) && !bio_full(bio, 0));
 
+	/*
+	 * Ensure that number of sectors in bio is aligned to
+	 * bio_required_sector_align()
+	 */
+	aligned_sectors = round_down(bio_sectors(bio),
+				     bio_required_sector_alignment(bio));
+	iov_iter_revert(iter, (bio_sectors(bio) - aligned_sectors) << SECTOR_SHIFT);
+	bio_truncate(bio, aligned_sectors << SECTOR_SHIFT);
+
 	/* don't account direct I/O as memory stall */
 	bio_clear_flag(bio, BIO_WORKINGSET);
 	return bio->bi_vcnt ? 0 : ret;
-- 
2.32.0.rc1.229.g3e70b5a671-goog


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

* [PATCH v9 6/9] iomap: support direct I/O with fscrypt using blk-crypto
  2021-06-04 21:08 [PATCH v9 0/9] add support for direct I/O with fscrypt using blk-crypto Satya Tangirala
                   ` (4 preceding siblings ...)
  2021-06-04 21:09 ` [PATCH v9 5/9] block: Make bio_iov_iter_get_pages() respect bio_required_sector_alignment() Satya Tangirala
@ 2021-06-04 21:09 ` Satya Tangirala
  2021-07-22 19:04   ` Darrick J. Wong
  2021-06-04 21:09 ` [PATCH v9 7/9] ext4: " Satya Tangirala
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 18+ messages in thread
From: Satya Tangirala @ 2021-06-04 21:09 UTC (permalink / raw)
  To: Theodore Y . Ts'o, Jaegeuk Kim, Eric Biggers, Chao Yu,
	Jens Axboe, Darrick J . Wong
  Cc: linux-kernel, linux-fscrypt, linux-f2fs-devel, linux-xfs,
	linux-block, linux-ext4, Eric Biggers, Satya Tangirala

From: Eric Biggers <ebiggers@google.com>

Set bio crypt contexts on bios by calling into fscrypt when required.
No DUN contiguity checks are done - callers are expected to set up the
iomap correctly to ensure that each bio submitted by iomap will not have
blocks with incontiguous DUNs by calling fscrypt_limit_io_blocks()
appropriately.

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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index 9398b8c31323..1c825deb36a9 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>
@@ -185,11 +186,14 @@ 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);
+	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_private = dio;
@@ -306,6 +310,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;
-- 
2.32.0.rc1.229.g3e70b5a671-goog


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

* [PATCH v9 7/9] ext4: support direct I/O with fscrypt using blk-crypto
  2021-06-04 21:08 [PATCH v9 0/9] add support for direct I/O with fscrypt using blk-crypto Satya Tangirala
                   ` (5 preceding siblings ...)
  2021-06-04 21:09 ` [PATCH v9 6/9] iomap: support direct I/O with fscrypt using blk-crypto Satya Tangirala
@ 2021-06-04 21:09 ` Satya Tangirala
  2021-06-04 21:09 ` [PATCH v9 8/9] f2fs: " Satya Tangirala
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: Satya Tangirala @ 2021-06-04 21:09 UTC (permalink / raw)
  To: Theodore Y . Ts'o, Jaegeuk Kim, Eric Biggers, Chao Yu,
	Jens Axboe, Darrick J . Wong
  Cc: linux-kernel, linux-fscrypt, linux-f2fs-devel, linux-xfs,
	linux-block, linux-ext4, 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 the *length* of the
I/O is aligned to the filesystem block size (which is *not* necessarily the
same as the block device's block size).

fscrypt_limit_io_blocks() is called before setting up the iomap to ensure
that the blocks of each bio that iomap will submit will have contiguous
DUNs. Note that fscrypt_limit_io_blocks() 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>
Acked-by: Theodore Ts'o <tytso@mit.edu>
---
 fs/ext4/file.c  | 10 ++++++----
 fs/ext4/inode.c |  7 +++++++
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 816dedcbd541..a2898a496c4e 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
@@ -511,7 +513,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
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index fe6045a46599..fe8006efb5ef 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3481,6 +3481,13 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
 	if (ret < 0)
 		return ret;
 out:
+	/*
+	 * 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.
+	 */
+	map.m_len = fscrypt_limit_io_blocks(inode, map.m_lblk, map.m_len);
+
 	ext4_set_iomap(inode, iomap, &map, offset, length);
 
 	return 0;
-- 
2.32.0.rc1.229.g3e70b5a671-goog


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

* [PATCH v9 8/9] f2fs: support direct I/O with fscrypt using blk-crypto
  2021-06-04 21:08 [PATCH v9 0/9] add support for direct I/O with fscrypt using blk-crypto Satya Tangirala
                   ` (6 preceding siblings ...)
  2021-06-04 21:09 ` [PATCH v9 7/9] ext4: " Satya Tangirala
@ 2021-06-04 21:09 ` Satya Tangirala
  2021-06-04 21:09 ` [PATCH v9 9/9] fscrypt: update documentation for direct I/O support Satya Tangirala
       [not found] ` <CAF2Aj3h-Gt3bOxH4wXB7aeQ3jVzR3TEqd3uLsh4T9Q=e6W6iqQ@mail.gmail.com>
  9 siblings, 0 replies; 18+ messages in thread
From: Satya Tangirala @ 2021-06-04 21:09 UTC (permalink / raw)
  To: Theodore Y . Ts'o, Jaegeuk Kim, Eric Biggers, Chao Yu,
	Jens Axboe, Darrick J . Wong
  Cc: linux-kernel, linux-fscrypt, linux-f2fs-devel, linux-xfs,
	linux-block, linux-ext4, 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 the *length* of the
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 c83d90125ebd..a416ea3a1a04 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -4181,7 +4181,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.32.0.rc1.229.g3e70b5a671-goog


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

* [PATCH v9 9/9] fscrypt: update documentation for direct I/O support
  2021-06-04 21:08 [PATCH v9 0/9] add support for direct I/O with fscrypt using blk-crypto Satya Tangirala
                   ` (7 preceding siblings ...)
  2021-06-04 21:09 ` [PATCH v9 8/9] f2fs: " Satya Tangirala
@ 2021-06-04 21:09 ` Satya Tangirala
       [not found] ` <CAF2Aj3h-Gt3bOxH4wXB7aeQ3jVzR3TEqd3uLsh4T9Q=e6W6iqQ@mail.gmail.com>
  9 siblings, 0 replies; 18+ messages in thread
From: Satya Tangirala @ 2021-06-04 21:09 UTC (permalink / raw)
  To: Theodore Y . Ts'o, Jaegeuk Kim, Eric Biggers, Chao Yu,
	Jens Axboe, Darrick J . Wong
  Cc: linux-kernel, linux-fscrypt, linux-f2fs-devel, linux-xfs,
	linux-block, linux-ext4, Satya Tangirala, Eric Biggers

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>
---
 Documentation/filesystems/fscrypt.rst | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst
index 44b67ebd6e40..c0c1747fa2fb 100644
--- a/Documentation/filesystems/fscrypt.rst
+++ b/Documentation/filesystems/fscrypt.rst
@@ -1047,8 +1047,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
@@ -1121,6 +1123,21 @@ 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,
+the starting position in the file and the length of any I/O must be aligned
+to the filesystem block size (*not* necessarily the same as the block
+device's 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. However,
+there aren't any additional requirements on user buffer alignment (apart
+from those already present when using direct I/O on unencrypted files).
+
 Encryption policy enforcement
 =============================
 
-- 
2.32.0.rc1.229.g3e70b5a671-goog


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

* Re: [PATCH v9 5/9] block: Make bio_iov_iter_get_pages() respect bio_required_sector_alignment()
  2021-06-04 21:09 ` [PATCH v9 5/9] block: Make bio_iov_iter_get_pages() respect bio_required_sector_alignment() Satya Tangirala
@ 2021-06-04 22:51   ` kernel test robot
  2021-06-05  2:55   ` kernel test robot
  2021-07-23 21:33   ` Eric Biggers
  2 siblings, 0 replies; 18+ messages in thread
From: kernel test robot @ 2021-06-04 22:51 UTC (permalink / raw)
  To: Satya Tangirala, Theodore Y . Ts'o, Jaegeuk Kim,
	Eric Biggers, Chao Yu, Chao Yu, Jens Axboe, Darrick J . Wong
  Cc: kbuild-all, linux-kernel, linux-fscrypt, linux-f2fs-devel, linux-xfs

[-- Attachment #1: Type: text/plain, Size: 5290 bytes --]

Hi Satya,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on block/for-next]
[also build test ERROR on ext4/dev f2fs/dev-test linus/master v5.13-rc4 next-20210604]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Satya-Tangirala/add-support-for-direct-I-O-with-fscrypt-using-blk-crypto/20210605-051023
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: um-x86_64_defconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/eea1225f680da16dce01bfb2914b9e8b78de298d
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Satya-Tangirala/add-support-for-direct-I-O-with-fscrypt-using-blk-crypto/20210605-051023
        git checkout eea1225f680da16dce01bfb2914b9e8b78de298d
        # save the attached .config to linux build tree
        make W=1 ARCH=um SUBARCH=x86_64

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/kernel.h:14,
                    from include/asm-generic/bug.h:20,
                    from ./arch/um/include/generated/asm/bug.h:1,
                    from include/linux/bug.h:5,
                    from include/linux/mmdebug.h:5,
                    from include/linux/mm.h:9,
                    from block/bio.c:5:
   block/bio.c: In function 'bio_iov_iter_get_pages':
>> block/bio.c:1131:10: error: implicit declaration of function 'bio_required_sector_alignment' [-Werror=implicit-function-declaration]
    1131 |          bio_required_sector_alignment(bio));
         |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/math.h:14:46: note: in definition of macro '__round_mask'
      14 | #define __round_mask(x, y) ((__typeof__(x))((y)-1))
         |                                              ^
   block/bio.c:1130:20: note: in expansion of macro 'round_down'
    1130 |  aligned_sectors = round_down(bio_sectors(bio),
         |                    ^~~~~~~~~~
   cc1: some warnings being treated as errors


vim +/bio_required_sector_alignment +1131 block/bio.c

  1083	
  1084	/**
  1085	 * bio_iov_iter_get_pages - add user or kernel pages to a bio
  1086	 * @bio: bio to add pages to
  1087	 * @iter: iov iterator describing the region to be added
  1088	 *
  1089	 * This takes either an iterator pointing to user memory, or one pointing to
  1090	 * kernel pages (BVEC iterator). If we're adding user pages, we pin them and
  1091	 * map them into the kernel. On IO completion, the caller should put those
  1092	 * pages. For bvec based iterators bio_iov_iter_get_pages() uses the provided
  1093	 * bvecs rather than copying them. Hence anyone issuing kiocb based IO needs
  1094	 * to ensure the bvecs and pages stay referenced until the submitted I/O is
  1095	 * completed by a call to ->ki_complete() or returns with an error other than
  1096	 * -EIOCBQUEUED. The caller needs to check if the bio is flagged BIO_NO_PAGE_REF
  1097	 * on IO completion. If it isn't, then pages should be released.
  1098	 *
  1099	 * The function tries, but does not guarantee, to pin as many pages as
  1100	 * fit into the bio, or are requested in @iter, whatever is smaller. If
  1101	 * MM encounters an error pinning the requested pages, it stops. Error
  1102	 * is returned only if 0 pages could be pinned. It also ensures that the number
  1103	 * of sectors added to the bio is aligned to bio_required_sector_alignment().
  1104	 *
  1105	 * It's intended for direct IO, so doesn't do PSI tracking, the caller is
  1106	 * responsible for setting BIO_WORKINGSET if necessary.
  1107	 */
  1108	int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
  1109	{
  1110		int ret = 0;
  1111		unsigned int aligned_sectors;
  1112	
  1113		if (iov_iter_is_bvec(iter)) {
  1114			if (bio_op(bio) == REQ_OP_ZONE_APPEND)
  1115				return bio_iov_bvec_set_append(bio, iter);
  1116			return bio_iov_bvec_set(bio, iter);
  1117		}
  1118	
  1119		do {
  1120			if (bio_op(bio) == REQ_OP_ZONE_APPEND)
  1121				ret = __bio_iov_append_get_pages(bio, iter);
  1122			else
  1123				ret = __bio_iov_iter_get_pages(bio, iter);
  1124		} while (!ret && iov_iter_count(iter) && !bio_full(bio, 0));
  1125	
  1126		/*
  1127		 * Ensure that number of sectors in bio is aligned to
  1128		 * bio_required_sector_align()
  1129		 */
  1130		aligned_sectors = round_down(bio_sectors(bio),
> 1131					     bio_required_sector_alignment(bio));
  1132		iov_iter_revert(iter, (bio_sectors(bio) - aligned_sectors) << SECTOR_SHIFT);
  1133		bio_truncate(bio, aligned_sectors << SECTOR_SHIFT);
  1134	
  1135		/* don't account direct I/O as memory stall */
  1136		bio_clear_flag(bio, BIO_WORKINGSET);
  1137		return bio->bi_vcnt ? 0 : ret;
  1138	}
  1139	EXPORT_SYMBOL_GPL(bio_iov_iter_get_pages);
  1140	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 8635 bytes --]

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

* Re: [PATCH v9 5/9] block: Make bio_iov_iter_get_pages() respect bio_required_sector_alignment()
  2021-06-04 21:09 ` [PATCH v9 5/9] block: Make bio_iov_iter_get_pages() respect bio_required_sector_alignment() Satya Tangirala
  2021-06-04 22:51   ` kernel test robot
@ 2021-06-05  2:55   ` kernel test robot
  2021-07-23 21:33   ` Eric Biggers
  2 siblings, 0 replies; 18+ messages in thread
From: kernel test robot @ 2021-06-05  2:55 UTC (permalink / raw)
  To: Satya Tangirala, Theodore Y . Ts'o, Jaegeuk Kim,
	Eric Biggers, Chao Yu, Chao Yu, Jens Axboe, Darrick J . Wong
  Cc: kbuild-all, clang-built-linux, linux-kernel, linux-fscrypt,
	linux-f2fs-devel, linux-xfs

[-- Attachment #1: Type: text/plain, Size: 5687 bytes --]

Hi Satya,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on block/for-next]
[also build test ERROR on ext4/dev f2fs/dev-test linus/master v5.13-rc4 next-20210604]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Satya-Tangirala/add-support-for-direct-I-O-with-fscrypt-using-blk-crypto/20210605-051023
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: powerpc64-randconfig-r025-20210604 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 5c0d1b2f902aa6a9cf47cc7e42c5b83bb2217cf9)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install powerpc64 cross compiling tool for clang build
        # apt-get install binutils-powerpc64-linux-gnu
        # https://github.com/0day-ci/linux/commit/eea1225f680da16dce01bfb2914b9e8b78de298d
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Satya-Tangirala/add-support-for-direct-I-O-with-fscrypt-using-blk-crypto/20210605-051023
        git checkout eea1225f680da16dce01bfb2914b9e8b78de298d
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from block/bio.c:5:
   In file included from include/linux/mm.h:9:
   In file included from include/linux/mmdebug.h:5:
   In file included from include/linux/bug.h:5:
   In file included from arch/powerpc/include/asm/bug.h:109:
   In file included from include/asm-generic/bug.h:20:
   In file included from include/linux/kernel.h:12:
   In file included from include/linux/bitops.h:32:
   In file included from arch/powerpc/include/asm/bitops.h:62:
   arch/powerpc/include/asm/barrier.h:49:9: warning: '__lwsync' macro redefined [-Wmacro-redefined]
   #define __lwsync()      __asm__ __volatile__ (stringify_in_c(LWSYNC) : : :"memory")
           ^
   <built-in>:309:9: note: previous definition is here
   #define __lwsync __builtin_ppc_lwsync
           ^
>> block/bio.c:1131:10: error: implicit declaration of function 'bio_required_sector_alignment' [-Werror,-Wimplicit-function-declaration]
                                        bio_required_sector_alignment(bio));
                                        ^
   1 warning and 1 error generated.


vim +/bio_required_sector_alignment +1131 block/bio.c

  1083	
  1084	/**
  1085	 * bio_iov_iter_get_pages - add user or kernel pages to a bio
  1086	 * @bio: bio to add pages to
  1087	 * @iter: iov iterator describing the region to be added
  1088	 *
  1089	 * This takes either an iterator pointing to user memory, or one pointing to
  1090	 * kernel pages (BVEC iterator). If we're adding user pages, we pin them and
  1091	 * map them into the kernel. On IO completion, the caller should put those
  1092	 * pages. For bvec based iterators bio_iov_iter_get_pages() uses the provided
  1093	 * bvecs rather than copying them. Hence anyone issuing kiocb based IO needs
  1094	 * to ensure the bvecs and pages stay referenced until the submitted I/O is
  1095	 * completed by a call to ->ki_complete() or returns with an error other than
  1096	 * -EIOCBQUEUED. The caller needs to check if the bio is flagged BIO_NO_PAGE_REF
  1097	 * on IO completion. If it isn't, then pages should be released.
  1098	 *
  1099	 * The function tries, but does not guarantee, to pin as many pages as
  1100	 * fit into the bio, or are requested in @iter, whatever is smaller. If
  1101	 * MM encounters an error pinning the requested pages, it stops. Error
  1102	 * is returned only if 0 pages could be pinned. It also ensures that the number
  1103	 * of sectors added to the bio is aligned to bio_required_sector_alignment().
  1104	 *
  1105	 * It's intended for direct IO, so doesn't do PSI tracking, the caller is
  1106	 * responsible for setting BIO_WORKINGSET if necessary.
  1107	 */
  1108	int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
  1109	{
  1110		int ret = 0;
  1111		unsigned int aligned_sectors;
  1112	
  1113		if (iov_iter_is_bvec(iter)) {
  1114			if (bio_op(bio) == REQ_OP_ZONE_APPEND)
  1115				return bio_iov_bvec_set_append(bio, iter);
  1116			return bio_iov_bvec_set(bio, iter);
  1117		}
  1118	
  1119		do {
  1120			if (bio_op(bio) == REQ_OP_ZONE_APPEND)
  1121				ret = __bio_iov_append_get_pages(bio, iter);
  1122			else
  1123				ret = __bio_iov_iter_get_pages(bio, iter);
  1124		} while (!ret && iov_iter_count(iter) && !bio_full(bio, 0));
  1125	
  1126		/*
  1127		 * Ensure that number of sectors in bio is aligned to
  1128		 * bio_required_sector_align()
  1129		 */
  1130		aligned_sectors = round_down(bio_sectors(bio),
> 1131					     bio_required_sector_alignment(bio));
  1132		iov_iter_revert(iter, (bio_sectors(bio) - aligned_sectors) << SECTOR_SHIFT);
  1133		bio_truncate(bio, aligned_sectors << SECTOR_SHIFT);
  1134	
  1135		/* don't account direct I/O as memory stall */
  1136		bio_clear_flag(bio, BIO_WORKINGSET);
  1137		return bio->bi_vcnt ? 0 : ret;
  1138	}
  1139	EXPORT_SYMBOL_GPL(bio_iov_iter_get_pages);
  1140	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32520 bytes --]

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

* Re: [PATCH v9 0/9] add support for direct I/O with fscrypt using blk-crypto
       [not found] ` <CAF2Aj3h-Gt3bOxH4wXB7aeQ3jVzR3TEqd3uLsh4T9Q=e6W6iqQ@mail.gmail.com>
@ 2021-07-22 14:48   ` Eric Biggers
  2021-08-04  8:26     ` Lee Jones
  0 siblings, 1 reply; 18+ messages in thread
From: Eric Biggers @ 2021-07-22 14:48 UTC (permalink / raw)
  To: Lee Jones
  Cc: Satya Tangirala, Theodore Y . Ts'o, Jaegeuk Kim, Chao Yu,
	Jens Axboe, Darrick J . Wong, open list, linux-fscrypt,
	linux-f2fs-devel, linux-xfs, linux-block, linux-ext4

Hi Lee,

On Thu, Jul 22, 2021 at 12:23:47PM +0100, Lee Jones wrote:
> 
> No review after 7 weeks on the list.
> 
> Is there anything Satya can do to help expedite this please?
> 

This series is basically ready, but I can't apply it because it depends on the
other patch series
"[PATCH v4 0/9] ensure bios aren't split in middle of crypto data unit"
(https://lkml.kernel.org/linux-block/20210707052943.3960-1-satyaprateek2357@gmail.com/T/#u).
I will be re-reviewing that other patch series soon, but it primary needs review
by the people who work more regularly with the block layer, and it will have to
go in through the block tree (I can't apply it to the fscrypt tree).

The original version of this series didn't require so many block layer changes,
but it would have only allowed direct I/O with user buffer pointers aligned to
the filesystem block size, which was too controversial with other filesystem
developers; see the long discussion at
https://lkml.kernel.org/linux-fscrypt/20200720233739.824943-1-satyat@google.com/T/#u.

In addition, it was requested that we not add features to the "legacy" direct
I/O implementation (fs/direct-io.c), so I have a patch series in progress
"[PATCH 0/9] f2fs: use iomap for direct I/O"
(https://lkml.kernel.org/linux-f2fs-devel/20210716143919.44373-1-ebiggers@kernel.org/T/#u)
which will change f2fs to use iomap.

Also please understand that Satya has left Google, so any further work from him
on this is happening on a personal capacity in his free time.

- Eric

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

* Re: [PATCH v9 6/9] iomap: support direct I/O with fscrypt using blk-crypto
  2021-06-04 21:09 ` [PATCH v9 6/9] iomap: support direct I/O with fscrypt using blk-crypto Satya Tangirala
@ 2021-07-22 19:04   ` Darrick J. Wong
  0 siblings, 0 replies; 18+ messages in thread
From: Darrick J. Wong @ 2021-07-22 19:04 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: Theodore Y . Ts'o, Jaegeuk Kim, Eric Biggers, Chao Yu,
	Jens Axboe, Darrick J . Wong, linux-kernel, linux-fscrypt,
	linux-f2fs-devel, linux-xfs, linux-block, linux-ext4,
	Eric Biggers

On Fri, Jun 04, 2021 at 09:09:05PM +0000, Satya Tangirala wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Set bio crypt contexts on bios by calling into fscrypt when required.
> No DUN contiguity checks are done - callers are expected to set up the
> iomap correctly to ensure that each bio submitted by iomap will not have
> blocks with incontiguous DUNs by calling fscrypt_limit_io_blocks()
> appropriately.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> Co-developed-by: Satya Tangirala <satyat@google.com>
> Signed-off-by: Satya Tangirala <satyat@google.com>

Looks like a straightforward conversion...

Acked-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/iomap/direct-io.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> index 9398b8c31323..1c825deb36a9 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>
> @@ -185,11 +186,14 @@ 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);
> +	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_private = dio;
> @@ -306,6 +310,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;
> -- 
> 2.32.0.rc1.229.g3e70b5a671-goog
> 

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

* Re: [PATCH v9 3/9] fscrypt: add functions for direct I/O support
  2021-06-04 21:09 ` [PATCH v9 3/9] fscrypt: add functions for direct I/O support Satya Tangirala
@ 2021-07-23 20:42   ` Eric Biggers
  0 siblings, 0 replies; 18+ messages in thread
From: Eric Biggers @ 2021-07-23 20:42 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: Theodore Y . Ts'o, Jaegeuk Kim, Chao Yu, Jens Axboe,
	Darrick J . Wong, linux-kernel, linux-fscrypt, linux-f2fs-devel,
	linux-xfs, linux-block, linux-ext4

On Fri, Jun 04, 2021 at 09:09:02PM +0000, Satya Tangirala wrote:
> +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_count(iter), blocksize))
> +		return false;

The above comment should make it clear that "block aligned" here intentionally
applies to just the position and total length, not to the individual data
buffers, for which only disk sector alignment is required.

- Eric

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

* Re: [PATCH v9 5/9] block: Make bio_iov_iter_get_pages() respect bio_required_sector_alignment()
  2021-06-04 21:09 ` [PATCH v9 5/9] block: Make bio_iov_iter_get_pages() respect bio_required_sector_alignment() Satya Tangirala
  2021-06-04 22:51   ` kernel test robot
  2021-06-05  2:55   ` kernel test robot
@ 2021-07-23 21:33   ` Eric Biggers
  2021-07-24  7:19     ` Christoph Hellwig
  2 siblings, 1 reply; 18+ messages in thread
From: Eric Biggers @ 2021-07-23 21:33 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: Theodore Y . Ts'o, Jaegeuk Kim, Chao Yu, Jens Axboe,
	Darrick J . Wong, linux-kernel, linux-fscrypt, linux-f2fs-devel,
	linux-xfs, linux-block, linux-ext4

On Fri, Jun 04, 2021 at 09:09:04PM +0000, Satya Tangirala wrote:
> Previously, bio_iov_iter_get_pages() wasn't used with bios that could have
> an encryption context. However, direct I/O support using blk-crypto
> introduces this possibility, so this function must now respect
> bio_required_sector_alignment() (otherwise, xfstests like generic/465 with
> ext4 will fail).

Can you be more clear that the fscrypt direct I/O support only requires this in
order to support I/O segments that aren't fs-block aligned?

I do still wonder if we should just not support that...  Dave is the only person
who has asked for it, and it's a lot of trouble to support.

I also noticed that f2fs has always only supported direct I/O that is *fully*
fs-block aligned (including the I/O segments) anyway.  So presumably that
limitation is not really that important after all...

Does anyone else have thoughts on this?

One more comment on this patch below:

> 
> Signed-off-by: Satya Tangirala <satyat@google.com>
> ---
>  block/bio.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/block/bio.c b/block/bio.c
> index 32f75f31bb5c..99c510f706e2 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -1099,7 +1099,8 @@ static int __bio_iov_append_get_pages(struct bio *bio, struct iov_iter *iter)
>   * The function tries, but does not guarantee, to pin as many pages as
>   * fit into the bio, or are requested in @iter, whatever is smaller. If
>   * MM encounters an error pinning the requested pages, it stops. Error
> - * is returned only if 0 pages could be pinned.
> + * is returned only if 0 pages could be pinned. It also ensures that the number
> + * of sectors added to the bio is aligned to bio_required_sector_alignment().
>   *
>   * It's intended for direct IO, so doesn't do PSI tracking, the caller is
>   * responsible for setting BIO_WORKINGSET if necessary.
> @@ -1107,6 +1108,7 @@ static int __bio_iov_append_get_pages(struct bio *bio, struct iov_iter *iter)
>  int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
>  {
>  	int ret = 0;
> +	unsigned int aligned_sectors;
>  
>  	if (iov_iter_is_bvec(iter)) {
>  		if (bio_op(bio) == REQ_OP_ZONE_APPEND)
> @@ -1121,6 +1123,15 @@ int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
>  			ret = __bio_iov_iter_get_pages(bio, iter);
>  	} while (!ret && iov_iter_count(iter) && !bio_full(bio, 0));
>  
> +	/*
> +	 * Ensure that number of sectors in bio is aligned to
> +	 * bio_required_sector_align()
> +	 */
> +	aligned_sectors = round_down(bio_sectors(bio),
> +				     bio_required_sector_alignment(bio));
> +	iov_iter_revert(iter, (bio_sectors(bio) - aligned_sectors) << SECTOR_SHIFT);
> +	bio_truncate(bio, aligned_sectors << SECTOR_SHIFT);
> +
>  	/* don't account direct I/O as memory stall */
>  	bio_clear_flag(bio, BIO_WORKINGSET);
>  	return bio->bi_vcnt ? 0 : ret;

Doesn't this need to return an error if the bio's size gets rounded down to 0?
For example if logical_block_size=512 and data_unit_size=4096, and the iov_iter
points to 4096 bytes in 8 512-byte segments but the last one isn't mapped, then
7 pages would be pinned and the last one would fail.  This would then truncate
the bio's size to 0, but bio->bi_vcnt would be 7, so this would still return 0.
It would also be necessary to release the pages before returning an error.

- Eric

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

* Re: [PATCH v9 5/9] block: Make bio_iov_iter_get_pages() respect bio_required_sector_alignment()
  2021-07-23 21:33   ` Eric Biggers
@ 2021-07-24  7:19     ` Christoph Hellwig
  0 siblings, 0 replies; 18+ messages in thread
From: Christoph Hellwig @ 2021-07-24  7:19 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Satya Tangirala, Theodore Y . Ts'o, Jaegeuk Kim, Chao Yu,
	Jens Axboe, Darrick J . Wong, linux-kernel, linux-fscrypt,
	linux-f2fs-devel, linux-xfs, linux-block, linux-ext4

On Fri, Jul 23, 2021 at 02:33:02PM -0700, Eric Biggers wrote:
> I do still wonder if we should just not support that...  Dave is the only person
> who has asked for it, and it's a lot of trouble to support.
> 
> I also noticed that f2fs has always only supported direct I/O that is *fully*
> fs-block aligned (including the I/O segments) anyway.  So presumably that
> limitation is not really that important after all...
> 
> Does anyone else have thoughts on this?

There are some use cases that really like sector aligned direct I/O,
what comes to mind is some data bases, and file system repair tools
(the latter on the raw block device).  So it is nice to support, but not
really required.

So for now I'd much prefer to initially support inline encryption for
direct I/O without that if that simplifies the support.  We can revisit
the additional complexity later.

Also note that for cheap flash media pretending support for 512 byte
blocks is actually a bit awwkward, so just presenting the media as
having 4096 sectors in these setups would be the better choice anyway.

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

* Re: [PATCH v9 0/9] add support for direct I/O with fscrypt using blk-crypto
  2021-07-22 14:48   ` [PATCH v9 0/9] add support for direct I/O with fscrypt using blk-crypto Eric Biggers
@ 2021-08-04  8:26     ` Lee Jones
  0 siblings, 0 replies; 18+ messages in thread
From: Lee Jones @ 2021-08-04  8:26 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Satya Tangirala, Theodore Y . Ts'o, Jaegeuk Kim, Chao Yu,
	Jens Axboe, Darrick J . Wong, open list, linux-fscrypt,
	linux-f2fs-devel, linux-xfs, linux-block, linux-ext4

On Thu, 22 Jul 2021, Eric Biggers wrote:

> Hi Lee,
> 
> On Thu, Jul 22, 2021 at 12:23:47PM +0100, Lee Jones wrote:
> > 
> > No review after 7 weeks on the list.
> > 
> > Is there anything Satya can do to help expedite this please?
> > 
> 
> This series is basically ready, but I can't apply it because it depends on the
> other patch series
> "[PATCH v4 0/9] ensure bios aren't split in middle of crypto data unit"
> (https://lkml.kernel.org/linux-block/20210707052943.3960-1-satyaprateek2357@gmail.com/T/#u).
> I will be re-reviewing that other patch series soon, but it primary needs review
> by the people who work more regularly with the block layer, and it will have to
> go in through the block tree (I can't apply it to the fscrypt tree).
> 
> The original version of this series didn't require so many block layer changes,
> but it would have only allowed direct I/O with user buffer pointers aligned to
> the filesystem block size, which was too controversial with other filesystem
> developers; see the long discussion at
> https://lkml.kernel.org/linux-fscrypt/20200720233739.824943-1-satyat@google.com/T/#u.
> 
> In addition, it was requested that we not add features to the "legacy" direct
> I/O implementation (fs/direct-io.c), so I have a patch series in progress
> "[PATCH 0/9] f2fs: use iomap for direct I/O"
> (https://lkml.kernel.org/linux-f2fs-devel/20210716143919.44373-1-ebiggers@kernel.org/T/#u)
> which will change f2fs to use iomap.
> 
> Also please understand that Satya has left Google, so any further work from him
> on this is happening on a personal capacity in his free time.

Thanks for the update Eric.  I'll push this to the back of my queue
and check back with you at a later date.  Hopefully we see some
interest from the other maintainers sooner, rather than later.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

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

end of thread, other threads:[~2021-08-04  8:26 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-04 21:08 [PATCH v9 0/9] add support for direct I/O with fscrypt using blk-crypto Satya Tangirala
2021-06-04 21:09 ` [PATCH v9 1/9] block: blk-crypto-fallback: handle data unit split across multiple bvecs Satya Tangirala
2021-06-04 21:09 ` [PATCH v9 2/9] block: blk-crypto: relax alignment requirements for bvecs in bios Satya Tangirala
2021-06-04 21:09 ` [PATCH v9 3/9] fscrypt: add functions for direct I/O support Satya Tangirala
2021-07-23 20:42   ` Eric Biggers
2021-06-04 21:09 ` [PATCH v9 4/9] direct-io: add support for fscrypt using blk-crypto Satya Tangirala
2021-06-04 21:09 ` [PATCH v9 5/9] block: Make bio_iov_iter_get_pages() respect bio_required_sector_alignment() Satya Tangirala
2021-06-04 22:51   ` kernel test robot
2021-06-05  2:55   ` kernel test robot
2021-07-23 21:33   ` Eric Biggers
2021-07-24  7:19     ` Christoph Hellwig
2021-06-04 21:09 ` [PATCH v9 6/9] iomap: support direct I/O with fscrypt using blk-crypto Satya Tangirala
2021-07-22 19:04   ` Darrick J. Wong
2021-06-04 21:09 ` [PATCH v9 7/9] ext4: " Satya Tangirala
2021-06-04 21:09 ` [PATCH v9 8/9] f2fs: " Satya Tangirala
2021-06-04 21:09 ` [PATCH v9 9/9] fscrypt: update documentation for direct I/O support Satya Tangirala
     [not found] ` <CAF2Aj3h-Gt3bOxH4wXB7aeQ3jVzR3TEqd3uLsh4T9Q=e6W6iqQ@mail.gmail.com>
2021-07-22 14:48   ` [PATCH v9 0/9] add support for direct I/O with fscrypt using blk-crypto Eric Biggers
2021-08-04  8:26     ` Lee Jones

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