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

[Sagi Grimberg]

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.