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

[Hannes Reinecke <hare@suse.de>]

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.

Cheers,

Hannes