Re: [PATCH 08/16] nvme-tcp: enable TLS handshake upcall

[Hannes Reinecke <hare@suse.de>]

On 8/9/23 11:47, Sagi Grimberg wrote:
> 
>> @@ -1802,9 +1922,17 @@ static int nvme_tcp_alloc_admin_queue(struct 
>> nvme_ctrl *ctrl)
>>   static int __nvme_tcp_alloc_io_queues(struct nvme_ctrl *ctrl)
>>   {
>>       int i, ret;
>> +    key_serial_t pskid = 0;
>> +    if (ctrl->opts->tls) {
>> +        if (!ctrl->tls_key) {
>> +            dev_err(ctrl->device, "no PSK negotiated\n");
>> +            return -ENOKEY;
> 
> This is really unexpected. its not that no PSK negotiated, something
> went awfully wrong if the psk wasn't negotiated and we still end up
> being here... Or there is a flow that can make this happen?
> 
Yeah, it _is_ unexpected.
But we're having two conditions here (->tls and ->tls_key), and we need 
to check for both to be true.
Although, truth to be said, we might get away with just checking for 
->tls_key; I'll check.

>> +        }
>> +        pskid = key_serial(ctrl->tls_key);
>> +    }
> 
> newline missing.
> 
> key_serial can operate on null.
> 
>>       for (i = 1; i < ctrl->queue_count; i++) {
>> -        ret = nvme_tcp_alloc_queue(ctrl, i);
>> +        ret = nvme_tcp_alloc_queue(ctrl, i, pskid);
>>           if (ret)
>>               goto out_free_queues;
>>       }
> 
> Perhaps do:
> -- 
> @@ -1793,7 +1793,7 @@ static int __nvme_tcp_alloc_io_queues(struct 
> nvme_ctrl *ctrl)
>          int i, ret;
> 
>          for (i = 1; i < ctrl->queue_count; i++) {
> -               ret = nvme_tcp_alloc_queue(ctrl, i);
> +               ret = nvme_tcp_alloc_queue(ctrl, i, 
> key_serial(ctrl->tls_key));
>                  if (ret)
>                          goto out_free_queues;
>          }
> @@ -1812,6 +1812,9 @@ static int nvme_tcp_alloc_io_queues(struct 
> nvme_ctrl *ctrl)
>          unsigned int nr_io_queues;
>          int ret;
> 
> +       if (ctrl->opts->tls && !ctrl->tls_key)
> +               return -ENOKEY;
> +
>          nr_io_queues = nvmf_nr_io_queues(ctrl->opts);
>          ret = nvme_set_queue_count(ctrl, &nr_io_queues);
>          if (ret)
> -- 
> 
Yeah, might be simpler. I'll give it a go.

Cheers,

Hannes