Re: [PATCH v2 3/3] blk-crypto: show crypto capabilities in sysfs

Hannes Reinecke
2021-11-30T06:50:00Z

On 11/30/21 5:03 AM, 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.
>
Why do you have a sub-directory here?
From what I can see, that subdirectory just contains the supported
modes, so wouldn't it be easier to create individual files like
'mode_<modename>' instead of a subdirectory?

Cheers,

Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer