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

[Sagi Grimberg]

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.