Re: [PATCH v3 1/2] tls: TLS 1.3 hardware offload support

[Jakub Kicinski <kuba@kernel.org>]

On Fri,  2 Jan 2026 11:47:07 -0700 Rishikesh Jethwani wrote:
> Add TLS 1.3 support to the kernel TLS hardware offload infrastructure,
> enabling hardware acceleration for TLS 1.3 connections on capable NICs.
> 
> This patch implements the critical differences between TLS 1.2 and TLS 1.3
> record formats for hardware offload:
> 
> TLS 1.2 record structure:
>   [Header (5)] + [Explicit IV (8)] + [Ciphertext] + [Tag (16)]
> 
> TLS 1.3 record structure:
>   [Header (5)] + [Ciphertext + ContentType (1)] + [Tag (16)]
> 
> Key changes:
> 1. Content type handling: In TLS 1.3, the content type byte is appended
>    to the plaintext before encryption and tag computation. This byte must
>    be encrypted along with the ciphertext to compute the correct
>    authentication tag. Modified tls_device_record_close() to append
>    the content type before the tag for TLS 1.3 records.
> 
> 2. Version validation: Both tls_set_device_offload() and
>    tls_set_device_offload_rx() now accept TLS_1_3_VERSION in addition
>    to TLS_1_2_VERSION.
> 
> 3. Pre-populate dummy_page with valid record types for memory
>    allocation failure fallback path.
> 
> Note: TLS 1.3 protocol parameters (aad_size, tail_size, prepend_size)
> are already handled by init_prot_info() in tls_sw.c.

I don't see you handling re-keying, which is supported in SW.

> Testing:
> Verified on Broadcom BCM957608 (Thor 2) and Mellanox ConnectX-6 Dx
> (Crypto Enabled) using ktls_test. Both TX and RX hardware offload working
> successfully with TLS 1.3 AES-GCM-128 and AES-GCM-256 cipher suites.

The kernel has come a long way in terms of HW testing since TLS was
added. We now require in-tree selftests for new capabilities. Some
relevant information here: https://github.com/linux-netdev/nipa/wiki

> The upstream Broadcom bnxt_en driver does not yet support kTLS offload.
> Testing was performed using the out-of-tree driver version
> bnxt_en-1.10.3-235.1.154.0, which works without modifications.

It's a bit odd to mention that you tested some out of tree code.
Glad it works, but I don't see the relevance upstream. Please drop
the mentions of Broadcom until the driver has TLS offload support added.

> Signed-off-by: Rishikesh Jethwani <rjethwani@purestorage.com>
> ---
>  net/tls/tls_device.c | 49 ++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 45 insertions(+), 4 deletions(-)
> 
> diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> index 82ea407e520a..f57e96862b1c 100644
> --- a/net/tls/tls_device.c
> +++ b/net/tls/tls_device.c
> @@ -319,6 +319,36 @@ static void tls_device_record_close(struct sock *sk,
>  	struct tls_prot_info *prot = &ctx->prot_info;
>  	struct page_frag dummy_tag_frag;
>  
> +	/* TLS 1.3: append content type byte before tag.
> +	 * Record structure: [Header (5)] + [Ciphertext + ContentType (1)] + [Tag (16)]
> +	 * The content type is encrypted with the ciphertext for authentication.
> +	 */
> +	if (prot->version == TLS_1_3_VERSION) {
> +		struct page_frag dummy_content_type_frag;
> +		struct page_frag *content_type_pfrag = pfrag;
> +
> +		/* Validate record type range */
> +		if (unlikely(record_type < TLS_RECORD_TYPE_CHANGE_CIPHER_SPEC ||
> +			     record_type > TLS_RECORD_TYPE_ACK)) {
> +			pr_err_once("tls_device: invalid record type %u\n",
> +				    record_type);
> +			return;

This check is really odd. Why is it relevant, and yet not relevant
enough to handle cleanly? On a quick look it appears that the user
can set whatever content type they want.

> +
> +		if (unlikely(pfrag->size - pfrag->offset < prot->tail_size) &&
> +		    !skb_page_frag_refill(prot->tail_size, pfrag, sk->sk_allocation)) {
> +			/* Out of memory: use pre-populated dummy_page */
> +			dummy_content_type_frag.page = dummy_page;
> +			dummy_content_type_frag.offset = record_type;
> +			content_type_pfrag = &dummy_content_type_frag;
> +		} else {
> +			/* Current pfrag has space or allocation succeeded - write content type */
> +			*(unsigned char *)(page_address(pfrag->page) + pfrag->offset) =
> +				record_type;

wrap at 80chars and please refactor this long line into something more
readable.

> +		}
> +		tls_append_frag(record, content_type_pfrag, prot->tail_size);
> +	}
> +
>  	/* append tag
>  	 * device will fill in the tag, we just need to append a placeholder
>  	 * use socket memory to improve coalescing (re-using a single buffer
> @@ -335,7 +365,7 @@ static void tls_device_record_close(struct sock *sk,
>  
>  	/* fill prepend */
>  	tls_fill_prepend(ctx, skb_frag_address(&record->frags[0]),
> -			 record->len - prot->overhead_size,
> +			 (record->len - prot->overhead_size) + prot->tail_size,
>  			 record_type);
>  }
>  
> @@ -1089,7 +1119,8 @@ int tls_set_device_offload(struct sock *sk)
>  	}
>  
>  	crypto_info = &ctx->crypto_send.info;
> -	if (crypto_info->version != TLS_1_2_VERSION) {
> +	if (crypto_info->version != TLS_1_2_VERSION &&
> +	    crypto_info->version != TLS_1_3_VERSION) {
>  		rc = -EOPNOTSUPP;
>  		goto release_netdev;
>  	}

Are all existing drivers rejecting TLS 1.3 sessions?
These days we prefer for drivers to explicitly opt into new features 
to avoid surprises.
-- 
pw-bot: cr