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

[Hannes Reinecke]

On 1/28/25 10:11, Christoph Hellwig wrote:
> 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;
> 	}
> 
> ?
> 

Ok.

>> +	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.
> 
Will do.

>> +		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.
> 

Will be fixing it up.

>> +			/*
>> +			 * The generated PSK is stored in the
>> +			 * fabric options
>> +			 */
> 
> Missing period and not using up the line as well.
> 

Ditto.

>> +/*
>> + * 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? :)
> 
Possibly. Will be fixing it up.

>> --- 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.
> 
> 
Ok.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare at suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich