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

[Linlin Zhang <linlin.zhang@oss.qualcomm.com>]

On 4/29/2026 12:20 AM, Benjamin Marzinski wrote:
> On Tue, Apr 28, 2026 at 06:43:08PM +0800, Linlin Zhang wrote:
>> Correct the response to Benjamin's comments.
>>
>> On 4/27/2026 8:20 PM, Linlin Zhang wrote:
>>>
>>>
>>> On 4/27/2026 9:19 AM, Benjamin Marzinski wrote:
>>>> On Fri, Apr 10, 2026 at 06:40:30AM -0700, Linlin Zhang wrote:
>>>>> From: Eric Biggers <ebiggers@google.com>
>>>>> +
>>>>> +static int inlinecrypt_map(struct dm_target *ti, struct bio *bio)
>>>>> +{
>>>>> +	const struct inlinecrypt_ctx *ctx = ti->private;
>>>>> +	sector_t sector_in_target;
>>>>> +	u64 dun[BLK_CRYPTO_DUN_ARRAY_SIZE] = {};
>>>>> +
>>>>> +	bio_set_dev(bio, ctx->dev->bdev);
>>>>> +
>>>>> +	/*
>>>>> +	 * If the bio is a device-level request which doesn't target a specific
>>>>> +	 * sector, there's nothing more to do.
>>>>> +	 */
>>>>> +	if (bio_sectors(bio) == 0)
>>>>> +		return DM_MAPIO_REMAPPED;
>>>>> +
>>>>> +	/*
>>>>> +	 * The bio should never have an encryption context already, since
>>>>> +	 * dm-inlinecrypt doesn't pass through any inline encryption
>>>>> +	 * capabilities to the layer above it.
>>>>> +	 */
>>>>> +	if (WARN_ON_ONCE(bio_has_crypt_ctx(bio)))
>>>>> +		return DM_MAPIO_KILL;
>>>>> +
>>>>> +	/* 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.
>>>>> +	 */
>>>>> +	if (!bio_has_data(bio))
>>>>> +		return DM_MAPIO_REMAPPED;
>>>>> +
>>>>> +	/* Calculate the DUN and enforce data-unit (crypto sector) alignment. */
>>>>> +	dun[0] = ctx->iv_offset + sector_in_target; /* 512-byte sectors */
>>>>> +	if (dun[0] & ((ctx->sector_size >> SECTOR_SHIFT) - 1))
>>>>> +		return DM_MAPIO_KILL;
>>>>
>>>> If ctx->iv_offset is not a multiple of ctx->sector_size, this will
>>>> always fail. ctx->iv_offset should probably get validated in
>>>> inlinecrypt_ctr()
>>>
>>> ACK
>>>
>>> Yes, this assumes iv_offset is aligned to sector_size when large crypto
>>> sectors are used. That’s a requirement of dm-inlinecrypt semantics, and
>>> adding an explicit check in inlinecrypt_ctr() would make this fail earlier
>>> and more clearly.
>>
>> Sorry, the last response is wrong. No need to add check in inlinecrypt_ctr().
>>
>> iv_offset is the starting offset for IVs that are generated as if the target were
>> preceded by iv_offset 512-byte sectors.
>>
>> I think this concern is based on an implicit assumption that
>> sector_in_target is always data-unit (crypto sector) aligned. In this
>> target, however, sector_in_target is derived from dm_target_offset() and
>> is in 512-byte sectors, so it is not guaranteed to be a multiple of
>> (sector_size >> SECTOR_SHIFT).
> 
> sector_in_target should be guaranteed to be sector_size aligned.
> inlinecrypt_io_hints() sets the device logical block size to at least
> ctx->sector_size, and validate_hardware_logical_block_alignment() makes
> sure that the target starts on a logical block boundary. The block
> layer enforces IO to be aligned with the logical block size, so
> an IO that starts 7 sectors into a device with a 4096 ctx->sector_size
> should be impossible. 

Thanks for the detailed clarification!

In the next patch, I will add an explicit alignment check in ctr() to ensure that
iv_offset is aligned to the configured sector size. For example:

	if (ctx->iv_offset & ((ctx->sector_size >> SECTOR_SHIFT) - 1)) {
		ti->error = "Wrong alignment of iv_offset sector";
		err = -EINVAL;
	}

Please let me know if you would like this validation to be handled differently,
or if additional checks are needed.

> 
> -Ben
>