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

[Sagi Grimberg]

>>> 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.

> 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.



>>> 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.

>> 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.

> 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.

> 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.

> 
>>> And we have to start the daemon _before_ the first packet arrives,
>>
>> Not sure why that is.
>>
>>> but only the first packet will tell us what we should have done.
>>> So really we have to start the upcall and see what happens.
>>> The 'real' handling / differentiation between these two modes is done 
>>> with the 'peek pdu' patch later on.
>>
>> I am hoping we can kill it.
> 
> Sorry, no. Can't.
> Not without killing standard discovery.

I don't understand the dependency.