* [PATCH 0/3] block: show crypto capabilities in sysfs
@ 2021-11-26 21:25 Eric Biggers
2021-11-26 21:25 ` [PATCH 1/3] block: simplify calling convention of elv_unregister_queue() Eric Biggers
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Eric Biggers @ 2021-11-26 21:25 UTC (permalink / raw)
To: linux-block; +Cc: linux-kernel, linux-api, linux-scsi, linux-mmc
This series adds sysfs files that expose the inline encryption
capabilities of request queues.
Patches 1 and 2 are some related cleanups for existing blk-sysfs code.
Patch 3 is the real change; see there for more details.
This applies to linux-block/for-next.
Eric Biggers (3):
block: simplify calling convention of elv_unregister_queue()
block: don't delete queue kobject before its children
blk-crypto: show crypto capabilities in sysfs
Documentation/block/queue-sysfs.rst | 30 +++++
block/Makefile | 3 +-
block/blk-crypto-internal.h | 12 ++
block/blk-crypto-sysfs.c | 177 ++++++++++++++++++++++++++++
block/blk-crypto.c | 3 +
block/blk-sysfs.c | 17 ++-
block/elevator.c | 8 +-
include/linux/blkdev.h | 1 +
8 files changed, 241 insertions(+), 10 deletions(-)
create mode 100644 block/blk-crypto-sysfs.c
base-commit: 4d162e24e9979dcb3d7825229982c172ca4bde54
--
2.34.1
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH 1/3] block: simplify calling convention of elv_unregister_queue() 2021-11-26 21:25 [PATCH 0/3] block: show crypto capabilities in sysfs Eric Biggers @ 2021-11-26 21:25 ` Eric Biggers 2021-11-26 21:25 ` [PATCH 2/3] block: don't delete queue kobject before its children Eric Biggers 2021-11-26 21:25 ` [PATCH 3/3] blk-crypto: show crypto capabilities in sysfs Eric Biggers 2 siblings, 0 replies; 7+ messages in thread From: Eric Biggers @ 2021-11-26 21:25 UTC (permalink / raw) To: linux-block; +Cc: linux-kernel, linux-api, linux-scsi, linux-mmc From: Eric Biggers <ebiggers@google.com> Make elv_unregister_queue() a no-op if q->elevator is NULL or is not registered. This simplifies the existing callers, as well as the future caller in the error path of blk_register_queue(). Also don't bother checking whether q is NULL, since it never is. Signed-off-by: Eric Biggers <ebiggers@google.com> --- block/blk-sysfs.c | 3 +-- block/elevator.c | 8 ++++---- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c index 4622da4bb9927..91d3805a6ec6b 100644 --- a/block/blk-sysfs.c +++ b/block/blk-sysfs.c @@ -957,8 +957,7 @@ void blk_unregister_queue(struct gendisk *disk) blk_trace_remove_sysfs(disk_to_dev(disk)); mutex_lock(&q->sysfs_lock); - if (q->elevator) - elv_unregister_queue(q); + elv_unregister_queue(q); disk_unregister_independent_access_ranges(disk); mutex_unlock(&q->sysfs_lock); mutex_unlock(&q->sysfs_dir_lock); diff --git a/block/elevator.c b/block/elevator.c index ec98aed39c4f5..b062c5bc10b9a 100644 --- a/block/elevator.c +++ b/block/elevator.c @@ -516,9 +516,11 @@ int elv_register_queue(struct request_queue *q, bool uevent) void elv_unregister_queue(struct request_queue *q) { + struct elevator_queue *e = q->elevator; + lockdep_assert_held(&q->sysfs_lock); - if (q) { + if (e && e->registered) { struct elevator_queue *e = q->elevator; kobject_uevent(&e->kobj, KOBJ_REMOVE); @@ -593,9 +595,7 @@ int elevator_switch_mq(struct request_queue *q, lockdep_assert_held(&q->sysfs_lock); if (q->elevator) { - if (q->elevator->registered) - elv_unregister_queue(q); - + elv_unregister_queue(q); ioc_clear_queue(q); blk_mq_sched_free_rqs(q); elevator_exit(q); -- 2.34.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/3] block: don't delete queue kobject before its children 2021-11-26 21:25 [PATCH 0/3] block: show crypto capabilities in sysfs Eric Biggers 2021-11-26 21:25 ` [PATCH 1/3] block: simplify calling convention of elv_unregister_queue() Eric Biggers @ 2021-11-26 21:25 ` Eric Biggers 2021-11-26 21:25 ` [PATCH 3/3] blk-crypto: show crypto capabilities in sysfs Eric Biggers 2 siblings, 0 replies; 7+ messages in thread From: Eric Biggers @ 2021-11-26 21:25 UTC (permalink / raw) To: linux-block; +Cc: linux-kernel, linux-api, linux-scsi, linux-mmc From: Eric Biggers <ebiggers@google.com> kobjects aren't supposed to be deleted before their child kobjects are deleted. Apparently this is usually benign; however, a WARN will be triggered if one of the child kobjects has a named attribute group: sysfs group 'modes' not found for kobject 'crypto' WARNING: CPU: 0 PID: 1 at fs/sysfs/group.c:278 sysfs_remove_group+0x72/0x80 ... Call Trace: sysfs_remove_groups+0x29/0x40 fs/sysfs/group.c:312 __kobject_del+0x20/0x80 lib/kobject.c:611 kobject_cleanup+0xa4/0x140 lib/kobject.c:696 kobject_release lib/kobject.c:736 [inline] kref_put include/linux/kref.h:65 [inline] kobject_put+0x53/0x70 lib/kobject.c:753 blk_crypto_sysfs_unregister+0x10/0x20 block/blk-crypto-sysfs.c:159 blk_unregister_queue+0xb0/0x110 block/blk-sysfs.c:962 del_gendisk+0x117/0x250 block/genhd.c:610 Fix this by moving the kobject_del() and the corresponding kobject_uevent() to the correct place. Fixes: 2c2086afc2b8 ("block: Protect less code with sysfs_lock in blk_{un,}register_queue()") Signed-off-by: Eric Biggers <ebiggers@google.com> --- block/blk-sysfs.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c index 91d3805a6ec6b..1368dfe3ee500 100644 --- a/block/blk-sysfs.c +++ b/block/blk-sysfs.c @@ -951,15 +951,17 @@ void blk_unregister_queue(struct gendisk *disk) */ if (queue_is_mq(q)) blk_mq_unregister_dev(disk_to_dev(disk), q); - - kobject_uevent(&q->kobj, KOBJ_REMOVE); - kobject_del(&q->kobj); blk_trace_remove_sysfs(disk_to_dev(disk)); mutex_lock(&q->sysfs_lock); elv_unregister_queue(q); disk_unregister_independent_access_ranges(disk); mutex_unlock(&q->sysfs_lock); + + /* Now that all child objects were deleted, the queue can be deleted. */ + kobject_uevent(&q->kobj, KOBJ_REMOVE); + kobject_del(&q->kobj); + mutex_unlock(&q->sysfs_dir_lock); kobject_put(&disk_to_dev(disk)->kobj); -- 2.34.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 3/3] blk-crypto: show crypto capabilities in sysfs 2021-11-26 21:25 [PATCH 0/3] block: show crypto capabilities in sysfs Eric Biggers 2021-11-26 21:25 ` [PATCH 1/3] block: simplify calling convention of elv_unregister_queue() Eric Biggers 2021-11-26 21:25 ` [PATCH 2/3] block: don't delete queue kobject before its children Eric Biggers @ 2021-11-26 21:25 ` Eric Biggers 2021-11-27 9:06 ` Greg KH 2 siblings, 1 reply; 7+ messages in thread From: Eric Biggers @ 2021-11-26 21:25 UTC (permalink / raw) To: linux-block; +Cc: linux-kernel, linux-api, linux-scsi, linux-mmc From: Eric Biggers <ebiggers@google.com> Add sysfs files that expose the inline encryption capabilities of request queues: /sys/class/block/$disk/queue/crypto/max_dun_bits /sys/class/block/$disk/queue/crypto/modes/$mode /sys/class/block/$disk/queue/crypto/num_keyslots Userspace can use these new files to decide what encryption settings to use, or whether to use inline encryption at all. This also brings the crypto capabilities in line with the other queue properties, which are already discoverable via the queue directory in sysfs. Design notes: - Place the new files in a new subdirectory "crypto" to group them together and to avoid complicating the main "queue" directory. This also makes it possible to replace "crypto" with a symlink later if we ever make the blk_crypto_profiles into real kobjects (see below). - It was necessary to define a new kobject that corresponds to the crypto subdirectory. For now, this kobject just contains a pointer to the blk_crypto_profile. Note that multiple queues (and hence multiple such kobjects) may refer to the same blk_crypto_profile. An alternative design would more closely match the current kernel data structures: the blk_crypto_profile could be a kobject itself, located directly under the host controller device's kobject, while /sys/class/block/$disk/queue/crypto would be a symlink to it. I decided not to do that for now because it would require a lot more changes, such as no longer embedding blk_crypto_profile in other structures, and also because I'm not sure we can rule out moving the crypto capabilities into 'struct queue_limits' in the future. (Even if multiple queues share the same crypto engine, maybe the supported data unit sizes could differ due to other queue properties.) It would also still be possible to switch to that design later without breaking userspace, by replacing the directory with a symlink. - Use "max_dun_bits" instead of "max_dun_bytes". Currently, the kernel internally stores this value in bytes, but that's an implementation detail. It probably makes more sense to talk about this value in bits, and choosing bits is more future-proof. - "modes" is a sub-subdirectory, since there may be multiple supported crypto modes, and sysfs is supposed to have one value per file. - Each mode had to be named. The crypto API names like "xts(aes)" are not appropriate because they don't specify the key size. Therefore, I assigned new names. The exact names chosen are arbitrary, but they happen to match the names used in log messages in fs/crypto/. - The "num_keyslots" file is a bit different from the others in that it is only useful to know for performance reasons. However, it's included as it can still be useful. For example, a user might not want to use inline encryption if there aren't very many keyslots. Signed-off-by: Eric Biggers <ebiggers@google.com> --- Documentation/block/queue-sysfs.rst | 30 +++++ block/Makefile | 3 +- block/blk-crypto-internal.h | 12 ++ block/blk-crypto-sysfs.c | 177 ++++++++++++++++++++++++++++ block/blk-crypto.c | 3 + block/blk-sysfs.c | 6 + include/linux/blkdev.h | 1 + 7 files changed, 231 insertions(+), 1 deletion(-) create mode 100644 block/blk-crypto-sysfs.c diff --git a/Documentation/block/queue-sysfs.rst b/Documentation/block/queue-sysfs.rst index 3f569d5324857..252939f340459 100644 --- a/Documentation/block/queue-sysfs.rst +++ b/Documentation/block/queue-sysfs.rst @@ -24,6 +24,36 @@ or host-managed, chunk_sectors indicates the size in 512B sectors of the zones of the device, with the eventual exception of the last zone of the device which may be smaller. +crypto +------ +This subdirectory is present if the device supports inline encryption. +(See :ref:`Documentation/block/inline-encryption.rst <inline_encryption>`.) +This subdirectory contains the following files and subdirectories which describe +the inline encryption capabilities of the device: + +max_dun_bits (RO) +~~~~~~~~~~~~~~~~~ +The maximum length, in bits, of data unit numbers (DUNs) accepted by the device. + +modes +~~~~~ +This subdirectory contains one file (RO) per crypto mode the device supports. +Each such file contains a hexadecimal number that is a bitmask of the supported +data unit sizes, in bytes, for the crypto mode. + +The crypto modes that may be supported are: + +* AES-256-XTS +* AES-128-CBC-ESSIV +* Adiantum + +For example, if a device supports AES-256-XTS with data unit sizes of 512 and +4096 bytes, the file "AES-256-XTS" will be present and will contain "0x1200". + +num_keyslots (RO) +~~~~~~~~~~~~~~~~~ +The number of crypto keyslots the device has. + dax (RO) -------- This file indicates whether the device supports Direct Access (DAX), diff --git a/block/Makefile b/block/Makefile index f38eaa6129296..3950ecbc5c263 100644 --- a/block/Makefile +++ b/block/Makefile @@ -36,6 +36,7 @@ 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) += blk-crypto.o blk-crypto-profile.o +obj-$(CONFIG_BLK_INLINE_ENCRYPTION) += blk-crypto.o blk-crypto-profile.o \ + blk-crypto-sysfs.o obj-$(CONFIG_BLK_INLINE_ENCRYPTION_FALLBACK) += blk-crypto-fallback.o obj-$(CONFIG_BLOCK_HOLDER_DEPRECATED) += holder.o diff --git a/block/blk-crypto-internal.h b/block/blk-crypto-internal.h index 2fb0d65a464ca..e6818ffaddbf8 100644 --- a/block/blk-crypto-internal.h +++ b/block/blk-crypto-internal.h @@ -11,6 +11,7 @@ /* Represents a crypto mode supported by blk-crypto */ struct blk_crypto_mode { + const char *name; /* name of this mode, shown in sysfs */ const char *cipher_str; /* crypto API name (for fallback case) */ unsigned int keysize; /* key size in bytes */ unsigned int ivsize; /* iv size in bytes */ @@ -20,6 +21,10 @@ extern const struct blk_crypto_mode blk_crypto_modes[]; #ifdef CONFIG_BLK_INLINE_ENCRYPTION +int blk_crypto_sysfs_register(struct request_queue *q); + +void blk_crypto_sysfs_unregister(struct request_queue *q); + void bio_crypt_dun_increment(u64 dun[BLK_CRYPTO_DUN_ARRAY_SIZE], unsigned int inc); @@ -62,6 +67,13 @@ static inline bool blk_crypto_rq_is_encrypted(struct request *rq) #else /* CONFIG_BLK_INLINE_ENCRYPTION */ +static inline int blk_crypto_sysfs_register(struct request_queue *q) +{ + return 0; +} + +static inline void blk_crypto_sysfs_unregister(struct request_queue *q) { } + static inline bool bio_crypt_rq_ctx_compatible(struct request *rq, struct bio *bio) { diff --git a/block/blk-crypto-sysfs.c b/block/blk-crypto-sysfs.c new file mode 100644 index 0000000000000..fcfb711c296e3 --- /dev/null +++ b/block/blk-crypto-sysfs.c @@ -0,0 +1,177 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright 2021 Google LLC + * + * sysfs support for blk-crypto. This file contains the code which exports the + * crypto capabilities of devices via /sys/class/block/$disk/queue/crypto/. + */ + +#include <linux/blk-crypto-profile.h> + +#include "blk-crypto-internal.h" + +struct blk_crypto_kobj { + struct kobject kobj; + struct blk_crypto_profile *profile; +}; + +struct blk_crypto_attr { + struct attribute attr; + ssize_t (*show)(struct blk_crypto_profile *profile, + struct blk_crypto_attr *attr, char *page); +}; + +static struct blk_crypto_profile *kobj_to_crypto_profile(struct kobject *kobj) +{ + return container_of(kobj, struct blk_crypto_kobj, kobj)->profile; +} + +static struct blk_crypto_attr *attr_to_crypto_attr(struct attribute *attr) +{ + return container_of(attr, struct blk_crypto_attr, attr); +} + +static ssize_t blk_crypto_max_dun_bits_show(struct blk_crypto_profile *profile, + struct blk_crypto_attr *attr, + char *page) +{ + return sprintf(page, "%u\n", 8 * profile->max_dun_bytes_supported); +} + +static ssize_t blk_crypto_num_keyslots_show(struct blk_crypto_profile *profile, + struct blk_crypto_attr *attr, + char *page) +{ + return sprintf(page, "%u\n", profile->num_slots); +} + +#define BLK_CRYPTO_RO_ATTR(_name) \ +static struct blk_crypto_attr blk_crypto_##_name = { \ + .attr = { .name = #_name, .mode = 0444 }, \ + .show = blk_crypto_##_name##_show, \ +} + +BLK_CRYPTO_RO_ATTR(max_dun_bits); +BLK_CRYPTO_RO_ATTR(num_keyslots); + +static struct attribute *blk_crypto_attrs[] = { + &blk_crypto_max_dun_bits.attr, + &blk_crypto_num_keyslots.attr, + NULL, +}; + +static const struct attribute_group blk_crypto_attr_group = { + .attrs = blk_crypto_attrs, +}; + +/* + * The encryption mode attributes. To avoid hard-coding the list of encryption + * modes, these are initialized at boot time by blk_crypto_sysfs_init(). + */ +static struct blk_crypto_attr __blk_crypto_mode_attrs[BLK_ENCRYPTION_MODE_MAX]; +static struct attribute *blk_crypto_mode_attrs[BLK_ENCRYPTION_MODE_MAX + 1]; + +static umode_t blk_crypto_mode_is_visible(struct kobject *kobj, + struct attribute *attr, int n) +{ + struct blk_crypto_profile *profile = kobj_to_crypto_profile(kobj); + struct blk_crypto_attr *a = attr_to_crypto_attr(attr); + int mode_num = a - __blk_crypto_mode_attrs; + + if (profile->modes_supported[mode_num]) + return 0444; + return 0; +} + +static ssize_t blk_crypto_mode_show(struct blk_crypto_profile *profile, + struct blk_crypto_attr *attr, char *page) +{ + int mode_num = attr - __blk_crypto_mode_attrs; + + return sprintf(page, "0x%x\n", profile->modes_supported[mode_num]); +} + +static const struct attribute_group blk_crypto_modes_attr_group = { + .name = "modes", + .attrs = blk_crypto_mode_attrs, + .is_visible = blk_crypto_mode_is_visible, +}; + +static const struct attribute_group *blk_crypto_attr_groups[] = { + &blk_crypto_attr_group, + &blk_crypto_modes_attr_group, + NULL, +}; + +static ssize_t blk_crypto_attr_show(struct kobject *kobj, + struct attribute *attr, char *page) +{ + struct blk_crypto_profile *profile = kobj_to_crypto_profile(kobj); + struct blk_crypto_attr *a = attr_to_crypto_attr(attr); + + return a->show(profile, a, page); +} + +static const struct sysfs_ops blk_crypto_attr_ops = { + .show = blk_crypto_attr_show, +}; + +static void blk_crypto_release(struct kobject *kobj) +{ + kfree(container_of(kobj, struct blk_crypto_kobj, kobj)); +} + +static struct kobj_type blk_crypto_ktype = { + .default_groups = blk_crypto_attr_groups, + .sysfs_ops = &blk_crypto_attr_ops, + .release = blk_crypto_release, +}; + +/* + * If the request_queue has a blk_crypto_profile, create the "crypto" + * subdirectory in sysfs (/sys/class/block/$disk/queue/crypto/). + */ +int blk_crypto_sysfs_register(struct request_queue *q) +{ + struct blk_crypto_kobj *obj; + int err; + + if (!q->crypto_profile) + return 0; + + obj = kzalloc(sizeof(*obj), GFP_KERNEL); + if (!obj) + return -ENOMEM; + obj->profile = q->crypto_profile; + + err = kobject_init_and_add(&obj->kobj, &blk_crypto_ktype, &q->kobj, + "crypto"); + if (err) { + kobject_put(&obj->kobj); + return err; + } + q->crypto_kobject = &obj->kobj; + return 0; +} + +void blk_crypto_sysfs_unregister(struct request_queue *q) +{ + kobject_put(q->crypto_kobject); +} + +static int __init blk_crypto_sysfs_init(void) +{ + int i; + + BUILD_BUG_ON(BLK_ENCRYPTION_MODE_INVALID != 0); + for (i = 1; i < BLK_ENCRYPTION_MODE_MAX; i++) { + struct blk_crypto_attr *attr = &__blk_crypto_mode_attrs[i]; + + attr->attr.name = blk_crypto_modes[i].name; + attr->attr.mode = 0444; + attr->show = blk_crypto_mode_show; + blk_crypto_mode_attrs[i - 1] = &attr->attr; + } + return 0; +} +subsys_initcall(blk_crypto_sysfs_init); diff --git a/block/blk-crypto.c b/block/blk-crypto.c index ec9efeeeca918..f8a36c723a987 100644 --- a/block/blk-crypto.c +++ b/block/blk-crypto.c @@ -19,16 +19,19 @@ const struct blk_crypto_mode blk_crypto_modes[] = { [BLK_ENCRYPTION_MODE_AES_256_XTS] = { + .name = "AES-256-XTS", .cipher_str = "xts(aes)", .keysize = 64, .ivsize = 16, }, [BLK_ENCRYPTION_MODE_AES_128_CBC_ESSIV] = { + .name = "AES-128-CBC-ESSIV", .cipher_str = "essiv(cbc(aes),sha256)", .keysize = 16, .ivsize = 16, }, [BLK_ENCRYPTION_MODE_ADIANTUM] = { + .name = "Adiantum", .cipher_str = "adiantum(xchacha12,aes)", .keysize = 32, .ivsize = 32, diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c index 1368dfe3ee500..e551498a0a850 100644 --- a/block/blk-sysfs.c +++ b/block/blk-sysfs.c @@ -876,6 +876,10 @@ int blk_register_queue(struct gendisk *disk) goto put_dev; } + ret = blk_crypto_sysfs_register(q); + if (ret) + goto put_dev; + blk_queue_flag_set(QUEUE_FLAG_REGISTERED, q); wbt_enable_default(q); blk_throtl_register_queue(q); @@ -907,6 +911,7 @@ int blk_register_queue(struct gendisk *disk) return ret; put_dev: + elv_unregister_queue(q); disk_unregister_independent_access_ranges(disk); mutex_unlock(&q->sysfs_lock); mutex_unlock(&q->sysfs_dir_lock); @@ -951,6 +956,7 @@ void blk_unregister_queue(struct gendisk *disk) */ if (queue_is_mq(q)) blk_mq_unregister_dev(disk_to_dev(disk), q); + blk_crypto_sysfs_unregister(q); blk_trace_remove_sysfs(disk_to_dev(disk)); mutex_lock(&q->sysfs_lock); diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index 0a4416ef4fbf8..1fed574e56133 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -261,6 +261,7 @@ struct request_queue { #ifdef CONFIG_BLK_INLINE_ENCRYPTION struct blk_crypto_profile *crypto_profile; + struct kobject *crypto_kobject; #endif unsigned int rq_timeout; -- 2.34.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 3/3] blk-crypto: show crypto capabilities in sysfs 2021-11-26 21:25 ` [PATCH 3/3] blk-crypto: show crypto capabilities in sysfs Eric Biggers @ 2021-11-27 9:06 ` Greg KH 2021-11-27 20:47 ` Eric Biggers 0 siblings, 1 reply; 7+ messages in thread From: Greg KH @ 2021-11-27 9:06 UTC (permalink / raw) To: Eric Biggers; +Cc: linux-block, linux-kernel, linux-api, linux-scsi, linux-mmc On Fri, Nov 26, 2021 at 01:25:14PM -0800, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > Add sysfs files that expose the inline encryption capabilities of > request queues: > > /sys/class/block/$disk/queue/crypto/max_dun_bits > /sys/class/block/$disk/queue/crypto/modes/$mode > /sys/class/block/$disk/queue/crypto/num_keyslots > > Userspace can use these new files to decide what encryption settings to > use, or whether to use inline encryption at all. This also brings the > crypto capabilities in line with the other queue properties, which are > already discoverable via the queue directory in sysfs. > > Design notes: > > - Place the new files in a new subdirectory "crypto" to group them > together and to avoid complicating the main "queue" directory. This > also makes it possible to replace "crypto" with a symlink later if > we ever make the blk_crypto_profiles into real kobjects (see below). > > - It was necessary to define a new kobject that corresponds to the > crypto subdirectory. For now, this kobject just contains a pointer > to the blk_crypto_profile. Note that multiple queues (and hence > multiple such kobjects) may refer to the same blk_crypto_profile. > > An alternative design would more closely match the current kernel > data structures: the blk_crypto_profile could be a kobject itself, > located directly under the host controller device's kobject, while > /sys/class/block/$disk/queue/crypto would be a symlink to it. > > I decided not to do that for now because it would require a lot more > changes, such as no longer embedding blk_crypto_profile in other > structures, and also because I'm not sure we can rule out moving the > crypto capabilities into 'struct queue_limits' in the future. (Even > if multiple queues share the same crypto engine, maybe the supported > data unit sizes could differ due to other queue properties.) It > would also still be possible to switch to that design later without > breaking userspace, by replacing the directory with a symlink. > > - Use "max_dun_bits" instead of "max_dun_bytes". Currently, the > kernel internally stores this value in bytes, but that's an > implementation detail. It probably makes more sense to talk about > this value in bits, and choosing bits is more future-proof. > > - "modes" is a sub-subdirectory, since there may be multiple supported > crypto modes, and sysfs is supposed to have one value per file. > > - Each mode had to be named. The crypto API names like "xts(aes)" are > not appropriate because they don't specify the key size. Therefore, > I assigned new names. The exact names chosen are arbitrary, but > they happen to match the names used in log messages in fs/crypto/. > > - The "num_keyslots" file is a bit different from the others in that > it is only useful to know for performance reasons. However, it's > included as it can still be useful. For example, a user might not > want to use inline encryption if there aren't very many keyslots. > > Signed-off-by: Eric Biggers <ebiggers@google.com> > --- > Documentation/block/queue-sysfs.rst | 30 +++++ > block/Makefile | 3 +- > block/blk-crypto-internal.h | 12 ++ > block/blk-crypto-sysfs.c | 177 ++++++++++++++++++++++++++++ > block/blk-crypto.c | 3 + > block/blk-sysfs.c | 6 + > include/linux/blkdev.h | 1 + > 7 files changed, 231 insertions(+), 1 deletion(-) > create mode 100644 block/blk-crypto-sysfs.c > > diff --git a/Documentation/block/queue-sysfs.rst b/Documentation/block/queue-sysfs.rst > index 3f569d5324857..252939f340459 100644 > --- a/Documentation/block/queue-sysfs.rst > +++ b/Documentation/block/queue-sysfs.rst Why is all of this information not in Documentation/ABI/ like the rest of the kernel's sysfs information? When it is there it can be automatically tested as well. Please don't add new entries to the wrong place if at all possible. > --- /dev/null > +++ b/block/blk-crypto-sysfs.c > @@ -0,0 +1,177 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright 2021 Google LLC > + * > + * sysfs support for blk-crypto. This file contains the code which exports the > + * crypto capabilities of devices via /sys/class/block/$disk/queue/crypto/. > + */ > + > +#include <linux/blk-crypto-profile.h> > + > +#include "blk-crypto-internal.h" > + > +struct blk_crypto_kobj { > + struct kobject kobj; > + struct blk_crypto_profile *profile; > +}; > + > +struct blk_crypto_attr { > + struct attribute attr; > + ssize_t (*show)(struct blk_crypto_profile *profile, > + struct blk_crypto_attr *attr, char *page); > +}; > + > +static struct blk_crypto_profile *kobj_to_crypto_profile(struct kobject *kobj) > +{ > + return container_of(kobj, struct blk_crypto_kobj, kobj)->profile; > +} > + > +static struct blk_crypto_attr *attr_to_crypto_attr(struct attribute *attr) > +{ > + return container_of(attr, struct blk_crypto_attr, attr); > +} > + > +static ssize_t blk_crypto_max_dun_bits_show(struct blk_crypto_profile *profile, > + struct blk_crypto_attr *attr, > + char *page) > +{ > + return sprintf(page, "%u\n", 8 * profile->max_dun_bytes_supported); sysfs_emit() please, for this, and all other show functions. > +} > + > +static ssize_t blk_crypto_num_keyslots_show(struct blk_crypto_profile *profile, > + struct blk_crypto_attr *attr, > + char *page) > +{ > + return sprintf(page, "%u\n", profile->num_slots); > +} > + > +#define BLK_CRYPTO_RO_ATTR(_name) \ > +static struct blk_crypto_attr blk_crypto_##_name = { \ > + .attr = { .name = #_name, .mode = 0444 }, \ __ATTR_RO()? > + .show = blk_crypto_##_name##_show, \ > +} > + > +BLK_CRYPTO_RO_ATTR(max_dun_bits); > +BLK_CRYPTO_RO_ATTR(num_keyslots); > + > +static struct attribute *blk_crypto_attrs[] = { > + &blk_crypto_max_dun_bits.attr, > + &blk_crypto_num_keyslots.attr, > + NULL, > +}; > + > +static const struct attribute_group blk_crypto_attr_group = { > + .attrs = blk_crypto_attrs, > +}; > + > +/* > + * The encryption mode attributes. To avoid hard-coding the list of encryption > + * modes, these are initialized at boot time by blk_crypto_sysfs_init(). > + */ > +static struct blk_crypto_attr __blk_crypto_mode_attrs[BLK_ENCRYPTION_MODE_MAX]; > +static struct attribute *blk_crypto_mode_attrs[BLK_ENCRYPTION_MODE_MAX + 1]; > + > +static umode_t blk_crypto_mode_is_visible(struct kobject *kobj, > + struct attribute *attr, int n) > +{ > + struct blk_crypto_profile *profile = kobj_to_crypto_profile(kobj); > + struct blk_crypto_attr *a = attr_to_crypto_attr(attr); > + int mode_num = a - __blk_crypto_mode_attrs; > + > + if (profile->modes_supported[mode_num]) > + return 0444; > + return 0; > +} > + > +static ssize_t blk_crypto_mode_show(struct blk_crypto_profile *profile, > + struct blk_crypto_attr *attr, char *page) > +{ > + int mode_num = attr - __blk_crypto_mode_attrs; > + > + return sprintf(page, "0x%x\n", profile->modes_supported[mode_num]); > +} > + > +static const struct attribute_group blk_crypto_modes_attr_group = { > + .name = "modes", > + .attrs = blk_crypto_mode_attrs, > + .is_visible = blk_crypto_mode_is_visible, > +}; > + > +static const struct attribute_group *blk_crypto_attr_groups[] = { > + &blk_crypto_attr_group, > + &blk_crypto_modes_attr_group, > + NULL, > +}; ATTRIBUTE_GROUP()? Hm, maybe not, but I think it could be used here. > + > +static ssize_t blk_crypto_attr_show(struct kobject *kobj, > + struct attribute *attr, char *page) > +{ > + struct blk_crypto_profile *profile = kobj_to_crypto_profile(kobj); > + struct blk_crypto_attr *a = attr_to_crypto_attr(attr); > + > + return a->show(profile, a, page); > +} > + > +static const struct sysfs_ops blk_crypto_attr_ops = { > + .show = blk_crypto_attr_show, > +}; > + > +static void blk_crypto_release(struct kobject *kobj) > +{ > + kfree(container_of(kobj, struct blk_crypto_kobj, kobj)); > +} > + > +static struct kobj_type blk_crypto_ktype = { > + .default_groups = blk_crypto_attr_groups, > + .sysfs_ops = &blk_crypto_attr_ops, > + .release = blk_crypto_release, > +}; > + > +/* > + * If the request_queue has a blk_crypto_profile, create the "crypto" > + * subdirectory in sysfs (/sys/class/block/$disk/queue/crypto/). > + */ > +int blk_crypto_sysfs_register(struct request_queue *q) > +{ > + struct blk_crypto_kobj *obj; > + int err; > + > + if (!q->crypto_profile) > + return 0; > + > + obj = kzalloc(sizeof(*obj), GFP_KERNEL); > + if (!obj) > + return -ENOMEM; > + obj->profile = q->crypto_profile; > + > + err = kobject_init_and_add(&obj->kobj, &blk_crypto_ktype, &q->kobj, > + "crypto"); > + if (err) { > + kobject_put(&obj->kobj); > + return err; > + } > + q->crypto_kobject = &obj->kobj; > + return 0; > +} > + > +void blk_crypto_sysfs_unregister(struct request_queue *q) > +{ > + kobject_put(q->crypto_kobject); > +} > + > +static int __init blk_crypto_sysfs_init(void) > +{ > + int i; > + > + BUILD_BUG_ON(BLK_ENCRYPTION_MODE_INVALID != 0); > + for (i = 1; i < BLK_ENCRYPTION_MODE_MAX; i++) { > + struct blk_crypto_attr *attr = &__blk_crypto_mode_attrs[i]; sysfs_attr_init() might be needed here, have you run with lockdep enabled? Other than those minor things, nice work, all looks good. greg k-h ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 3/3] blk-crypto: show crypto capabilities in sysfs 2021-11-27 9:06 ` Greg KH @ 2021-11-27 20:47 ` Eric Biggers 2021-11-28 8:14 ` Greg KH 0 siblings, 1 reply; 7+ messages in thread From: Eric Biggers @ 2021-11-27 20:47 UTC (permalink / raw) To: Greg KH; +Cc: linux-block, linux-kernel, linux-api, linux-scsi, linux-mmc Hi Greg, thanks for the review! On Sat, Nov 27, 2021 at 10:06:18AM +0100, Greg KH wrote: > > diff --git a/Documentation/block/queue-sysfs.rst b/Documentation/block/queue-sysfs.rst > > index 3f569d5324857..252939f340459 100644 > > --- a/Documentation/block/queue-sysfs.rst > > +++ b/Documentation/block/queue-sysfs.rst > > Why is all of this information not in Documentation/ABI/ like the rest > of the kernel's sysfs information? When it is there it can be > automatically tested as well. > > Please don't add new entries to the wrong place if at all possible. Some of the block queue attributes are documented in Documentation/ABI/testing/sysfs-block, but Documentation/block/queue-sysfs.rst seems to be the authoritative source in practice. I checked all QUEUE_*_ENTRY in block/blk-sysfs.c, and I got: - 16 attributes are documented in both places - 23 attributes are documented in Documentation/block/ only - 0 attributes are documented in Documentation/ABI/ only - 2 attributes ("virt_boundary_mask" and "stable_writes") not documented in either place So most block queue attributes are documented only in Documentation/block/. And if I added my new attributes to Documentation/ABI/ only, as you're requesting, they would be the only block queue attributes that would be documented in only that place. I think that would make things worse, as then there would be no authoritative source anymore. If both you and the block people agree that *all* block queue attributes should be documented in Documentation/ABI/ only, I'd be glad to send a separate patch that adds anything missing to Documentation/ABI/testing/sysfs-block, then removes Documentation/block/queue-sysfs.rst. (BTW, shouldn't it really be in Documentation/ABI/stable/? This ABI has been around a long time, so surely users are relying on it.) But it doesn't seem fair to block this patch on that. > > +static ssize_t blk_crypto_max_dun_bits_show(struct blk_crypto_profile *profile, > > + struct blk_crypto_attr *attr, > > + char *page) > > +{ > > + return sprintf(page, "%u\n", 8 * profile->max_dun_bytes_supported); > > sysfs_emit() please, for this, and all other show functions. Sure. Note that in .show() functions kernel-wide, it appears that sprintf() is much more commonly used than sysfs_emit(). Is there any plan to convert these? As-is, if people use existing code as a reference, it will be "wrong" most of the time, which is unfortunate. > > +} > > + > > +static ssize_t blk_crypto_num_keyslots_show(struct blk_crypto_profile *profile, > > + struct blk_crypto_attr *attr, > > + char *page) > > +{ > > + return sprintf(page, "%u\n", profile->num_slots); > > +} > > + > > +#define BLK_CRYPTO_RO_ATTR(_name) \ > > +static struct blk_crypto_attr blk_crypto_##_name = { \ > > + .attr = { .name = #_name, .mode = 0444 }, \ > > __ATTR_RO()? Sure. This would require removing the "blk_crypto_" prefix from the .show() functions, which I'd prefer to have, but it doesn't really matter. > > +static const struct attribute_group *blk_crypto_attr_groups[] = { > > + &blk_crypto_attr_group, > > + &blk_crypto_modes_attr_group, > > + NULL, > > +}; > > ATTRIBUTE_GROUP()? > > Hm, maybe not, but I think it could be used here. ATTRIBUTE_GROUP() doesn't exist; probably you're referring to ATTRIBUTE_GROUPS()? ATTRIBUTE_GROUPS() is only usable when there is only one attribute group. In this case, there are two attribute groups. > > +static int __init blk_crypto_sysfs_init(void) > > +{ > > + int i; > > + > > + BUILD_BUG_ON(BLK_ENCRYPTION_MODE_INVALID != 0); > > + for (i = 1; i < BLK_ENCRYPTION_MODE_MAX; i++) { > > + struct blk_crypto_attr *attr = &__blk_crypto_mode_attrs[i]; > > sysfs_attr_init() might be needed here, have you run with lockdep > enabled? It's not needed because __blk_crypto_mode_attrs isn't dynamically allocated memory. Yes, I've run with lockdep enabled. - Eric ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 3/3] blk-crypto: show crypto capabilities in sysfs 2021-11-27 20:47 ` Eric Biggers @ 2021-11-28 8:14 ` Greg KH 0 siblings, 0 replies; 7+ messages in thread From: Greg KH @ 2021-11-28 8:14 UTC (permalink / raw) To: Eric Biggers; +Cc: linux-block, linux-kernel, linux-api, linux-scsi, linux-mmc On Sat, Nov 27, 2021 at 12:47:14PM -0800, Eric Biggers wrote: > Hi Greg, thanks for the review! > > On Sat, Nov 27, 2021 at 10:06:18AM +0100, Greg KH wrote: > > > diff --git a/Documentation/block/queue-sysfs.rst b/Documentation/block/queue-sysfs.rst > > > index 3f569d5324857..252939f340459 100644 > > > --- a/Documentation/block/queue-sysfs.rst > > > +++ b/Documentation/block/queue-sysfs.rst > > > > Why is all of this information not in Documentation/ABI/ like the rest > > of the kernel's sysfs information? When it is there it can be > > automatically tested as well. > > > > Please don't add new entries to the wrong place if at all possible. > > Some of the block queue attributes are documented in > Documentation/ABI/testing/sysfs-block, but Documentation/block/queue-sysfs.rst > seems to be the authoritative source in practice. I checked all QUEUE_*_ENTRY > in block/blk-sysfs.c, and I got: > > - 16 attributes are documented in both places > - 23 attributes are documented in Documentation/block/ only > - 0 attributes are documented in Documentation/ABI/ only > - 2 attributes ("virt_boundary_mask" and "stable_writes") not documented in > either place > > So most block queue attributes are documented only in Documentation/block/. And > if I added my new attributes to Documentation/ABI/ only, as you're requesting, > they would be the only block queue attributes that would be documented in only > that place. I think that would make things worse, as then there would be no > authoritative source anymore. I agree, it should all move to the proper location in Documentation/ABI/ as that is where all sysfs attributes need to be documented. Block queues are not special here. > If both you and the block people agree that *all* block queue attributes should > be documented in Documentation/ABI/ only, I'd be glad to send a separate patch > that adds anything missing to Documentation/ABI/testing/sysfs-block, then > removes Documentation/block/queue-sysfs.rst. (BTW, shouldn't it really be in > Documentation/ABI/stable/? This ABI has been around a long time, so surely > users are relying on it.) But it doesn't seem fair to block this patch on that. "stable" is fine with me, people abuse "testing" by throwing everything into it. > > > > +static ssize_t blk_crypto_max_dun_bits_show(struct blk_crypto_profile *profile, > > > + struct blk_crypto_attr *attr, > > > + char *page) > > > +{ > > > + return sprintf(page, "%u\n", 8 * profile->max_dun_bytes_supported); > > > > sysfs_emit() please, for this, and all other show functions. > > Sure. Note that in .show() functions kernel-wide, it appears that sprintf() is > much more commonly used than sysfs_emit(). Is there any plan to convert these? > As-is, if people use existing code as a reference, it will be "wrong" most of > the time, which is unfortunate. Doing a wholesale replacement across the kernel is a pain and disruptive and not really needed. But for all new code, please use the new functions. If you want to convert your driver/subsystem to the new functions, no objection from me! > > > +} > > > + > > > +static ssize_t blk_crypto_num_keyslots_show(struct blk_crypto_profile *profile, > > > + struct blk_crypto_attr *attr, > > > + char *page) > > > +{ > > > + return sprintf(page, "%u\n", profile->num_slots); > > > +} > > > + > > > +#define BLK_CRYPTO_RO_ATTR(_name) \ > > > +static struct blk_crypto_attr blk_crypto_##_name = { \ > > > + .attr = { .name = #_name, .mode = 0444 }, \ > > > > __ATTR_RO()? > > Sure. This would require removing the "blk_crypto_" prefix from the .show() > functions, which I'd prefer to have, but it doesn't really matter. Ah, you are right, but I think using the default macros sometimes can be nicer as they are easier to verify you are doing things correctly. > > > +static const struct attribute_group *blk_crypto_attr_groups[] = { > > > + &blk_crypto_attr_group, > > > + &blk_crypto_modes_attr_group, > > > + NULL, > > > +}; > > > > ATTRIBUTE_GROUP()? > > > > Hm, maybe not, but I think it could be used here. > > ATTRIBUTE_GROUP() doesn't exist; probably you're referring to > ATTRIBUTE_GROUPS()? ATTRIBUTE_GROUPS() is only usable when there is only one > attribute group. In this case, there are two attribute groups. You are right, sorry. > > > +static int __init blk_crypto_sysfs_init(void) > > > +{ > > > + int i; > > > + > > > + BUILD_BUG_ON(BLK_ENCRYPTION_MODE_INVALID != 0); > > > + for (i = 1; i < BLK_ENCRYPTION_MODE_MAX; i++) { > > > + struct blk_crypto_attr *attr = &__blk_crypto_mode_attrs[i]; > > > > sysfs_attr_init() might be needed here, have you run with lockdep > > enabled? > > It's not needed because __blk_crypto_mode_attrs isn't dynamically allocated > memory. Yes, I've run with lockdep enabled. Ok, good, just checking. thanks, greg k-h ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-11-28 8:16 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-26 21:25 [PATCH 0/3] block: show crypto capabilities in sysfs Eric Biggers 2021-11-26 21:25 ` [PATCH 1/3] block: simplify calling convention of elv_unregister_queue() Eric Biggers 2021-11-26 21:25 ` [PATCH 2/3] block: don't delete queue kobject before its children Eric Biggers 2021-11-26 21:25 ` [PATCH 3/3] blk-crypto: show crypto capabilities in sysfs Eric Biggers 2021-11-27 9:06 ` Greg KH 2021-11-27 20:47 ` Eric Biggers 2021-11-28 8:14 ` Greg KH
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).