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

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

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

The intended alignment requirement is on the *final* DUN, i.e. on
(iv_offset + sector_in_target) in 512-byte sector units, before it gets
shifted down to crypto-sector units. That's why the code checks alignment
on the sum (iv_offset + sector_in_target).

With this definition, iv_offset itself does not need to be aligned to the
data-unit size; any non-negative value is valid as long as the resulting
DUN for a given bio is data-unit aligned. For example, iv_offset = 1 can
still be valid when sector_in_target is 7 (4096-byte sector case), since
their sum is aligned. Validating iv_offset alone in inlinecrypt_ctr()
would therefore reject configurations that are otherwise correct per the
(sum-based) DUN definition.

So I believe the runtime check in ->map() is the right place to enforce
the data-unit alignment constraint, as it has the actual bio offset
(sector_in_target) needed to evaluate the constraint.

Let me know if you'd prefer we document this more explicitly in the map()
argument description; I'm fine adding a short note about the
iv_offset units and the sum-based alignment rule.

> 
>>
>> -Ben
>>
>>> +	dun[0] >>= ctx->sector_bits - SECTOR_SHIFT; /* crypto sectors */
>>> +
>>> +	/*
>>> +	 * This check isn't necessary as we should have calculated max_dun
>>> +	 * correctly, but be safe.
>>> +	 */
>>> +	if (WARN_ON_ONCE(dun[0] > ctx->max_dun))
>>> +		return DM_MAPIO_KILL;
>>> +
>>> +	bio_crypt_set_ctx(bio, &ctx->key, dun, GFP_NOIO);
>>> +
>>> +	/*
>>> +	 * Since we've added an encryption context to the bio and
>>> +	 * blk-crypto-fallback may be needed to process it, it's necessary to
>>> +	 * use the fallback-aware bio submission code rather than
>>> +	 * unconditionally returning DM_MAPIO_REMAPPED.
>>> +	 *
>>> +	 * To get the correct accounting for a dm target in the case where
>>> +	 * __blk_crypto_submit_bio() doesn't take ownership of the bio (returns
>>> +	 * true), call __blk_crypto_submit_bio() directly and return
>>> +	 * DM_MAPIO_REMAPPED in that case, rather than relying on
>>> +	 * blk_crypto_submit_bio() which calls submit_bio() in that case.
>>> +	 */
>>> +	if (__blk_crypto_submit_bio(bio))
>>> +		return DM_MAPIO_REMAPPED;
>>> +	return DM_MAPIO_SUBMITTED;
>>> +}
>>> +
<snip> 
>>> +MODULE_AUTHOR("Eric Biggers <ebiggers@google.com>");
>>> +MODULE_AUTHOR("Linlin Zhang <linlin.zhang@oss.qualcomm.com>");
>>> +MODULE_DESCRIPTION(DM_NAME " target for inline encryption");
>>> +MODULE_LICENSE("GPL");
>>> -- 
>>> 2.34.1
>>>
>>
>