[PATCH 6/9] nvme-tcp: request secure channel concatenation

[Hannes Reinecke]

On 10/20/24 23:04, Sagi Grimberg wrote:
> 
> 
> 
> On 18/10/2024 9:33, 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.
>>
>> Signed-off-by: Hannes Reinecke <hare at kernel.org>
>> ---
>>   drivers/nvme/host/auth.c    | 108 +++++++++++++++++++++++++++++++++++-
>>   drivers/nvme/host/fabrics.c |  34 +++++++++++-
>>   drivers/nvme/host/fabrics.h |   3 +
>>   drivers/nvme/host/nvme.h    |   2 +
>>   drivers/nvme/host/sysfs.c   |   4 +-
>>   drivers/nvme/host/tcp.c     |  47 ++++++++++++++--
>>   include/linux/nvme.h        |   7 +++
>>   7 files changed, 191 insertions(+), 14 deletions(-)
>>
[ .. ]
>> @@ -2314,6 +2345,8 @@ static void nvme_tcp_error_recovery_work(struct 
>> work_struct *work)
>>                   struct nvme_tcp_ctrl, err_work);
>>       struct nvme_ctrl *ctrl = &tcp_ctrl->ctrl;
>> +    if (nvme_tcp_key_revoke_needed(ctrl))
>> +        nvme_auth_revoke_tls_key(ctrl);
> 
> Having this sprayed in various places in the code is really confusing.
> 
> Can you please add a small comment on each call-site? just for our 
> future selves
> reading this code?
> 
> Outside of that, patch looks good.
> 
Weelll ...
We need to reset the negotiated PSK exactly in three places:
- reset
- error recovery
- teardown
Much like we need to do for every other queue-related resource.

And in one of your previous reviews you stated that you do _not_
want to have 'nvme_auth_revoke_tls_key()' checking if the key
needs to be revoked, but rather have a check function.
Otherwise I could just move the check into nvme_auth_revoke_tls_key()'
and drop the 'revoke needed' call.

Furthermore we don't need to check if the key needs to be revoked
during teardown (answer will always be 'yes').

So I'm quite unsure what to do now ... document that we need
to release the key when doing a reset or error recovery?
Move the check back into nvme_auth_tls_revoke_key()?
Hmm?

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