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

[Hannes Reinecke <hare@suse.de>]

On 3/22/23 16:42, Sagi Grimberg wrote:
> 
>>>> The 'data_ready' call might happen at any time after the 'accept' 
>>>> call and us calling into userspace.
>>>> In particular we have this flow of control:
>>>>
>>>> 1. Kernel: accept()
>>>> 2. Kernel: handshake request
>>>> 3. Userspace: read data from socket
>>>> 4. Userspace: tls handshake
>>>> 5. Kernel: handshake complete
>>>>
>>>> If the 'data_ready' event occurs between 1. and 3. userspace 
>>>> wouldn't know that something has happened, and will be sitting there 
>>>> waiting for data which is already present.
>>>
>>> Umm, doesn't userspace read from the socket once we trigger the upcall?
>>> it should. But I still don't understand what is the difference between
>>> us waiking up userspace, from the default sock doing the same?
>>>
>> No, it doesn't (or, rather, can't).
>> After processing 'accept()' (from the kernel code) data might already 
>> be present (after all, why would we get an 'accept' call otherwise?).
>> But the daemon has not been started up (yet); that's only done in
>> step 3). But 'data_ready' has already been called, so by the time 
>> userland is able to do a 'read()' on the socket it won't be seeing 
>> anything.
> 
> Not sure I understand. if data exists, userspace will read from the
> socket and get data, whenever that is. >
That's what I thought, too.
But then the userspace daemon just sat there doing nothing.

>> To be precise: that's the reasoning I've come up with.
>> Might be completely wrong, or beside the point.
>> But what I _do_ know is that without this call userspace would
>> _not_ see any data, and would happily sit there until we close the 
>> socket due to a timeout (which also why I put in the timeout
>> module options ...), and only _then_ see the data.
>> But with this call everything works. So there.
> 
> I may be missing something, and maybe its because I haven't looked at
> the userspace side yet. But if it calls recvmsg, it should either
> consume data, or it should block until it arrives.
> 
> I'm finding it difficult to understand why emulating almost 100% the
> default socket .data_ready is what is needed. But I'll try to understand
> better.
> 
Oh, I don't claim to have a full understanding, either.
I had initially assumed that it would work 'out of the box', without us 
having to specify a callback.
Turns out that it doesn't.
Maybe it's enough to call sk->sk_data_ready() after the upcall has been 
started. But we sure have to do _something_.

If you have other solutions I'm all ears...

> 
> 
>>>> As outlined in the response to the nvme-tcp upcall, on the server 
>>>> side we _have_ to allow for non-TLS connections (eg. for discovery).
>>>
>>> But in essence what you are doing is that you allow normal connections
>>> for a secured port...
>>>
>> Correct.
> 
> That is not what the feature is supposed to do.
> 
Well ... it's a difference in interpretation.
The NVMe TCP spec just says '... describes whether TLS is supported.'.
It does not say 'required'.
But there currently is no way how the server could describe 'TLS 
supported' vs 'TLS required'.

>>> btw, why not enforce a psk for the discovery controller (on this port)
>>> as well? for secured ports? No one said that we must accept a
>>> non-secured discovery connecting host on a secured port.
>>>
>> Because we can't have a PSK for standard discovery.
>> Every discovery controller has the canonical discovery NQN, so every
>> PSK will have the same subsystem NQN, and you will have to provide the 
>> _same_ PSK to every attached storage system.
> 
> Right, because they have the same NQN. Sounds good, what is the problem?
> It is much better than to have sectype be nothing more than a
> recommendation.
> 
> If sectype tls1.x is defined on a port then every connection to that
> port is a tls connection.
> 
See above. Only if we decide to give TSAS a 'TLS required' meaning.
With the this patchset it's a 'TLS supported' syntax.

>> If we had unique discovery controller NQNs everything would be good,
>> but as we don't I guess we have to support normal connections in 
>> addition to secured ones.
> 
> Its orthogonal. the well-known disc-nqn may be "weaker", but tls
> is still enforced. when we add unique disc-nqn, we can use different
> psks.
> 
Not only weaker, but _identical_ for each server.

>> As I said; maybe it's time to revisit the unique discovery NQN patches;
>> with those we should be able to separate the various use-case like
>> having a dedicated secured discovery port.
> 
> I don't see why this would be needed.
> 
We don't if we treat TSAS as 'TLS required', true.
But that's not what the spec says.
And there is also the dodgy statement in section 3.6.1.6:
 > If a host that supports TLS for NVMe/TCP receives a discovery
 > log entry indicating that the NVM subsystem uses NVMe/TCP and
 > does not support TLS, then the host should nonetheless
 > attempt to establish an NVMe/TCP connection that uses TLS.
 >
which really indicates that the TSAS field is a recommendation only.

But really, we shouldn't read too much into what the TSAS field says.
With the upcoming TPAR for TLS clarification all these different 
interpretations should be clarified.

The key question, however, will remain: Do we _want_ to support a 'TLS 
supported' mode for nvmet?
I'd say we should, to have compability with older (client) installations.
And I guess we have to anyway for secure concatenation
(as this _requires_ that we start off unencrypted).
So I'd suggest to leave it for now, have the code to support both, and 
wait for the TPAR to be ratified and then revisit this issue.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman