[PATCH 15/16] nvmet-tcp: enable TLS handshake upcall

[Hannes Reinecke]

On 8/9/23 14:54, Sagi Grimberg wrote:
> 
> 
> On 8/9/23 15:51, Sagi Grimberg wrote:
>>
>>>>> @@ -1621,6 +1642,75 @@ static int nvmet_tcp_set_queue_sock(struct 
>>>>> nvmet_tcp_queue *queue)
>>>>>       return ret;
>>>>>   }
>>>>> +#ifdef CONFIG_NVME_TARGET_TCP_TLS
>>>>> +static void nvmet_tcp_tls_handshake_done(void *data, int status,
>>>>> +                     key_serial_t peerid)
>>>>> +{
>>>>> +    struct nvmet_tcp_queue *queue = data;
>>>>> +
>>>>> +    pr_debug("queue %d: TLS handshake done, key %x, status %d\n",
>>>>> +         queue->idx, peerid, status);
>>>>> +    spin_lock_irq(&queue->state_lock);
>>>>> +    if (queue->state != NVMET_TCP_Q_TLS_HANDSHAKE) {
>>>>> +        pr_warn("queue %d: TLS handshake already completed\n",
>>>>> +            queue->idx);
>>>>> +        spin_unlock_irq(&queue->state_lock);
>>>>> +        return;
>>>>> +    }
>>>>> +    if (!status)
>>>>> +        queue->tls_pskid = peerid;
>>>>> +    queue->state = NVMET_TCP_Q_CONNECTING;
>>>>> +    spin_unlock_irq(&queue->state_lock);
>>>>> +
>>>>> +    cancel_delayed_work_sync(&queue->tls_handshake_work);
>>>>> +    if (status) {
>>>>
>>>> I think that after this call, you cannot reference anything
>>>> in queue as it may have been released. Or I'm missing something?
>>>>
>>> I guess I can, as this code is gated by the state change above.
>>> Once we are here the state has been updated, so the timeout work will
>>> short circuit and not delete the queue.
>>
>> But who guarantees that queue->state_lock is a valid dereference?
> 
> The lock acquired in the first line of the function...
> 
> I think we need a reference count here...

Or use:

static void nvmet_tcp_schedule_release_queue(struct nvmet_tcp_queue *queue)
{
         spin_lock_irq(&queue->state_lock);
         if (queue->state != NVMET_TCP_Q_TLS_HANDSHAKE &&
             queue->state != NVMET_TCP_Q_DISCONNECTING) {
                 queue->state = NVMET_TCP_Q_DISCONNECTING;
                 queue_work(nvmet_wq, &queue->release_work);
         }
         spin_unlock_irq(&queue->state_lock);
}

Such that release_queue will be ignored when a socket state change
happens while handshake is in progress.
Handshake will complete with done or timeout, both of which will
call schedule_release_queue() (with the corrrect state) on error.

Cheers,

Hannes