> On Jul 18, 2022, at 3:53 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
> On Mon, 2022-06-06 at 10:51 -0400, Chuck Lever wrote:
>> kTLS sockets use cmsg to report decryption errors and the need
>> for session re-keying. An "application data" message contains a ULP
>> payload, and that is passed along to the RPC client. An "alert"
>> message triggers connection reset. Everything else is discarded.
>>
>> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
>> ---
>> include/net/tls.h | 2 ++
>> include/trace/events/sunrpc.h | 40 +++++++++++++++++++++++++++++++++
>> net/sunrpc/xprtsock.c | 49 +++++++++++++++++++++++++++++++++++++++--
>> 3 files changed, 89 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/net/tls.h b/include/net/tls.h
>> index 6b1bf46daa34..54bccb2e4014 100644
>> --- a/include/net/tls.h
>> +++ b/include/net/tls.h
>> @@ -71,6 +71,8 @@ static inline struct tlsh_sock *tlsh_sk(struct sock *sk)
>>
>> #define TLS_CRYPTO_INFO_READY(info) ((info)->cipher_type)
>>
>> +#define TLS_RECORD_TYPE_ALERT 0x15
>> +#define TLS_RECORD_TYPE_HANDSHAKE 0x16
>> #define TLS_RECORD_TYPE_DATA 0x17
>>
>> #define TLS_AAD_SPACE_SIZE 13
>> diff --git a/include/trace/events/sunrpc.h b/include/trace/events/sunrpc.h
>> index 986e135e314f..d7d07f3b850e 100644
>> --- a/include/trace/events/sunrpc.h
>> +++ b/include/trace/events/sunrpc.h
>> @@ -1319,6 +1319,46 @@ TRACE_EVENT(xs_data_ready,
>> TP_printk("peer=[%s]:%s", __get_str(addr), __get_str(port))
>> );
>>
>> +/*
>> + * From https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml
>> + *
>> + * Captured March 2022. Other values are unassigned or reserved.
>> + */
>> +#define rpc_show_tls_content_type(type) \
>> + __print_symbolic(type, \
>> + { 20, "change cipher spec" }, \
>> + { 21, "alert" }, \
>> + { 22, "handshake" }, \
>> + { 23, "application data" }, \
>> + { 24, "heartbeat" }, \
>> + { 25, "tls12_cid" }, \
>> + { 26, "ACK" })
>> +
>> +TRACE_EVENT(xs_tls_contenttype,
>> + TP_PROTO(
>> + const struct rpc_xprt *xprt,
>> + unsigned char ctype
>> + ),
>> +
>> + TP_ARGS(xprt, ctype),
>> +
>> + TP_STRUCT__entry(
>> + __string(addr, xprt->address_strings[RPC_DISPLAY_ADDR])
>> + __string(port, xprt->address_strings[RPC_DISPLAY_PORT])
>> + __field(unsigned long, ctype)
>> + ),
>> +
>> + TP_fast_assign(
>> + __assign_str(addr, xprt->address_strings[RPC_DISPLAY_ADDR]);
>> + __assign_str(port, xprt->address_strings[RPC_DISPLAY_PORT]);
>> + __entry->ctype = ctype;
>> + ),
>> +
>> + TP_printk("peer=[%s]:%s: %s", __get_str(addr), __get_str(port),
>> + rpc_show_tls_content_type(__entry->ctype)
>> + )
>> +);
>> +
>> TRACE_EVENT(xs_stream_read_data,
>> TP_PROTO(struct rpc_xprt *xprt, ssize_t err, size_t total),
>>
>> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
>> index 0a521aee0b2f..c73af8f1c3d4 100644
>> --- a/net/sunrpc/xprtsock.c
>> +++ b/net/sunrpc/xprtsock.c
>> @@ -47,6 +47,8 @@
>> #include <net/checksum.h>
>> #include <net/udp.h>
>> #include <net/tcp.h>
>> +#include <net/tls.h>
>> +
>> #include <linux/bvec.h>
>> #include <linux/highmem.h>
>> #include <linux/uio.h>
>> @@ -350,13 +352,56 @@ xs_alloc_sparse_pages(struct xdr_buf *buf, size_t want, gfp_t gfp)
>> return want;
>> }
>>
>> +static int
>> +xs_sock_process_cmsg(struct socket *sock, struct msghdr *msg,
>> + struct cmsghdr *cmsg, int ret)
>> +{
>> + if (cmsg->cmsg_level == SOL_TLS &&
>> + cmsg->cmsg_type == TLS_GET_RECORD_TYPE) {
>> + u8 content_type = *((u8 *)CMSG_DATA(cmsg));
>> +
>> + trace_xs_tls_contenttype(xprt_from_sock(sock->sk), content_type);
>> + switch (content_type) {
>> + case TLS_RECORD_TYPE_DATA:
>> + /* TLS sets EOR at the end of each application data
>> + * record, even though there might be more frames
>> + * waiting to be decrypted. */
>> + msg->msg_flags &= ~MSG_EOR;
>> + break;
>> + case TLS_RECORD_TYPE_ALERT:
>> + ret = -ENOTCONN;
>> + break;
>> + default:
>> + ret = -EAGAIN;
>> + }
>> + }
>> + return ret;
>> +}
>> +
>> +static int
>> +xs_sock_recv_cmsg(struct socket *sock, struct msghdr *msg, int flags)
>> +{
>> + union {
>> + struct cmsghdr cmsg;
>> + u8 buf[CMSG_SPACE(sizeof(u8))];
>> + } u;
>> + int ret;
>> +
>> + msg->msg_control = &u;
>> + msg->msg_controllen = sizeof(u);
>> + ret = sock_recvmsg(sock, msg, flags);
>> + if (msg->msg_controllen != sizeof(u))
>
> Do you also need to check for ret < 0 here? Can msg_controllen be
> trusted if sock_recvmsg returns an error?
This is not the most clear of APIs, and code I audited to help me
understand how it should work was inconsistent. Some just outright
does not work.
sock_recvmsg() has to explicitly modify the msg_controllen field
to indicate there is a control message. I think we are good here.
Also, no-one barked at this when it was posted on netdev.
>> + ret = xs_sock_process_cmsg(sock, msg, &u.cmsg, ret);
>> + return ret;
>> +}
>> +
>> static ssize_t
>> xs_sock_recvmsg(struct socket *sock, struct msghdr *msg, int flags, size_t seek)
>> {
>> ssize_t ret;
>> if (seek != 0)
>> iov_iter_advance(&msg->msg_iter, seek);
>> - ret = sock_recvmsg(sock, msg, flags);
>> + ret = xs_sock_recv_cmsg(sock, msg, flags);
>> return ret > 0 ? ret + seek : ret;
>> }
>>
>> @@ -382,7 +427,7 @@ xs_read_discard(struct socket *sock, struct msghdr *msg, int flags,
>> size_t count)
>> {
>> iov_iter_discard(&msg->msg_iter, READ, count);
>> - return sock_recvmsg(sock, msg, flags);
>> + return xs_sock_recv_cmsg(sock, msg, flags);
>> }
>>
>> #if ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE
>>
>>
>
> --
> Jeff Layton <jlayton@xxxxxxxxxx>
--
Chuck Lever
