[PATCH 13/14] nvmet-tcp: enable TLS handshake upcall

[Hannes Reinecke]

On 8/7/23 13:49, Sagi Grimberg wrote:
> 
>>>> @@ -389,6 +427,16 @@ static ssize_t nvmet_addr_tsas_store(struct 
>>>> config_item *item,
>>>>   found:
>>>>       nvmet_port_init_tsas_tcp(port, nvmet_addr_tsas_tcp[i].type);
>>>> +    if (nvmet_addr_tsas_tcp[i].type == NVMF_TCP_SECTYPE_TLS13) {
>>>> +        if (nvmet_port_treq(port) == NVMF_TREQ_NOT_SPECIFIED)
>>>> +            treq |= NVMF_TREQ_REQUIRED;
>>>> +        else
>>>> +            treq |= nvmet_port_treq(port);
>>>> +    } else {
>>>> +        /* Set to 'not specified' if TLS is not enabled */
>>>> +        treq |= NVMF_TREQ_NOT_SPECIFIED;
>>>> +    }
>>>> +    port->disc_addr.treq = treq;
>>>>       return count;
>>>>   }
>>>
>>> Can the treq/tsas be split from the actual nvmet-tcp upcall addition?
>>>
>> I guess it can; it's just that it doesn't make any sense if we don't 
>> have the TLS upcall implemented.
> 
> Still its mixing two parts together and makes the patch unnecessarily
> complicated.
> 
>>>> @@ -1285,12 +1302,12 @@ static int nvmet_tcp_try_recv(struct 
>>>> nvmet_tcp_queue *queue,
>>>>   static void nvmet_tcp_schedule_release_queue(struct 
>>>> nvmet_tcp_queue *queue)
>>>>   {
>>>> -    spin_lock(&queue->state_lock);
>>>> +    spin_lock_irq(&queue->state_lock);
>>>
>>> Where is this lock taken in irq context that needs disabling irq?
>>>
>> Let me check; might be that it got solved with the workqueue fixes 
>> which went in lately.
> 
> Can you explain what was the original issue for this change?
> 
The original issue was a lockdep warning when tearing down the target;
this got addressed by the patch 'nvmet-tcp: add new workqueue to 
suppress lockdep warning' from Guoqing Jiang.

>>>> +{
>>>> +    struct nvmet_tcp_queue *queue = data;
>>>> +
>>>> +    pr_debug("queue %d: TLS handshake done, key %x, status %d\n",
>>>> +         queue->idx, peerid, status);
>>>> +    if (!status) {
>>>> +        spin_lock_irq(&queue->state_lock);
>>>> +        queue->tls_pskid = peerid;
>>>> +        spin_unlock_irq(&queue->state_lock);
>>>> +    }
>>>> +    cancel_delayed_work_sync(&queue->tls_handshake_work);
>>>
>>> Hmm, the cancel_delayed_work_sync is scary.
>>>
>>> What happens if it ran and already scheduled a release (which
>>> already ran and completed)?
>>>
>> Well, we need to stop the timeout at some point; granted, for a 
>> failure we can just flush the workqueue, but in the success case we 
>> need terminate the workqueue.
>> And nvmet_tcp_schedule_release_queue() already checks for the queue 
>> state, so we should be (reasonably) safe.
> 
> Still it looks error prone (although granted, I didn't spot a particular
> race where this breaks).
> 
> I wish the handshake code could have given us a timeout functionality
> so we don't need to deal with it...
> 
> In any event, preferably the synchronization does not require to
> synchronize an active work element on the workqueue.
> 
With the timeout code we always will have two threads; the handshake 
itself and the timeout. So when handshake completes the timeout will
be running, and we will have to terminate it somehow. I really can't
see how we could avoid that.
But I do get your point; will be adding an atomic variable to 
synchronize between these two threads.

>>>> +    pr_debug("queue %d: TLS ServerHello\n", queue->idx);
>>>> +    memset(&args, 0, sizeof(args));
>>>> +    args.ta_sock = queue->sock;
>>>> +    args.ta_done = nvmet_tcp_tls_handshake_done;
>>>> +    args.ta_data = queue;
>>>> +    args.ta_keyring = key_serial(queue->port->nport->keyring);
>>>> +    args.ta_timeout_ms = tls_handshake_timeout * 2 * 1024;
>>>
>>> * 2 * 1024 ? I didn't know we have 2048 ms in a second...
>>>
>> The '_ms' bit is just an indicator of the unit, not the value.
>> We don't have an 'args.ta_timeout_sec' ...
> 
> I expected to see tls_handshake_timeout * 1000, i.e. a trivial
> conversion from sec to ms.

Okay, will do that.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare at suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman