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

[Sagi Grimberg]

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.

> +
>   #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.