[01/12] dm-integrity: use the nop integrity profile

Commit Message

Christoph Hellwig June 5, 2024, 6:28 a.m. UTC

Use the block layer built-in nop profile instead of reinventing it.

Tested by:

$ dd if=/dev/urandom of=key.bin bs=512 count=1

$ cryptsetup luksFormat -q --type luks2 --integrity hmac-sha256 \
 	--integrity-no-wipe /dev/nvme0n1 key.bin
$ cryptsetup luksOpen /dev/nvme0n1 luks-integrity --key-file key.bin

and then doing mkfs.xfs and simple I/O on the mount file system.

Signed-off-by: Christoph Hellwig <[e-mail]>
---
 drivers/md/dm-crypt.c     |  4 ++--
 drivers/md/dm-integrity.c | 20 --------------------
 2 files changed, 2 insertions(+), 22 deletions(-)

Comments

Milan Broz June 5, 2024, 2:52 p.m. UTC | #1

On 6/5/24 8:28 AM, Christoph Hellwig wrote:
> Use the block layer built-in nop profile instead of reinventing it.

As this is my "invention", I am pretty sure that "nop" profile was
not available at the time I was prototyping AEAD dmcrypt extension.
(It was months before we submitted it upstream - and then nobody
apparently fixed it.)

So, I am quite happy this hack can go away!

I run full cryptsetup testuite with this patch and everything
works, nice cleanup.

Reviewed-by: Milan Broz <[e-mail]>

Thanks,
Milan

> 
> Tested by:
> 
> $ dd if=/dev/urandom of=key.bin bs=512 count=1
> 
> $ cryptsetup luksFormat -q --type luks2 --integrity hmac-sha256 \
>   	--integrity-no-wipe /dev/nvme0n1 key.bin
> $ cryptsetup luksOpen /dev/nvme0n1 luks-integrity --key-file key.bin
> 
> and then doing mkfs.xfs and simple I/O on the mount file system.
> 
> Signed-off-by: Christoph Hellwig <[e-mail]>
> ---
>   drivers/md/dm-crypt.c     |  4 ++--
>   drivers/md/dm-integrity.c | 20 --------------------
>   2 files changed, 2 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index 1b7a97cc377943..1dfc462f29cd6f 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -1176,8 +1176,8 @@ static int crypt_integrity_ctr(struct crypt_config *cc, struct dm_target *ti)
>   	struct blk_integrity *bi = blk_get_integrity(cc->dev->bdev->bd_disk);
>   	struct mapped_device *md = dm_table_get_md(ti->table);
>   
> -	/* From now we require underlying device with our integrity profile */
> -	if (!bi || strcasecmp(bi->profile->name, "DM-DIF-EXT-TAG")) {
> +	/* We require an underlying device with non-PI metadata */
> +	if (!bi || strcmp(bi->profile->name, "nop")) {
>   		ti->error = "Integrity profile not supported.";
>   		return -EINVAL;
>   	}
> diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
> index 417fddebe367a2..c1cc27541673c7 100644
> --- a/drivers/md/dm-integrity.c
> +++ b/drivers/md/dm-integrity.c
> @@ -350,25 +350,6 @@ static struct kmem_cache *journal_io_cache;
>   #define DEBUG_bytes(bytes, len, msg, ...)	do { } while (0)
>   #endif
>   
> -static void dm_integrity_prepare(struct request *rq)
> -{
> -}
> -
> -static void dm_integrity_complete(struct request *rq, unsigned int nr_bytes)
> -{
> -}
> -
> -/*
> - * DM Integrity profile, protection is performed layer above (dm-crypt)
> - */
> -static const struct blk_integrity_profile dm_integrity_profile = {
> -	.name			= "DM-DIF-EXT-TAG",
> -	.generate_fn		= NULL,
> -	.verify_fn		= NULL,
> -	.prepare_fn		= dm_integrity_prepare,
> -	.complete_fn		= dm_integrity_complete,
> -};
> -
>   static void dm_integrity_map_continue(struct dm_integrity_io *dio, bool from_map);
>   static void integrity_bio_wait(struct work_struct *w);
>   static void dm_integrity_dtr(struct dm_target *ti);
> @@ -3656,7 +3637,6 @@ static void dm_integrity_set(struct dm_target *ti, struct dm_integrity_c *ic)
>   	struct blk_integrity bi;
>   
>   	memset(&bi, 0, sizeof(bi));
> -	bi.profile = &dm_integrity_profile;
>   	bi.tuple_size = ic->tag_size;
>   	bi.tag_size = bi.tuple_size;
>   	bi.interval_exp = ic->sb->log2_sectors_per_block + SECTOR_SHIFT;

Chaitanya Kulkarni June 6, 2024, 12:12 a.m. UTC | #2

On 6/4/2024 11:28 PM, Christoph Hellwig wrote:
> Use the block layer built-in nop profile instead of reinventing it.
> 
> Tested by:
> 
> $ dd if=/dev/urandom of=key.bin bs=512 count=1
> 
> $ cryptsetup luksFormat -q --type luks2 --integrity hmac-sha256 \
>   	--integrity-no-wipe /dev/nvme0n1 key.bin
> $ cryptsetup luksOpen /dev/nvme0n1 luks-integrity --key-file key.bin
> 
> and then doing mkfs.xfs and simple I/O on the mount file system.
> 
> Signed-off-by: Christoph Hellwig<[e-mail]>

Looks good.

Reviewed-by: Chaitanya Kulkarni <[e-mail]>

-ck

Christoph Hellwig June 6, 2024, 4:45 a.m. UTC | #3

On Wed, Jun 05, 2024 at 04:52:27PM +0200, Milan Broz wrote:
> On 6/5/24 8:28 AM, Christoph Hellwig wrote:
>> Use the block layer built-in nop profile instead of reinventing it.
>
> As this is my "invention", I am pretty sure that "nop" profile was
> not available at the time I was prototyping AEAD dmcrypt extension.
> (It was months before we submitted it upstream - and then nobody
> apparently fixed it.)

Looking at the history the nop profile was moved from nvme to common
code to also support btt in 2015, dm-integrity was added in 2017.
So maybe you just missed it.  Anyway, I'm also happy to tone this
down a bit, it sounds a bit too aggressive..

Patch

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 1b7a97cc377943..1dfc462f29cd6f 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -1176,8 +1176,8 @@  static int crypt_integrity_ctr(struct crypt_config *cc, struct dm_target *ti)
 	struct blk_integrity *bi = blk_get_integrity(cc->dev->bdev->bd_disk);
 	struct mapped_device *md = dm_table_get_md(ti->table);
 
-	/* From now we require underlying device with our integrity profile */
-	if (!bi || strcasecmp(bi->profile->name, "DM-DIF-EXT-TAG")) {
+	/* We require an underlying device with non-PI metadata */
+	if (!bi || strcmp(bi->profile->name, "nop")) {
 		ti->error = "Integrity profile not supported.";
 		return -EINVAL;
 	}
diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
index 417fddebe367a2..c1cc27541673c7 100644
--- a/drivers/md/dm-integrity.c
+++ b/drivers/md/dm-integrity.c
@@ -350,25 +350,6 @@  static struct kmem_cache *journal_io_cache;
 #define DEBUG_bytes(bytes, len, msg, ...)	do { } while (0)
 #endif
 
-static void dm_integrity_prepare(struct request *rq)
-{
-}
-
-static void dm_integrity_complete(struct request *rq, unsigned int nr_bytes)
-{
-}
-
-/*
- * DM Integrity profile, protection is performed layer above (dm-crypt)
- */
-static const struct blk_integrity_profile dm_integrity_profile = {
-	.name			= "DM-DIF-EXT-TAG",
-	.generate_fn		= NULL,
-	.verify_fn		= NULL,
-	.prepare_fn		= dm_integrity_prepare,
-	.complete_fn		= dm_integrity_complete,
-};
-
 static void dm_integrity_map_continue(struct dm_integrity_io *dio, bool from_map);
 static void integrity_bio_wait(struct work_struct *w);
 static void dm_integrity_dtr(struct dm_target *ti);
@@ -3656,7 +3637,6 @@  static void dm_integrity_set(struct dm_target *ti, struct dm_integrity_c *ic)
 	struct blk_integrity bi;
 
 	memset(&bi, 0, sizeof(bi));
-	bi.profile = &dm_integrity_profile;
 	bi.tuple_size = ic->tag_size;
 	bi.tag_size = bi.tuple_size;
 	bi.interval_exp = ic->sb->log2_sectors_per_block + SECTOR_SHIFT;