[PATCH 16/16] nvmet-tcp: control messages for recvmsg()

[Hannes Reinecke]

On 8/9/23 13:30, Sagi Grimberg wrote:
> 
> 
> On 8/8/23 19:53, Hannes Reinecke wrote:
>> kTLS requires control messages for recvmsg() to relay any out-of-band
>> TLS messages (eg TLS alerts) to the caller.
>>
>> Signed-off-by: Hannes Reinecke <hare at suse.de>
>> ---
>>   drivers/nvme/target/tcp.c | 83 +++++++++++++++++++++++++++++++++------
>>   1 file changed, 72 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
>> index 75e4cb4c2f29..e1aea52e58da 100644
>> --- a/drivers/nvme/target/tcp.c
>> +++ b/drivers/nvme/target/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/inet.h>
>>   #include <linux/llist.h>
>> @@ -70,6 +71,9 @@ device_param_cb(idle_poll_period_usecs, &set_param_ops,
>>   MODULE_PARM_DESC(idle_poll_period_usecs,
>>           "nvmet tcp io_work poll till idle time period in usecs: 
>> Default 0");
>> +#define nvmet_queue_is_tls(queue) \
>> +    ((queue)->port->nport->disc_addr.tsas.tcp.sectype == 
>> NVMF_TCP_SECTYPE_TLS13)
> 
> That is some steep dereferencing... I'd say just cache a flag
> in the queue itself, or in the port.
> 
Yeah, not happy with that one, either.
Will be adding a queue flag.

>> +
>>   #ifdef CONFIG_NVME_TARGET_TCP_TLS
>>   /*
>>    * TLS handshake timeout
>> @@ -118,6 +122,7 @@ struct nvmet_tcp_cmd {
>>       u32                pdu_len;
>>       u32                pdu_recv;
>>       int                sg_idx;
>> +    char                recv_cbuf[CMSG_LEN(sizeof(char))];
>>       struct msghdr            recv_msg;
>>       struct bio_vec            *iov;
>>       u32                flags;
>> @@ -1116,20 +1121,59 @@ static inline bool nvmet_tcp_pdu_valid(u8 type)
>>       return false;
>>   }
>> +static int nvmet_tcp_tls_record_ok(struct nvmet_tcp_queue *queue,
>> +        struct msghdr *msg, char *cbuf)
>> +{
>> +    struct cmsghdr *cmsg = (struct cmsghdr *)cbuf;
>> +    u8 ctype, level, description;
>> +    int ret = 0;
>> +
>> +    if (!nvmet_queue_is_tls(queue))
>> +        return 0;
>> +
>> +    ctype = tls_get_record_type(queue->sock->sk, cmsg);
>> +    switch (ctype) {
>> +    case 0:
>> +        break;
>> +    case TLS_RECORD_TYPE_DATA:
>> +        break;
>> +    case TLS_RECORD_TYPE_ALERT:
>> +        tls_alert_recv(queue->sock->sk, msg, &level, &description);
>> +        pr_err("TLS Alert level %u desc %u\n", level, description);
>> +        ret = (level == TLS_ALERT_LEVEL_FATAL) ?
>> +            -ENOTCONN : -EAGAIN;
>> +        break;
>> +    default:
>> +        /* discard this record type */
>> +        pr_err("TLS record %d unhandled\n", ctype);
>> +        ret = -EAGAIN;
>> +        break;
>> +    }
>> +    return ret;
>> +}
>> +
>>   static int nvmet_tcp_try_recv_pdu(struct nvmet_tcp_queue *queue)
>>   {
>>       struct nvme_tcp_hdr *hdr = &queue->pdu.cmd.hdr;
>> -    int len;
>> +    int len, ret;
>>       struct kvec iov;
>> +    char cbuf[CMSG_LEN(sizeof(char))] = {};
>>       struct msghdr msg = { .msg_flags = MSG_DONTWAIT };
>>   recv:
>>       iov.iov_base = (void *)&queue->pdu + queue->offset;
>>       iov.iov_len = queue->left;
>> +    if (nvmet_queue_is_tls(queue)) {
>> +        msg.msg_control = cbuf;
>> +        msg.msg_controllen = sizeof(cbuf);
>> +    }
>>       len = kernel_recvmsg(queue->sock, &msg, &iov, 1,
>>               iov.iov_len, msg.msg_flags);
>>       if (unlikely(len < 0))
>>           return len;
>> +    ret = nvmet_tcp_tls_record_ok(queue, &msg, cbuf);
>> +    if (ret < 0)
>> +        return ret;
>>       queue->offset += len;
>>       queue->left -= len;
>> @@ -1182,16 +1226,21 @@ static void nvmet_tcp_prep_recv_ddgst(struct 
>> nvmet_tcp_cmd *cmd)
>>   static int nvmet_tcp_try_recv_data(struct nvmet_tcp_queue *queue)
>>   {
>>       struct nvmet_tcp_cmd  *cmd = queue->cmd;
>> -    int ret;
>> +    int len, ret;
>>       while (msg_data_left(&cmd->recv_msg)) {
>> -        ret = sock_recvmsg(cmd->queue->sock, &cmd->recv_msg,
>> +        len = sock_recvmsg(cmd->queue->sock, &cmd->recv_msg,
>>               cmd->recv_msg.msg_flags);
>> -        if (ret <= 0)
>> +        if (len <= 0)
>> +            return len;
>> +        ret = nvmet_tcp_tls_record_ok(cmd->queue,
>> +                          &cmd->recv_msg,
>> +                          cmd->recv_cbuf);
> 
> I think it will be better to condition on tls on the
> call sites, so it will be clearer that we are checking
> the tls record only for tls queues.

Ok, will be doing that.

Cheers,

Hannes