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

[Sagi Grimberg]

On 21/10/2024 14:00, Hannes Reinecke wrote:
> 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.

Sounds right.

> Otherwise I could just move the check into nvme_auth_revoke_tls_key()'
> and drop the 'revoke needed' call.

I prefer that we don't

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

won't it also be revoked in setup?

>
> 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?

Just a little documentation to why we like to revoke the key so many 
times ;)