Re: [PATCH v1 2/3] dm-inlinecrypt: add target for inline block device encryption

[Milan Broz <gmazyland@gmail.com>]

Hi,

just few comments below, but I am not DM maintainer so feel free to ignore it :)

On 3/4/26 1:17 PM, Linlin Zhang wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Add a new device-mapper target "dm-inlinecrypt" that is similar to
> dm-crypt but uses the blk-crypto API instead of the regular crypto API.
> This allows it to take advantage of inline encryption hardware such as
> that commonly built into UFS host controllers.
> 
> The table syntax matches dm-crypt's, but for now only a stripped-down
> set of parameters is supported.  For example, for now AES-256-XTS is the
> only supported cipher.
> 
> dm-inlinecrypt is based on Android's dm-default-key with the
> controversial passthrough support removed.  Note that due to the removal
> of passthrough support, use of dm-inlinecrypt in combination with
> fscrypt causes double encryption of file contents (similar to dm-crypt +
> fscrypt), with the fscrypt layer not being able to use the inline
> encryption hardware.  This makes dm-inlinecrypt unusable on systems such
> as Android that use fscrypt and where a more optimized approach is
> needed.  It is however suitable as a replacement for dm-crypt.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> Signed-off-by: Linlin Zhang <linlin.zhang@oss.qualcomm.com>
> ---
>   drivers/md/Kconfig          |  10 +
>   drivers/md/Makefile         |   1 +
>   drivers/md/dm-inlinecrypt.c | 416 ++++++++++++++++++++++++++++++++++++

I think it should also add doc in
Documentation/admin-guide/device-mapper/dm-inlinecrypt.rst
...

> +#define DM_MSG_PREFIX	"inlinecrypt"
> +
> +static const struct dm_inlinecrypt_cipher {
> +	const char *name;
> +	enum blk_crypto_mode_num mode_num;
> +	int key_size;
> +} dm_inlinecrypt_ciphers[] = {
> +	{
> +		.name = "aes-xts-plain64",
> +		.mode_num = BLK_ENCRYPTION_MODE_AES_256_XTS,
> +		.key_size = 64,

Hm. I can understand some translation table for this stupid
dm-crypt notation to inline enum, but why you need key size here?

Shouldn't there be some helper for inline crypt returning
keysize based on BLK_ENCRYPTION_MODE_AES_256_XTS?

I guess you have fixed cipher list already, but what about IV?
Is it always little-endian, or someone already reinvented plain64be (big-endian)?

...> +	while (opt_params--) {

...> +/*
> + * Construct an inlinecrypt mapping:
> + * <cipher> <key> <iv_offset> <dev_path> <start>

As above, it supports opt params, it should mention it here (or in doc).


...
> +	/* <key> */
> +	if (strlen(argv[1]) != 2 * cipher->key_size) {
> +		ti->error = "Incorrect key size for cipher";
> +		err = -EINVAL;
> +		goto bad;
> +	}
> +	if (hex2bin(raw_key, argv[1], cipher->key_size) != 0) {
> +		ti->error = "Malformed key string";
> +		err = -EINVAL;
> +		goto bad;
> +	}


Any reason it does not support keyring keys from the beginning?

...
> +static int inlinecrypt_map(struct dm_target *ti, struct bio *bio)

> +	/* Map the bio's sector to the underlying device. (512-byte sectors) */
> +	sector_in_target = dm_target_offset(ti, bio->bi_iter.bi_sector);
> +	bio->bi_iter.bi_sector = ctx->start + sector_in_target;
> +	/*
> +	 * If the bio doesn't have any data (e.g. if it's a DISCARD request),
> +	 * there's nothing more to do.
> +	 */

dmcrypt uses bio_set_dev() for REQ_PREFLUSH or REQ_OP_DISCARD, why this differs?

> +
> +	switch (type) {
> +	case STATUSTYPE_INFO:
> +	case STATUSTYPE_IMA:
> +		result[0] = '\0';

This should really emit audit information similar to dm-crypt.

> +		break;
> +
> +	case STATUSTYPE_TABLE:
> +		/*
> +		 * Warning: like dm-crypt, dm-inlinecrypt includes the key in
> +		 * the returned table.  Userspace is responsible for redacting
> +		 * the key when needed.

Again, why not support keyring format? LUKS2 uses it by default for dm-crypt table.

Milan