Message-ID: <624296dc-69d6-4bdd-bed1-ffcb747fb96d@kernel.org>
Date: Tue, 29 Jul 2025 17:07:54 +0900
From: Damien Le Moal <dlemoal@...nel.org>
To: Wilfred Mallawa <wilfred.opensource@...il.com>, alistair.francis@....com,
 chuck.lever@...cle.com, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, pabeni@...hat.com, horms@...nel.org,
 donald.hunter@...il.com, corbet@....net, kbusch@...nel.org, axboe@...nel.dk,
 hch@....de, sagi@...mberg.me, kch@...dia.com, borisp@...dia.com,
 john.fastabend@...il.com, jlayton@...nel.org, neil@...wn.name,
 okorniev@...hat.com, Dai.Ngo@...cle.com, tom@...pey.com, trondmy@...nel.org,
 anna@...nel.org, kernel-tls-handshake@...ts.linux.dev, netdev@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
 linux-nvme@...ts.infradead.org, linux-nfs@...r.kernel.org,
 Wilfred Mallawa <wilfred.mallawa@....com>
Subject: Re: [RFC 1/4] net/handshake: get negotiated tls record size limit

On 7/29/25 11:41, Wilfred Mallawa wrote:
> From: Wilfred Mallawa <wilfred.mallawa@....com>
> 
> During a handshake, an endpoint may specify a maximum record size limit.
> Currently, this limit is not visble to the kernel particularly in the case
> where userspace handles the handshake (tlshd/gnutls). This patch adds
> support for retrieving the record size limit.
> 
> This is the first step in ensuring that the kernel can respect the record
> size limit imposed by the endpoint.
> 
> Signed-off-by: Wilfred Mallawa <wilfred.mallawa@....com>
> ---
>  Documentation/netlink/specs/handshake.yaml |  3 +++
>  Documentation/networking/tls-handshake.rst |  8 +++++++-
>  drivers/nvme/host/tcp.c                    |  3 ++-
>  drivers/nvme/target/tcp.c                  |  3 ++-
>  include/net/handshake.h                    |  4 +++-
>  include/uapi/linux/handshake.h             |  1 +
>  net/handshake/genl.c                       |  5 +++--
>  net/handshake/tlshd.c                      | 15 +++++++++++++--
>  net/sunrpc/svcsock.c                       |  4 +++-
>  net/sunrpc/xprtsock.c                      |  4 +++-
>  10 files changed, 40 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/netlink/specs/handshake.yaml b/Documentation/netlink/specs/handshake.yaml
> index b934cc513e3d..35d5eb91a3f9 100644
> --- a/Documentation/netlink/specs/handshake.yaml
> +++ b/Documentation/netlink/specs/handshake.yaml
> @@ -84,6 +84,9 @@ attribute-sets:
>          name: remote-auth
>          type: u32
>          multi-attr: true
> +      -
> +        name: record-size-limit
> +        type: u32
>  
>  operations:
>    list:
> diff --git a/Documentation/networking/tls-handshake.rst b/Documentation/networking/tls-handshake.rst
> index 6f5ea1646a47..cd984a137779 100644
> --- a/Documentation/networking/tls-handshake.rst
> +++ b/Documentation/networking/tls-handshake.rst
> @@ -169,7 +169,8 @@ The synopsis of this function is:
>  .. code-block:: c
>  
>    typedef void	(*tls_done_func_t)(void *data, int status,
> -                                   key_serial_t peerid);
> +                                   key_serial_t peerid,
> +                                   size_t tls_record_size_limit);
>  
>  The consumer provides a cookie in the @ta_data field of the
>  tls_handshake_args structure that is returned in the @data parameter of
> @@ -200,6 +201,11 @@ The @peerid parameter contains the serial number of a key containing the
>  remote peer's identity or the value TLS_NO_PEERID if the session is not
>  authenticated.
>  
> +The @tls_record_size_limit parameter, if non-zero, exposes the tls max
> +record size advertised by the endpoint. Record size must not exceed this amount.
> +A negative value shall indicate that the endpoint did not advertise the
> +maximum record size limit.

size_t cannot be negative... Did you mean:
"A value of 0 (TLS_NO_RECORD_SIZE_LIMIT)..."

Also note that even if the endpoint does not advertize a record sie limit, we
still have one (16K was it ?). So I think that the name TLS_NO_RECORD_SIZE_LIMIT
is a little misleading.

> +
>  A best practice is to close and destroy the socket immediately if the
>  handshake failed.

[...]

> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index e1c85123b445..2014d906ff06 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -417,13 +417,15 @@ static void svc_tcp_kill_temp_xprt(struct svc_xprt *xprt)
>   * @data: address of xprt to wake
>   * @status: status of handshake
>   * @peerid: serial number of key containing the remote peer's identity
> + * @tls_record_size_limit: Max tls_record_size_limit of the endpoint

Please make a proper sentence to describe tls_record_size_limit instead of
repeating that argument name.

>   *
>   * If a security policy is specified as an export option, we don't
>   * have a specific export here to check. So we set a "TLS session
>   * is present" flag on the xprt and let an upper layer enforce local
>   * security policy.
>   */
> -static void svc_tcp_handshake_done(void *data, int status, key_serial_t peerid)
> +static void svc_tcp_handshake_done(void *data, int status, key_serial_t peerid,
> +				   size_t tls_record_size_limit)
>  {
>  	struct svc_xprt *xprt = data;
>  	struct svc_sock *svsk = container_of(xprt, struct svc_sock, sk_xprt);
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index 04ff66758fc3..509aa6269b0a 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -2569,9 +2569,11 @@ static int xs_tcp_tls_finish_connecting(struct rpc_xprt *lower_xprt,
>   * @data: address of xprt to wake
>   * @status: status of handshake
>   * @peerid: serial number of key containing the remote's identity
> + * @tls_record_size_limit: Max tls_record_size_limit of the endpoint

Same here.

>   *
>   */
> -static void xs_tls_handshake_done(void *data, int status, key_serial_t peerid)
> +static void xs_tls_handshake_done(void *data, int status, key_serial_t peerid,
> +				  size_t tls_record_size_limit)
>  {
>  	struct rpc_xprt *lower_xprt = data;
>  	struct sock_xprt *lower_transport =


-- 
Damien Le Moal
Western Digital Research

Powered by blists - more mailing lists