Re: [PATCH 07/10] nvme-tcp: request secure channel concatenation

[Christoph Hellwig <hch@lst.de>]

On Wed, Jan 22, 2025 at 05:58:26PM +0100, Hannes Reinecke wrote:
> Add a fabrics option 'concat' to request secure channel concatenation.
> When secure channel concatenation is enabled a 'generated PSK' is inserted
> into the keyring such that it's available after reset.

That's a very sparse commit message.  What is the point of doing this?
What is the spec reference for the implementation?  What is the user
interface?  Why does this now always select NVME_KEYRING?

> +	if (!ctrl->opts->concat || chap->qid != 0)
> +		data->sc_c = NVME_AUTH_SECP_NOSC;
> +	else if (ctrl->opts->tls_key)
> +		data->sc_c = NVME_AUTH_SECP_REPLACETLSPSK;
> +	else
> +		data->sc_c = NVME_AUTH_SECP_NEWTLSPSK;

Took me a while to unwind this.  Why not make this a little easier as:

	if (ctrl->opts->concat && chap->qid == 0) {
		if (ctrl->opts->tls_key)
			data->sc_c = NVME_AUTH_SECP_REPLACETLSPSK;
		else
			data->sc_c = NVME_AUTH_SECP_NEWTLSPSK;
	} else {
		data->sc_c = NVME_AUTH_SECP_NOSC;
	}

?

> +	ret = nvme_auth_derive_tls_psk(chap->hash_id, psk, psk_len, digest, &tls_psk);

Overly long line.

> +	tls_key = nvme_tls_psk_refresh(ctrl->opts->keyring, ctrl->opts->host->nqn,

Same.  Not going to mention more of that, but please fix it up.

> +		if (ctrl->opts->concat &&
> +		    (ret = nvme_auth_secure_concat(ctrl, chap))) {

		if (ctrl->opts->concat) {
			ret = nvme_auth_secure_concat(ctrl, chap);
			if (ret) {
				...

> +	/*
> +	 * Only run authentication on the admin queue for
> 	 * secure concatenation
> +	 */

The two lines of comment can be folded in one, and it's missing a period
at the end of the sentence.

> +			/*
> +			 * The generated PSK is stored in the
> +			 * fabric options
> +			 */

Missing period and not using up the line as well.

> +/*
> + * The TLS key is set by secure concatenation after negotiation
> + * has been completed on the admin queue. We need to revoke the
> + * key when:
> + * - concatenation is enabled
> + *   (otherwise it's a static key set by the user)
> + * and
> + * - the generated key is present in ctrl->tls_key
> + *   (otherwise there's nothing to revoke)
> + * and
> + * - a valid PSK key ID has been set in ctrl->tls_pskid
> + *   (otherwise TLS negotiation has not run).
> + *
> + * We cannot always revoke the key, as we're calling
> + * nvme_tcp_alloc_admin_queue() twice during secure
> + * concatenation, once on a 'normal' connection to run
> + * the DH-HMAC-CHAP negotiation (which generates the key,
> + * so it _must not_ be set), and once after the negotiation
> + * (which uses the key, so it _must_ be set).
> + */

Another overly dense fomatting.  Are you trying to make up the
extra width used in the first patches? :)

> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -1746,6 +1746,13 @@ enum {
>  	NVME_AUTH_DHGROUP_INVALID	= 0xff,
>  };
>  
> +enum {
> +	NVME_AUTH_SECP_NOSC		= 0x00,
> +	NVME_AUTH_SECP_SC		= 0x01,
> +	NVME_AUTH_SECP_NEWTLSPSK	= 0x02,
> +	NVME_AUTH_SECP_REPLACETLSPSK	= 0x03,
> +};

Comments please to explain what fields this applies to.  Also we
usuall try to split protocol definition additions into separate
patches.