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

Hannes Reinecke
2023-08-08T06:39:30Z

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@suse.de>
>> Reviewed-by: Sagi Grimberg <sagi@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.

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