[PATCH 11/18] nvme-tcp: enable TLS handshake upcall

[Hannes Reinecke]

On 4/18/23 12:32, Sagi Grimberg wrote:
> 
> 
> On 4/18/23 13:28, Hannes Reinecke wrote:
>> On 4/18/23 12:12, Sagi Grimberg wrote:
>>>
>>>>>   --- a/drivers/nvme/host/fabrics.c
>>>>>> +++ b/drivers/nvme/host/fabrics.c
>>>>>> @@ -609,6 +609,7 @@ static const match_table_t opt_tokens = {
>>>>>>       { NVMF_OPT_DISCOVERY,        "discovery"        },
>>>>>>       { NVMF_OPT_DHCHAP_SECRET,    "dhchap_secret=%s"    },
>>>>>>       { NVMF_OPT_DHCHAP_CTRL_SECRET,    "dhchap_ctrl_secret=%s"    },
>>>>>> +    { NVMF_OPT_TLS,            "tls"            },
>>>>>>       { NVMF_OPT_ERR,            NULL            }
>>>>>>   };
>>>>>
>>>>> Coming back to your previous discussion about this. 'tls' is always 
>>>>> announced
>>>>> but not always supported. This renders the unsupported option 
>>>>> filtering added to
>>>>> libnvme useles:
>>>>>
>>>>>    https://github.com/linux-nvme/libnvme/issues/612
>>>>>
>>>>> Let's assume we go ahead with this patch. The user set explicit the 
>>>>> tls option
>>>>> via 'nvme connect ... --tls' and the kernel will return EINVAL, but 
>>>>> userland
>>>>> can't really know what it was and drop --tls and retry again (or 
>>>>> print some
>>>>> useful information). It could be any other parameter provided by 
>>>>> the user.
>>>>>
>>>>> How is userland supposed to do any feature detection?
>>>>>
>>>> And that is a good question.
>>>> I've just removed most of the #ifdef annotations on request from Sagi.
>>>> If we were going with your suggestion I would need to bring them back.
>>>>
>>>> So, Sagi: What should we do?
>>>> Re-clutter and allow nvme-cli to figure out if TLS is supported?
>>>> And have it always enabled and invent other means for nvme-cli?
>>>
>>> I don't understand the issue.
>>>
>>> userspace already has a way to know if a feature is supported by reading
>>> the misc device.
>>>
>> Yes. But the output there is generated by evaluating the fields from 
>> the fabrics 'opts' structure.
>>
>> So if the 'tls' option is present it assumes that TLS is supported.
>> With my current patchset the _option_ is always present, but the 
>> evaluation of the option once set by nvme-cli might return -EINVAL if 
>> it's not compiled in.
>>
>> The request from Daniel is to have the compile option for the fabrics 
>> 'tls' option, too, such that it doesn't show up with the misc device.
>> IE revert the changes which you requested to convert all the
>> '#ifdef CONFIG_NVME_TCP_TLS' conditionals.
> 
> Thanks for explaining.
> 
> Then we can add the ifdef on the opts statement alone, with a comment.

Good, will do for the next round.

Cheers,

Hannes