Re: [PATCH v5 2/9] block: Add encryption context to struct bio

[Christoph Hellwig <hch@infradead.org>]

> +static int num_prealloc_crypt_ctxs = 128;

Where does that magic number come from?

> +struct bio_crypt_ctx *bio_crypt_alloc_ctx(gfp_t gfp_mask)
> +{
> +	return mempool_alloc(bio_crypt_ctx_pool, gfp_mask);
> +}
> +EXPORT_SYMBOL(bio_crypt_alloc_ctx);

This isn't used by an modular code.

> +void bio_crypt_free_ctx(struct bio *bio)
> +{
> +	mempool_free(bio->bi_crypt_context, bio_crypt_ctx_pool);
> +	bio->bi_crypt_context = NULL;
> +}
> +EXPORT_SYMBOL(bio_crypt_free_ctx);

This one is called from modular code, but I think the usage in DM
is bogus, as the caller of the function eventually does a bio_put,
which ends up in bio_free and takes care of the freeing as well.

> +bool bio_crypt_should_process(struct bio *bio, struct request_queue *q)
> +{
> +	if (!bio_has_crypt_ctx(bio))
> +		return false;
> +
> +	WARN_ON(!bio_crypt_has_keyslot(bio));
> +	return q->ksm == bio->bi_crypt_context->processing_ksm;
> +}

Passing a struct request here and also adding the ->bio != NULL check
here would simplify the only caller in ufs a bit.

> +/*
> + * Checks that two bio crypt contexts are compatible - i.e. that
> + * they are mergeable except for data_unit_num continuity.
> + */
> +bool bio_crypt_ctx_compatible(struct bio *b_1, struct bio *b_2)
> +{
> +	struct bio_crypt_ctx *bc1 = b_1->bi_crypt_context;
> +	struct bio_crypt_ctx *bc2 = b_2->bi_crypt_context;
> +
> +	if (bio_has_crypt_ctx(b_1) != bio_has_crypt_ctx(b_2))
> +		return false;
> +
> +	if (!bio_has_crypt_ctx(b_1))
> +		return true;
> +
> +	return bc1->keyslot == bc2->keyslot &&
> +	       bc1->data_unit_size_bits == bc2->data_unit_size_bits;
> +}

I think we'd normally call this bio_crypt_ctx_mergeable.

> +	if (bio_crypt_clone(b, bio, gfp_mask) < 0) {
> +		bio_put(b);
> +		return NULL;
> +	}
>  
> -		if (ret < 0) {
> -			bio_put(b);
> -			return NULL;
> -		}
> +	if (bio_integrity(bio) &&
> +	    bio_integrity_clone(b, bio, gfp_mask) < 0) {
> +		bio_put(b);
> +		return NULL;

Pleae use a goto to merge the common error handling path

> +		if (!bio_crypt_ctx_back_mergeable(req->bio,
> +						  blk_rq_sectors(req),
> +						  next->bio)) {
> +			return ELEVATOR_NO_MERGE;
> +		}

No neef for the braces.  And pretty weird alignment, normal Linux style
would be:

		if (!bio_crypt_ctx_back_mergeable(req->bio,
				blk_rq_sectors(req), next->bio))
			return ELEVATOR_NO_MERGE;

> +		if (!bio_crypt_ctx_back_mergeable(rq->bio,
> +						  blk_rq_sectors(rq), bio)) {
> +			return ELEVATOR_NO_MERGE;
> +		}
>  		return ELEVATOR_BACK_MERGE;
> -	else if (blk_rq_pos(rq) - bio_sectors(bio) == bio->bi_iter.bi_sector)
> +	} else if (blk_rq_pos(rq) - bio_sectors(bio) ==
> +		   bio->bi_iter.bi_sector) {
> +		if (!bio_crypt_ctx_back_mergeable(bio,
> +						  bio_sectors(bio), rq->bio)) {
> +			return ELEVATOR_NO_MERGE;
> +		}

Same for these two.

> +++ b/block/bounce.c
> @@ -267,14 +267,15 @@ static struct bio *bounce_clone_bio(struct bio *bio_src, gfp_t gfp_mask,
>  		break;
>  	}
>  
> -	if (bio_integrity(bio_src)) {
> -		int ret;
> +	if (bio_crypt_clone(bio, bio_src, gfp_mask) < 0) {
> +		bio_put(bio);
> +		return NULL;
> +	}
>  
> -		ret = bio_integrity_clone(bio, bio_src, gfp_mask);
> -		if (ret < 0) {
> -			bio_put(bio);
> -			return NULL;
> -		}
> +	if (bio_integrity(bio_src) &&
> +	    bio_integrity_clone(bio, bio_src, gfp_mask) < 0) {
> +		bio_put(bio);
> +		return NULL;

Use a common error path with a goto, please.

> +static inline int bio_crypt_set_ctx(struct bio *bio,
> +				    const u8 *raw_key,
> +				    enum blk_crypto_mode_num crypto_mode,
> +				    u64 dun,
> +				    unsigned int dun_bits,
> +				    gfp_t gfp_mask)

Pleae just open code this in the only caller.

> +{
> +	struct bio_crypt_ctx *crypt_ctx;
> +
> +	crypt_ctx = bio_crypt_alloc_ctx(gfp_mask);
> +	if (!crypt_ctx)
> +		return -ENOMEM;

Also bio_crypt_alloc_ctx with a waiting mask will never return an
error.  Changing this function and its call chain to void returns will
clean up a lot of code in this series.

> +static inline void bio_set_data_unit_num(struct bio *bio, u64 dun)
> +{
> +	bio->bi_crypt_context->data_unit_num = dun;
> +}

This function is never used and can be removed.

> +static inline void bio_crypt_set_keyslot(struct bio *bio,
> +					 unsigned int keyslot,
> +					 struct keyslot_manager *ksm)
> +{
> +	bio->bi_crypt_context->keyslot = keyslot;
> +	bio->bi_crypt_context->processing_ksm = ksm;
> +}

Just adding these two lines to the only caller will be a lot cleaner.

> +static inline const u8 *bio_crypt_raw_key(struct bio *bio)
> +{
> +	return bio->bi_crypt_context->raw_key;
> +}

Can be inlined into the only caller.

> +
> +static inline enum blk_crypto_mode_num bio_crypto_mode(struct bio *bio)
> +{
> +	return bio->bi_crypt_context->crypto_mode;
> +}

Same here.

> +static inline u64 bio_crypt_sw_data_unit_num(struct bio *bio)
> +{
> +	return bio->bi_crypt_context->sw_data_unit_num;
> +}

Same here.