Re: [RFC PATCH v2 1/8] block: Keyslot Manager for Inline Encryption

[Eric Biggers <ebiggers@kernel.org>]

On Wed, Jun 05, 2019 at 04:28:30PM -0700, Satya Tangirala wrote:
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 592669bcc536..f76d5dff27fe 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -385,6 +385,10 @@ static inline int blkdev_reset_zones_ioctl(struct block_device *bdev,
>  
>  #endif /* CONFIG_BLK_DEV_ZONED */
>  
> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
> +struct keyslot_manager;
> +#endif
> +

This should be placed with the other forward declarations at the beginning of
the file.  It also doesn't need to be behind an #ifdef.  See e.g. struct
blkcg_gq which is another conditional field in struct request_queue.

> diff --git a/include/linux/keyslot-manager.h b/include/linux/keyslot-manager.h
> new file mode 100644
> index 000000000000..76a9c255cb7e
> --- /dev/null
> +++ b/include/linux/keyslot-manager.h
[...]
> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
> +struct keyslot_manager;
> +
> +extern struct keyslot_manager *keyslot_manager_create(unsigned int num_slots,
> +				const struct keyslot_mgmt_ll_ops *ksm_ops,
> +				void *ll_priv_data);
> +
> +extern int
> +keyslot_manager_get_slot_for_key(struct keyslot_manager *ksm,
> +				 const u8 *key,
> +				 enum blk_crypt_mode_num crypt_mode,
> +				 unsigned int data_unit_size);
> +
> +extern void keyslot_manager_get_slot(struct keyslot_manager *ksm,
> +				     unsigned int slot);
> +
> +extern void keyslot_manager_put_slot(struct keyslot_manager *ksm,
> +				     unsigned int slot);
> +
> +extern int keyslot_manager_evict_key(struct keyslot_manager *ksm,
> +				     const u8 *key,
> +				     enum blk_crypt_mode_num crypt_mode,
> +				     unsigned int data_unit_size);
> +
> +extern void keyslot_manager_destroy(struct keyslot_manager *ksm);
> +
> +#else /* CONFIG_BLK_INLINE_ENCRYPTION */
> +struct keyslot_manager {};

This is actually a struct definition, not a declaration.  This doesn't make
sense, since the CONFIG_BLK_INLINE_ENCRYPTION case only needs a forward
declaration here.  Both cases should just use a forward declaration.

> +
> +static inline struct keyslot_manager *
> +keyslot_manager_create(unsigned int num_slots,
> +		       const struct keyslot_mgmt_ll_ops *ksm_ops,
> +		       void *ll_priv_data)
> +{
> +	return NULL;
> +}
> +
> +static inline int
> +keyslot_manager_get_slot_for_key(struct keyslot_manager *ksm,
> +				 const u8 *key,
> +				 enum blk_crypt_mode_num crypt_mode,
> +				 unsigned int data_unit_size)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static inline void keyslot_manager_get_slot(struct keyslot_manager *ksm,
> +					    unsigned int slot) { }
> +
> +static inline int keyslot_manager_put_slot(struct keyslot_manager *ksm,
> +					   unsigned int slot)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static inline int keyslot_manager_evict_key(struct keyslot_manager *ksm,
> +				     const u8 *key,
> +				     enum blk_crypt_mode_num crypt_mode,
> +				     unsigned int data_unit_size)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static inline void keyslot_manager_destroy(struct keyslot_manager *ksm)
> +{ }
> +
> +#endif /* CONFIG_BLK_INLINE_ENCRYPTION */

However, it seems we don't actually need these stub functions, since the
keyslot_manager_ functions are only called from .c files that are only compiled
when CONFIG_BLK_INLINE_ENCRYPTION, except for the call to
keyslot_manager_evict_key() in fscrypt_evict_crypt_key().  But it would make
more sense to stub out fscrypt_evict_crypt_key() instead.

So I suggest removing the keyslot_manager_* stubs for now.

- Eric