Re: [PATCH 11/18] nvme-tcp: control message handling for recvmsg()

From: Sagi Grimberg <sagi@grimberg.me>
To: Hannes Reinecke <hare@suse.de>, Christoph Hellwig <hch@lst.de>,
	"boris.pismenny@gmail.com" <boris.pismenny@gmail.com>
Cc: Keith Busch <kbusch@kernel.org>,
	linux-nvme@lists.infradead.org, Chuck Lever <cel@kernel.org>,
	kernel-tls-handshake@lists.linux.dev
Subject: Re: [PATCH 11/18] nvme-tcp: control message handling for recvmsg()
Date: Wed, 22 Mar 2023 14:29:05 +0200	[thread overview]
Message-ID: <4963a908-8829-5894-a815-9b27fd0f9dd7@grimberg.me> (raw)
In-Reply-To: <0a8061ca-9c44-cae5-aab4-fd3222f372bd@suse.de>

>>>>> kTLS is sending TLS ALERT messages as control messages for recvmsg().
>>>>> As we can't do anything sensible with it just abort the connection
>>>>> and let the userspace agent to a re-negotiation.
>>>>
>>>> Is this a problem if we do end up adding read_sock to tls?
>>>> Although I do see that the tls code does manage this in the
>>>> sk_buff control buffer, so I assume there is access to this info.
>>>>
>>>> CC'ing Boris here as well.
>>>>
>>> Yeah, that was the other reason; cmsg aka TLS alerts are only 
>>> available for recvmsg().
>>>
>>> However, for TLS 1.3 the only TLS alert which does not trigger a 
>>> connection reset would the the ominous 'new session ticket' alert.
>>> But during TLS handshake development we already decided to _disable_
>>> session tickets as they are pretty meaningless for our us.
>>> Consequently all TLS alerts will trigger a connection reset, and 
>>> realistically we don't _need_ to know which alert type has triggered
>>> the reset.
>>>
>>> So 'read_sock()' could be implemented by closing the connection on
>>> any TLS alert, and not giving us access to any of the alerts via
>>> control messages. If that makes life easier ...
>>
>> Most likely read_sock() is the wrong place to ignore alerts. Probably
>> the better way is to do this in nvme, although its a bit harder.
> 
> But that's just the point; you don't have OOB information for 
> read_sock(). So I wouldn't know how you could pass the TLS alert 
> messages.

tls_sw populates cmsg from skb control block, and accesses it from the
skb with tls_msg(skb)...

> Hence my idea of converting them into a connection reset for 
> read_sock().

Probably its the wrong layer for this decision.

> As the whole idea of using ktls is that you do _not_ have to handle TLS 
> details, it's questionable what exactly one could do here except from 
> resetting the connection...

Well, in theory, you could have passed it to userspace along with the
sockfd to handle it for you. But I'm definitely not suggesting that we
do that. But at least the connection reset should be decided above
read_sock.