Linux-f2fs-devel Archive on lore.kernel.org
 help / color / Atom feed
From: Eric Biggers <ebiggers@kernel.org>
To: Satya Tangirala <satyat@google.com>
Cc: linux-scsi@vger.kernel.org, Kim Boojin <boojin.kim@samsung.com>,
	Kuohong Wang <kuohong.wang@mediatek.com>,
	Barani Muthukumaran <bmuthuku@qti.qualcomm.com>,
	linux-f2fs-devel@lists.sourceforge.net,
	linux-block@vger.kernel.org, linux-fscrypt@vger.kernel.org,
	linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org
Subject: Re: [f2fs-dev] [PATCH v9 01/11] block: Keyslot Manager for Inline Encryption
Date: Wed, 25 Mar 2020 23:22:13 -0700
Message-ID: <20200326062213.GF858@sol.localdomain> (raw)
In-Reply-To: <20200326030702.223233-2-satyat@google.com>

On Wed, Mar 25, 2020 at 08:06:52PM -0700, Satya Tangirala wrote:
> Inline Encryption hardware allows software to specify an encryption context
> (an encryption key, crypto algorithm, data unit num, data unit size) along
> with a data transfer request to a storage device, and the inline encryption
> hardware will use that context to en/decrypt the data. The inline
> encryption hardware is part of the storage device, and it conceptually sits
> on the data path between system memory and the storage device.
> 
> Inline Encryption hardware implementations often function around the
> concept of "keyslots". These implementations often have a limited number
> of "keyslots", each of which can hold a key (we say that a key can be
> "programmed" into a keyslot). Requests made to the storage device may have
> a keyslot and a data unit number associated with them, and the inline
> encryption hardware will en/decrypt the data in the requests using the key
> programmed into that associated keyslot and the data unit number specified
> with the request.
> 
> As keyslots are limited, and programming keys may be expensive in many
> implementations, and multiple requests may use exactly the same encryption
> contexts, we introduce a Keyslot Manager to efficiently manage keyslots.
> 
> We also introduce a blk_crypto_key, which will represent the key that's
> programmed into keyslots managed by keyslot managers. The keyslot manager
> also functions as the interface that upper layers will use to program keys
> into inline encryption hardware. For more information on the Keyslot
> Manager, refer to documentation found in block/keyslot-manager.c and
> linux/keyslot-manager.h.
> 
> Co-developed-by: Eric Biggers <ebiggers@google.com>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> Signed-off-by: Satya Tangirala <satyat@google.com>

Thanks, this patch looks much better now.  I don't see any real issues, but as
usual here are a couple nits I noticed while reading through this latest version
:-)

> +/**
> + * blk_ksm_init() - Initialize a keyslot manager
> + * @ksm: The keyslot_manager to initialize.
> + * @dev: Device for runtime power management (NULL if none)
> + * @num_slots: The number of key slots to manage.
> + *
> + * Allocate memory for keyslots and initialize a keyslot manager. Called by
> + * e.g. storage drivers to set up a keyslot manager in their request_queue.
> + *
> + * Return: 0 on success, or else a negative error code.
> + */
> +int blk_ksm_init(struct blk_keyslot_manager *ksm, unsigned int num_slots)

The @dev parameter was removed, so its kerneldoc should be too.

One tip, you can check the validity of kerneldoc comments by running:

$ ./scripts/kernel-doc -v -none block/keyslot-manager.c
block/keyslot-manager.c:67: info: Scanning doc for blk_ksm_init
block/keyslot-manager.c:78: warning: Excess function parameter 'dev' description in 'blk_ksm_init'
block/keyslot-manager.c:180: info: Scanning doc for blk_ksm_get_slot_for_key
block/keyslot-manager.c:198: warning: Function parameter or member 'slot_ptr' not described in 'blk_ksm_get_slot_for_key'
block/keyslot-manager.c:198: warning: Excess function parameter 'keyslot' description in 'blk_ksm_get_slot_for_key'
block/keyslot-manager.c:258: info: Scanning doc for blk_ksm_put_slot
block/keyslot-manager.c:282: info: Scanning doc for blk_ksm_crypto_key_supported
block/keyslot-manager.c:303: info: Scanning doc for blk_ksm_evict_key
block/keyslot-manager.c:343: info: Scanning doc for blk_ksm_reprogram_all_keys
3 warnings

> +/**
> + * blk_ksm_get_slot_for_key() - Program a key into a keyslot.
> + * @ksm: The keyslot manager to program the key into.
> + * @key: Pointer to the key object to program, including the raw key, crypto
> + *	 mode, and data unit size.
> + * @keyslot: A pointer to return the pointer of the allocated keyslot.
> + *
> + * Get a keyslot that's been programmed with the specified key.  If one already
> + * exists, return it with incremented refcount.  Otherwise, wait for a keyslot
> + * to become idle and program it.
> + *
> + * Context: Process context. Takes and releases ksm->lock.
> + * Return: BLK_STATUS_OK on success (and keyslot is set to the pointer of the
> + *	   allocated keyslot), or some other blk_status_t otherwise (and
> + *	   keyslot is set to NULL).
> + */

It's actually "BLK_STS_OK", not "BLK_STATUS_OK".

> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index f629d40c645cd..27d460d0a8508 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -43,6 +43,7 @@ struct pr_ops;
>  struct rq_qos;
>  struct blk_queue_stats;
>  struct blk_stat_callback;
> +struct blk_keyslot_manager;
>  
>  #define BLKDEV_MIN_RQ	4
>  #define BLKDEV_MAX_RQ	128	/* Default maximum */
> @@ -474,6 +475,11 @@ struct request_queue {
>  	unsigned int		dma_pad_mask;
>  	unsigned int		dma_alignment;
>  
> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
> +	/* Inline crypto capabilities */
> +	struct blk_keyslot_manager *ksm;
> +#endif

I do still wonder whether the concept of inline crypto support should be more
separated from keyslot management, to be better prepared for device-mapper
passthrough support and for hardware that accepts keys directly.  (Such hardware
exists, though I'm not sure support for it will be upstreamed.)  For example,
the crypto capabilities could be stored in a 'struct blk_crypto_capabilities'
rather than in 'struct blk_keyslot_manager', and the latter could be optional.

What you have now is fine for the functionality in the current patchset though,
so I'm not really complaining.  Just something to think about.

- Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

  reply index

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-26  3:06 [f2fs-dev] [PATCH v9 00/11] Inline Encryption Support Satya Tangirala via Linux-f2fs-devel
2020-03-26  3:06 ` [f2fs-dev] [PATCH v9 01/11] block: Keyslot Manager for Inline Encryption Satya Tangirala via Linux-f2fs-devel
2020-03-26  6:22   ` Eric Biggers [this message]
2020-03-27 17:00     ` Christoph Hellwig
2020-03-26  3:06 ` [f2fs-dev] [PATCH v9 02/11] block: Inline encryption support for blk-mq Satya Tangirala via Linux-f2fs-devel
2020-03-26 20:05   ` Eric Biggers
2020-03-27 17:05     ` Christoph Hellwig
2020-03-26  3:06 ` [f2fs-dev] [PATCH v9 03/11] block: Make blk-integrity preclude hardware inline encryption Satya Tangirala via Linux-f2fs-devel
2020-03-26  3:06 ` [f2fs-dev] [PATCH v9 04/11] block: blk-crypto-fallback for Inline Encryption Satya Tangirala via Linux-f2fs-devel
2020-03-26 20:28   ` Eric Biggers
2020-03-26  3:06 ` [f2fs-dev] [PATCH v9 05/11] scsi: ufs: UFS driver v2.1 spec crypto additions Satya Tangirala via Linux-f2fs-devel
2020-03-26  3:06 ` [f2fs-dev] [PATCH v9 06/11] scsi: ufs: UFS crypto API Satya Tangirala via Linux-f2fs-devel
2020-03-26  5:07   ` Eric Biggers
2020-03-26  3:06 ` [f2fs-dev] [PATCH v9 07/11] scsi: ufs: Add inline encryption support to UFS Satya Tangirala via Linux-f2fs-devel
2020-03-26  5:09   ` Eric Biggers
2020-03-26  3:06 ` [f2fs-dev] [PATCH v9 08/11] fs: introduce SB_INLINECRYPT Satya Tangirala via Linux-f2fs-devel
2020-03-26  5:56   ` Eric Biggers
2020-03-26  3:07 ` [f2fs-dev] [PATCH v9 09/11] fscrypt: add inline encryption support Satya Tangirala via Linux-f2fs-devel
2020-03-26  5:45   ` Eric Biggers
2020-03-26  3:07 ` [f2fs-dev] [PATCH v9 10/11] f2fs: " Satya Tangirala via Linux-f2fs-devel
2020-03-26  3:07 ` [f2fs-dev] [PATCH v9 11/11] ext4: " Satya Tangirala via Linux-f2fs-devel
2020-03-26  3:32 ` [f2fs-dev] [PATCH v9 00/11] Inline Encryption Support Eric Biggers

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200326062213.GF858@sol.localdomain \
    --to=ebiggers@kernel.org \
    --cc=bmuthuku@qti.qualcomm.com \
    --cc=boojin.kim@samsung.com \
    --cc=kuohong.wang@mediatek.com \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-f2fs-devel@lists.sourceforge.net \
    --cc=linux-fscrypt@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=satyat@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-f2fs-devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-f2fs-devel/0 linux-f2fs-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-f2fs-devel linux-f2fs-devel/ https://lore.kernel.org/linux-f2fs-devel \
		linux-f2fs-devel@lists.sourceforge.net
	public-inbox-index linux-f2fs-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/net.sourceforge.lists.linux-f2fs-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git