[PATCH 0/6] dm-crypt: misc cleanups and fixes

[PATCH 0/6] dm-crypt: misc cleanups and fixes

[Hou Tao]

From: Hou Tao <houtao1@huawei.com>

Hi,

The patchset contains misc cleanups and minor fixes for dm-crypt. These
problem were spotted during the code inspection. The patchset have been
tested by running fio on dm-crypt devices (both with and without
integrity feature). Patch #6 is tested by asynchronizing the
crypt/decrypt algorithm when using random IV.

Please see individual patches for more details. Comments are always
welcome.

Hou Tao (6):
  dm-crypt: set atomic as false when calling crypt_convert() in kworker
  dm-crypt: fully initialize clone->bi_iter in crypt_alloc_buffer()
  dm-crypt: use bi_sector in bio when initialize integrity seed
  dm-crypt: don't update io->sector after
    kcryptd_crypt_write_io_submit()
  dm-crypt: don't initialize cc_sector again
  dm-crypt: track tag_offset in convert_context

 drivers/md/dm-crypt.c | 42 ++++++++++++++++--------------------------
 1 file changed, 16 insertions(+), 26 deletions(-)

-- 
2.29.2

[PATCH 1/6] dm-crypt: set atomic as false when calling crypt_convert() in kworker

[Hou Tao]

From: Hou Tao <houtao1@huawei.com>

Both kcryptd_crypt_write_continue() and kcryptd_crypt_read_continue()
are running in the kworker context, it is OK to call cond_resched(),
Therefore, set atomic as false when invoking crypt_convert() under
kworker context.

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 drivers/md/dm-crypt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 1ae2c71bb383b..424c8fc3f0ada 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -2098,7 +2098,7 @@ static void kcryptd_crypt_write_continue(struct work_struct *work)
 	wait_for_completion(&ctx->restart);
 	reinit_completion(&ctx->restart);
 
-	r = crypt_convert(cc, &io->ctx, true, false);
+	r = crypt_convert(cc, &io->ctx, false, false);
 	if (r)
 		io->error = r;
 	crypt_finished = atomic_dec_and_test(&ctx->cc_pending);
@@ -2203,7 +2203,7 @@ static void kcryptd_crypt_read_continue(struct work_struct *work)
 	wait_for_completion(&io->ctx.restart);
 	reinit_completion(&io->ctx.restart);
 
-	r = crypt_convert(cc, &io->ctx, true, false);
+	r = crypt_convert(cc, &io->ctx, false, false);
 	if (r)
 		io->error = r;
 
-- 
2.29.2

[PATCH 2/6] dm-crypt: fully initialize clone->bi_iter in crypt_alloc_buffer()

[Hou Tao]

From: Hou Tao <houtao1@huawei.com>

Both kcryptd_io_read() and kcryptd_crypt_write_convert() will invoke
crypt_alloc_buffer() to allocate a new bio. Both of these two callers
initialize bi_iter.bi_sector for the new bio separatedly after
crypt_alloc_buffer() returns. However, kcryptd_crypt_write_convert()
will copy the bi_iter of the new bio into ctx.iter_out or ctx.iter_in.
Although it doesn't incur any harm now, it is better to fully initialize
bi_iter before it is used.

Therefore, initialize bi_iter.bi_sector in crypt_alloc_buffer() instead.

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 drivers/md/dm-crypt.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 424c8fc3f0ada..551c934bfc501 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -1719,6 +1719,7 @@ static struct bio *crypt_alloc_buffer(struct dm_crypt_io *io, unsigned int size)
 	clone->bi_private = io;
 	clone->bi_end_io = crypt_endio;
 	clone->bi_ioprio = io->base_bio->bi_ioprio;
+	clone->bi_iter.bi_sector = cc->start + io->sector;
 
 	remaining_size = size;
 
@@ -1909,7 +1910,6 @@ static int kcryptd_io_read(struct dm_crypt_io *io, gfp_t gfp)
 			crypt_dec_pending(io);
 			return 1;
 		}
-		clone->bi_iter.bi_sector = cc->start + io->sector;
 		crypt_convert_init(cc, &io->ctx, clone, clone, io->sector);
 		io->saved_bi_iter = clone->bi_iter;
 		dm_submit_bio_remap(io->base_bio, clone);
@@ -1925,13 +1925,13 @@ static int kcryptd_io_read(struct dm_crypt_io *io, gfp_t gfp)
 	clone = bio_alloc_clone(cc->dev->bdev, io->base_bio, gfp, &cc->bs);
 	if (!clone)
 		return 1;
+
+	clone->bi_iter.bi_sector = cc->start + io->sector;
 	clone->bi_private = io;
 	clone->bi_end_io = crypt_endio;
 
 	crypt_inc_pending(io);
 
-	clone->bi_iter.bi_sector = cc->start + io->sector;
-
 	if (dm_crypt_integrity_io_alloc(io, clone)) {
 		crypt_dec_pending(io);
 		bio_put(clone);
@@ -2039,8 +2039,6 @@ static void kcryptd_crypt_write_io_submit(struct dm_crypt_io *io, int async)
 	/* crypt_convert should have filled the clone bio */
 	BUG_ON(io->ctx.iter_out.bi_size);
 
-	clone->bi_iter.bi_sector = cc->start + io->sector;
-
 	if ((likely(!async) && test_bit(DM_CRYPT_NO_OFFLOAD, &cc->flags)) ||
 	    test_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags)) {
 		dm_submit_bio_remap(io->base_bio, clone);
-- 
2.29.2

[PATCH 3/6] dm-crypt: use bi_sector in bio when initialize integrity seed

[Hou Tao]

From: Hou Tao <houtao1@huawei.com>

bio->bi_iter.bi_sector has already been initialized when initialize the
integrity seed in dm_crypt_integrity_io_alloc(). There is no need to
calculate it again. Therefore, use the helper bip_set_seed() to
initialize the seed and pass bi_iter.bi_sector to it instead.

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 drivers/md/dm-crypt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 551c934bfc501..4634e2d850f21 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -1187,7 +1187,7 @@ static int dm_crypt_integrity_io_alloc(struct dm_crypt_io *io, struct bio *bio)
 
 	tag_len = io->cc->tuple_size * (bio_sectors(bio) >> io->cc->sector_shift);
 
-	bip->bip_iter.bi_sector = io->cc->start + io->sector;
+	bip_set_seed(bip, bio->bi_iter.bi_sector);
 
 	ret = bio_integrity_add_page(bio, virt_to_page(io->integrity_metadata),
 				     tag_len, offset_in_page(io->integrity_metadata));
-- 
2.29.2

[PATCH 4/6] dm-crypt: don't update io->sector after kcryptd_crypt_write_io_submit()

[Hou Tao]

From: Hou Tao <houtao1@huawei.com>

The updates of io->sector are the leftovers when dm-crypt allocated
pages for partial write request. However, since commit cf2f1abfbd0db
("dm crypt: don't allocate pages for a partial request"), there is no
partial request anymore.

After the introduction of write request rb-tree, the updates of
io->sectors may interfere the insertion procedure, because ->sectors of
these write requests which have already been added in the rb-tree may be
changed during the insertion of new write request.

Fix it by removing these buggy updates of io->sectors. Considering these
updates only effect the write request rb-tree, the commit which
introduces the write request rb-tree is used as the fix tag.

Fixes: b3c5fd305249 ("dm crypt: sort writes")
Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 drivers/md/dm-crypt.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 4634e2d850f21..8cee6b660e90d 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -2090,7 +2090,6 @@ static void kcryptd_crypt_write_continue(struct work_struct *work)
 	struct crypt_config *cc = io->cc;
 	struct convert_context *ctx = &io->ctx;
 	int crypt_finished;
-	sector_t sector = io->sector;
 	blk_status_t r;
 
 	wait_for_completion(&ctx->restart);
@@ -2107,10 +2106,8 @@ static void kcryptd_crypt_write_continue(struct work_struct *work)
 	}
 
 	/* Encryption was already finished, submit io now */
-	if (crypt_finished) {
+	if (crypt_finished)
 		kcryptd_crypt_write_io_submit(io, 0);
-		io->sector = sector;
-	}
 
 	crypt_dec_pending(io);
 }
@@ -2121,14 +2118,13 @@ static void kcryptd_crypt_write_convert(struct dm_crypt_io *io)
 	struct convert_context *ctx = &io->ctx;
 	struct bio *clone;
 	int crypt_finished;
-	sector_t sector = io->sector;
 	blk_status_t r;
 
 	/*
 	 * Prevent io from disappearing until this function completes.
 	 */
 	crypt_inc_pending(io);
-	crypt_convert_init(cc, ctx, NULL, io->base_bio, sector);
+	crypt_convert_init(cc, ctx, NULL, io->base_bio, io->sector);
 
 	clone = crypt_alloc_buffer(io, io->base_bio->bi_iter.bi_size);
 	if (unlikely(!clone)) {
@@ -2145,8 +2141,6 @@ static void kcryptd_crypt_write_convert(struct dm_crypt_io *io)
 		io->ctx.iter_in = clone->bi_iter;
 	}
 
-	sector += bio_sectors(clone);
-
 	crypt_inc_pending(io);
 	r = crypt_convert(cc, ctx,
 			  test_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags), true);
@@ -2170,10 +2164,8 @@ static void kcryptd_crypt_write_convert(struct dm_crypt_io *io)
 	}
 
 	/* Encryption was already finished, submit io now */
-	if (crypt_finished) {
+	if (crypt_finished)
 		kcryptd_crypt_write_io_submit(io, 0);
-		io->sector = sector;
-	}
 
 dec:
 	crypt_dec_pending(io);
-- 
2.29.2

[PATCH 5/6] dm-crypt: don't initialize cc_sector again

[Hou Tao]

From: Hou Tao <houtao1@huawei.com>

For aead_recheck case, cc_sector has already been initialized in
crypt_convert_init() when trying to re-read the read. Therefore, remove
the duplicated initialization.

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 drivers/md/dm-crypt.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 8cee6b660e90d..8b15f57af36a9 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -2211,7 +2211,6 @@ static void kcryptd_crypt_read_convert(struct dm_crypt_io *io)
 	crypt_inc_pending(io);
 
 	if (io->ctx.aead_recheck) {
-		io->ctx.cc_sector = io->sector + cc->iv_offset;
 		r = crypt_convert(cc, &io->ctx,
 				  test_bit(DM_CRYPT_NO_READ_WORKQUEUE, &cc->flags), true);
 	} else {
-- 
2.29.2

[PATCH 6/6] dm-crypt: track tag_offset in convert_context

[Hou Tao]

From: Hou Tao <houtao1@huawei.com>

dm-crypt uses tag_offset to index the integrity metadata for each crypt
sector. When the initial crypt_convert() returns BLK_STS_DEV_RESOURCE,
dm-crypt will try to continue the crypt/decrypt procedure in a kworker.
However, it resets tag_offset as zero instead of using the tag_offset
related with current sector. It may return unexpected data when using
random IV or return unexpected integrity related error.

Fix the problem by tracking tag_offset in per-IO convert_context.
Therefore, when the crypt/decrypt procedure continues in a kworker, it
could use the next tag_offset saved in convert_context.

Fixes: 8abec36d1274 ("dm crypt: do not wait for backlogged crypto request completion in softirq")
Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 drivers/md/dm-crypt.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 8b15f57af36a9..f6973fd2925b3 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -59,6 +59,7 @@ struct convert_context {
 	struct bio *bio_out;
 	struct bvec_iter iter_out;
 	atomic_t cc_pending;
+	unsigned int tag_offset;
 	u64 cc_sector;
 	union {
 		struct skcipher_request *req;
@@ -1256,6 +1257,7 @@ static void crypt_convert_init(struct crypt_config *cc,
 	if (bio_out)
 		ctx->iter_out = bio_out->bi_iter;
 	ctx->cc_sector = sector + cc->iv_offset;
+	ctx->tag_offset = 0;
 	init_completion(&ctx->restart);
 }
 
@@ -1588,7 +1590,6 @@ static void crypt_free_req(struct crypt_config *cc, void *req, struct bio *base_
 static blk_status_t crypt_convert(struct crypt_config *cc,
 			 struct convert_context *ctx, bool atomic, bool reset_pending)
 {
-	unsigned int tag_offset = 0;
 	unsigned int sector_step = cc->sector_size >> SECTOR_SHIFT;
 	int r;
 
@@ -1611,9 +1612,9 @@ static blk_status_t crypt_convert(struct crypt_config *cc,
 		atomic_inc(&ctx->cc_pending);
 
 		if (crypt_integrity_aead(cc))
-			r = crypt_convert_block_aead(cc, ctx, ctx->r.req_aead, tag_offset);
+			r = crypt_convert_block_aead(cc, ctx, ctx->r.req_aead, ctx->tag_offset);
 		else
-			r = crypt_convert_block_skcipher(cc, ctx, ctx->r.req, tag_offset);
+			r = crypt_convert_block_skcipher(cc, ctx, ctx->r.req, ctx->tag_offset);
 
 		switch (r) {
 		/*
@@ -1633,8 +1634,8 @@ static blk_status_t crypt_convert(struct crypt_config *cc,
 					 * exit and continue processing in a workqueue
 					 */
 					ctx->r.req = NULL;
+					ctx->tag_offset++;
 					ctx->cc_sector += sector_step;
-					tag_offset++;
 					return BLK_STS_DEV_RESOURCE;
 				}
 			} else {
@@ -1648,8 +1649,8 @@ static blk_status_t crypt_convert(struct crypt_config *cc,
 		 */
 		case -EINPROGRESS:
 			ctx->r.req = NULL;
+			ctx->tag_offset++;
 			ctx->cc_sector += sector_step;
-			tag_offset++;
 			continue;
 		/*
 		 * The request was already processed (synchronously).
@@ -1657,7 +1658,7 @@ static blk_status_t crypt_convert(struct crypt_config *cc,
 		case 0:
 			atomic_dec(&ctx->cc_pending);
 			ctx->cc_sector += sector_step;
-			tag_offset++;
+			ctx->tag_offset++;
 			if (!atomic)
 				cond_resched();
 			continue;
-- 
2.29.2

Re: [PATCH 1/6] dm-crypt: set atomic as false when calling crypt_convert() in kworker

[Ignat Korchagin]

Hi,

On Mon, Jan 20, 2025 at 8:18 AM Hou Tao <houtao@huaweicloud.com> wrote:
>
> From: Hou Tao <houtao1@huawei.com>
>
> Both kcryptd_crypt_write_continue() and kcryptd_crypt_read_continue()
> are running in the kworker context, it is OK to call cond_resched(),

yes, indeed

> Therefore, set atomic as false when invoking crypt_convert() under
> kworker context.
>
> Signed-off-by: Hou Tao <houtao1@huawei.com>

Maybe add Fixes: 8abec36d1274 ("dm crypt: do not wait for backlogged
crypto request completion in softirq")?

Reviewed-by: Ignat Korchagin <ignat@cloudflare.com>

> ---
>  drivers/md/dm-crypt.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index 1ae2c71bb383b..424c8fc3f0ada 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -2098,7 +2098,7 @@ static void kcryptd_crypt_write_continue(struct work_struct *work)
>         wait_for_completion(&ctx->restart);
>         reinit_completion(&ctx->restart);
>
> -       r = crypt_convert(cc, &io->ctx, true, false);
> +       r = crypt_convert(cc, &io->ctx, false, false);
>         if (r)
>                 io->error = r;
>         crypt_finished = atomic_dec_and_test(&ctx->cc_pending);
> @@ -2203,7 +2203,7 @@ static void kcryptd_crypt_read_continue(struct work_struct *work)
>         wait_for_completion(&io->ctx.restart);
>         reinit_completion(&io->ctx.restart);
>
> -       r = crypt_convert(cc, &io->ctx, true, false);
> +       r = crypt_convert(cc, &io->ctx, false, false);
>         if (r)
>                 io->error = r;
>
> --
> 2.29.2
>

Re: [PATCH 0/6] dm-crypt: misc cleanups and fixes

[Mikulas Patocka]

On Mon, 20 Jan 2025, Hou Tao wrote:

> From: Hou Tao <houtao1@huawei.com>
> 
> Hi,
> 
> The patchset contains misc cleanups and minor fixes for dm-crypt. These
> problem were spotted during the code inspection. The patchset have been
> tested by running fio on dm-crypt devices (both with and without
> integrity feature). Patch #6 is tested by asynchronizing the
> crypt/decrypt algorithm when using random IV.
> 
> Please see individual patches for more details. Comments are always
> welcome.
> 
> Hou Tao (6):
>   dm-crypt: set atomic as false when calling crypt_convert() in kworker
>   dm-crypt: fully initialize clone->bi_iter in crypt_alloc_buffer()
>   dm-crypt: use bi_sector in bio when initialize integrity seed
>   dm-crypt: don't update io->sector after
>     kcryptd_crypt_write_io_submit()
>   dm-crypt: don't initialize cc_sector again
>   dm-crypt: track tag_offset in convert_context
> 
>  drivers/md/dm-crypt.c | 42 ++++++++++++++++--------------------------
>  1 file changed, 16 insertions(+), 26 deletions(-)
> 
> -- 
> 2.29.2

I applied the patches. Thanks.

Mikulas