Re: [f2fs-dev] [PATCH v5 2/9] block: Add encryption context to struct bio
> +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;
> +}
[f2fs-dev] [PATCH v5 2/9] block: Add encryption context to struct bio
We must have some way of letting a storage device driver know what
encryption context it should use for en/decrypting a request. However,
it's the filesystem/fscrypt that knows about and manages encryption
contexts. As such, when the filesystem layer submits a bio to the block
layer, and this bio eventually reaches a device driver with support for
inline encryption, the device driver will need to have been told the
encryption context for that bio.
We want to communicate the encryption context from the filesystem layer
to the storage device along with the bio, when the bio is submitted to the
block layer. To do this, we add a struct bio_crypt_ctx to struct bio, which
can represent an encryption context (note that we can't use the bi_private
field in struct bio to do this because that field does not function to pass
information across layers in the storage stack). We also introduce various
functions to manipulate the bio_crypt_ctx and make the bio/request merging
logic aware of the bio_crypt_ctx.
Signed-off-by: Satya Tangirala
---
block/Makefile| 2 +-
block/bio-crypt-ctx.c | 137 +
block/bio.c | 18 +--
block/blk-core.c | 3 +
block/blk-merge.c | 35 +-
block/bounce.c| 15 +--
drivers/md/dm.c | 15 ++-
include/linux/bio-crypt-ctx.h | 219 ++
include/linux/bio.h | 6 +-
include/linux/blk_types.h | 6 +
10 files changed, 426 insertions(+), 30 deletions(-)
create mode 100644 block/bio-crypt-ctx.c
create mode 100644 include/linux/bio-crypt-ctx.h
diff --git a/block/Makefile b/block/Makefile
index e922844219c2..f39611ed151f 100644
--- a/block/Makefile
+++ b/block/Makefile
@@ -36,4 +36,4 @@ obj-$(CONFIG_BLK_DEBUG_FS)+= blk-mq-debugfs.o
obj-$(CONFIG_BLK_DEBUG_FS_ZONED)+= blk-mq-debugfs-zoned.o
obj-$(CONFIG_BLK_SED_OPAL) += sed-opal.o
obj-$(CONFIG_BLK_PM) += blk-pm.o
-obj-$(CONFIG_BLK_INLINE_ENCRYPTION)+= keyslot-manager.o
+obj-$(CONFIG_BLK_INLINE_ENCRYPTION)+= keyslot-manager.o bio-crypt-ctx.o
diff --git a/block/bio-crypt-ctx.c b/block/bio-crypt-ctx.c
new file mode 100644
index ..aa3571f72ee7
--- /dev/null
+++ b/block/bio-crypt-ctx.c
@@ -0,0 +1,137 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2019 Google LLC
+ */
+
+#include
+#include
+#include
+#include
+
+static int num_prealloc_crypt_ctxs = 128;
+static struct kmem_cache *bio_crypt_ctx_cache;
+static mempool_t *bio_crypt_ctx_pool;
+
+int bio_crypt_ctx_init(void)
+{
+ bio_crypt_ctx_cache = KMEM_CACHE(bio_crypt_ctx, 0);
+ if (!bio_crypt_ctx_cache)
+ return -ENOMEM;
+
+ bio_crypt_ctx_pool = mempool_create_slab_pool(
+ num_prealloc_crypt_ctxs,
+ bio_crypt_ctx_cache);
+
+ if (!bio_crypt_ctx_pool)
+ return -ENOMEM;
+
+ return 0;
+}
+
+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);
+
+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);
+
+int bio_crypt_clone(struct bio *dst, struct bio *src, gfp_t gfp_mask)
+{
+ if (!bio_has_crypt_ctx(src))
+ return 0;
+
+ dst->bi_crypt_context = bio_crypt_alloc_ctx(gfp_mask);
+ if (!dst->bi_crypt_context)
+ return -ENOMEM;
+
+ *dst->bi_crypt_context = *src->bi_crypt_context;
+
+ if (bio_crypt_has_keyslot(src))
+ keyslot_manager_get_slot(src->bi_crypt_context->processing_ksm,
+src->bi_crypt_context->keyslot);
+
+ return 0;
+}
+EXPORT_SYMBOL(bio_crypt_clone);
+
+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;
+}
+EXPORT_SYMBOL(bio_crypt_should_process);
+
+/*
+ * 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;
+}
+
+/*
+ * Checks that two bio crypt contexts are compatible, and also
+ * that their data_unit_nums are continuous (and can hence be merged)
+ */
+bool
