[PATCH 09/14] nvme-tcp: control message handling for recvmsg()

[Hannes Reinecke]

On 8/8/23 10:41, Sagi Grimberg wrote:
> 
> 
> On 8/8/23 09:39, Hannes Reinecke wrote:
>> On 8/7/23 10:22, Sagi Grimberg wrote:
>>>
>>>
>>> On 8/3/23 13:50, Hannes Reinecke wrote:
>>>> 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.
>>>>
>>>> Signed-off-by: Hannes Reinecke <hare at suse.de>
>>>> Reviewed-by: Sagi Grimberg <sagi at grimberg.me>
>>>> ---
>>>>   drivers/nvme/host/tcp.c | 18 +++++++++++++++++-
>>>>   1 file changed, 17 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
>>>> index 98400ad5f969..4d0e3de39c26 100644
>>>> --- a/drivers/nvme/host/tcp.c
>>>> +++ b/drivers/nvme/host/tcp.c
>>>> @@ -14,6 +14,7 @@
>>>>   #include <net/sock.h>
>>>>   #include <net/tcp.h>
>>>>   #include <net/tls.h>
>>>> +#include <net/tls_prot.h>
>>>>   #include <net/handshake.h>
>>>>   #include <linux/blk-mq.h>
>>>>   #include <crypto/hash.h>
>>>> @@ -1369,6 +1370,10 @@ static int nvme_tcp_init_connection(struct 
>>>> nvme_tcp_queue *queue)
>>>>   {
>>>>       struct nvme_tcp_icreq_pdu *icreq;
>>>>       struct nvme_tcp_icresp_pdu *icresp;
>>>> +#ifdef CONFIG_NVME_TCP_TLS
>>>> +    char cbuf[CMSG_LEN(sizeof(char))] = {};
>>>> +    u8 ctype;
>>>> +#endif
>>>>       struct msghdr msg = {};
>>>>       struct kvec iov;
>>>>       bool ctrl_hdgst, ctrl_ddgst;
>>>> @@ -1406,11 +1411,22 @@ static int nvme_tcp_init_connection(struct 
>>>> nvme_tcp_queue *queue)
>>>>       memset(&msg, 0, sizeof(msg));
>>>>       iov.iov_base = icresp;
>>>>       iov.iov_len = sizeof(*icresp);
>>>> +#ifdef CONFIG_NVME_TCP_TLS
>>>> +    msg.msg_control = cbuf;
>>>> +    msg.msg_controllen = sizeof(cbuf);
>>>> +#endif
>>>>       ret = kernel_recvmsg(queue->sock, &msg, &iov, 1,
>>>>               iov.iov_len, msg.msg_flags);
>>>>       if (ret < 0)
>>>>           goto free_icresp;
>>>
>>> Is the cmsg passed unconditionally? meaning this does not
>>> impact non-tls?
>>>
>>> If so, why is the ifdef needed?
>>>
>> Passing in a cmsg (and a controllen) always enables the use of
>> control messages, irrespective of the type.
>> As we never enabled control messages prior to this we would
>> change the implementation when TLS is not enabled; who knows,
>> someone might have been sending us control messages all along,
>> but we never checked. So I thought it prudent to only enable
>> it for TLS where we know that we might be getting control messages.
> 
> But you are doing this only based on the compile-time config option, not
> based on a runtime setting. I am not sure I follow your logic.

For TLS I actually check the cmsg type, so even if we were to receive 
other control messages they would be caught.
If I were to enable control messages in general I would need to check 
for control messages on every packet, which may well impact performance.
Do we want that?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare at suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Frankenstr. 146, 90461 Nürnberg
Managing Directors: I. Totev, A. Myers, A. McDonald, M. B. Moerman
(HRB 36809, AG Nürnberg)