Re: [PATCH 13/17] nvme-tcp: reset after recovery for secure concatenation

[Sagi Grimberg <sagi@grimberg.me>]

On 09/04/2024 1:10, Hannes Reinecke wrote:
> On 4/8/24 23:23, Sagi Grimberg wrote:
>>
>>
>> On 08/04/2024 9:25, Hannes Reinecke wrote:
>>> On 4/7/24 23:49, Sagi Grimberg wrote:
>>>>
>>>>
>>>> On 18/03/2024 17:03, Hannes Reinecke wrote:
>>>>> From: Hannes Reinecke <hare@suse.de>
>>>>>
>>>>> With TP8018 a new key will be generated from the DH-HMAC-CHAP
>>>>> protocol after reset or recovery, but we need to start over
>>>>> to establish a new TLS connection with the new keys.
>>>>>
>>>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>>>> ---
>>>>>   drivers/nvme/host/tcp.c | 20 ++++++++++++++++++++
>>>>>   1 file changed, 20 insertions(+)
>>>>>
>>>>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
>>>>> index 94152ded123a..3811ee9cd040 100644
>>>>> --- a/drivers/nvme/host/tcp.c
>>>>> +++ b/drivers/nvme/host/tcp.c
>>>>> @@ -2219,6 +2219,22 @@ static void 
>>>>> nvme_tcp_reconnect_or_remove(struct nvme_ctrl *ctrl)
>>>>>       }
>>>>>   }
>>>>> +static bool nvme_tcp_reset_for_secure_concat(struct nvme_ctrl *ctrl)
>>>>> +{
>>>>> +    if (!ctrl->opts->concat)
>>>>> +        return false;
>>>>> +    /*
>>>>> +     * If a key has been generated and TLS has not been enabled
>>>>> +     * reset the queue to start TLS handshake.
>>>>> +     */
>>>>> +    if (ctrl->opts->tls_key && !ctrl->tls_key) {
>>>>> +        dev_info(ctrl->device, "Reset to enable TLS with 
>>>>> generated PSK\n");
>>>>> +        nvme_reset_ctrl(ctrl);
>>>>> +        return true;
>>>>> +    }
>>>>> +    return false;
>>>>> +}
>>>>> +
>>>>>   static void nvme_tcp_revoke_generated_tls_key(struct nvme_ctrl 
>>>>> *ctrl)
>>>>>   {
>>>>>       if (!ctrl->opts->concat)
>>>>> @@ -2321,6 +2337,9 @@ static void 
>>>>> nvme_tcp_reconnect_ctrl_work(struct work_struct *work)
>>>>>       if (nvme_tcp_setup_ctrl(ctrl, false))
>>>>>           goto requeue;
>>>>> +    if (nvme_tcp_reset_for_secure_concat(ctrl))
>>>>> +        return;
>>>>> +
>>>>>       dev_info(ctrl->device, "Successfully reconnected (%d 
>>>>> attempt)\n",
>>>>>               ctrl->nr_reconnects);
>>>>> @@ -2396,6 +2415,7 @@ static void nvme_reset_ctrl_work(struct 
>>>>> work_struct *work)
>>>>>       if (nvme_tcp_setup_ctrl(ctrl, false))
>>>>>           goto out_fail;
>>>>> +    nvme_tcp_reset_for_secure_concat(ctrl);
>>>>
>>>> This is really strange. I'd imagine that this would be needed only 
>>>> if the derived psk expired no?
>>>
>>> You are absolutely correct. But once you reset the connection the 
>>> derived PSK of the previous connection _is_ expired as DH-HMAC-CHAP
>>> generated a new one.
>>>
>>> Remember: for secure concatenation we _always_ have a two-step 
>>> connection setup. The initial connection runs for DH-HMAC-CHAP
>>> to generate the PSK, and then a connection reset to start over
>>> with TLS and the generated PSK.
>>> So upon reset we have to invalidate the generated PSK, reset the
>>> connection to run DH-HMAC-CHAP, and reset _again_ to start over
>>> with the new PSK.
>>
>> So what is the point of setting the derived psk with a timeout?
>>
>> Seems rather silly doing that _every_ reset/reconnect... But ok...
>
> Mandated by TP8018. Generated TLS PSK should be renewed after a
> certain time. Makes sense in general, but still a pain.

I understand that psk can expire, I don't understand why we need to 
reset every
single time. What if I'm doing the loop: while true; do nvme reset 
/dev/nvme0; done?

Will it do 2 resets every time?