Re: [PATCH v8 3/6] tls: add TLS 1.3 hardware offload support

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
tls: add TLS 1.3 hardware offload support

Add TLS 1.3 support to the kernel TLS hardware offload infrastructure,
enabling hardware acceleration for TLS 1.3 connections on capable NICs.

> diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> index 99c8eff9783e..5f5c0db118ff 100644
> --- a/net/tls/tls_device.c
> +++ b/net/tls/tls_device.c
> @@ -319,6 +319,27 @@ 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 */
> +	if (prot->version == TLS_1_3_VERSION) {
> +		struct page_frag dummy_content_type_frag;
> +		struct page_frag *content_type_pfrag = pfrag;
> +
> +		if (unlikely(pfrag->size - pfrag->offset < prot->tail_size) &&
> +		    !skb_page_frag_refill(prot->tail_size, pfrag,
> +					  sk->sk_allocation)) {
> +			dummy_content_type_frag.page = dummy_page;
> +			dummy_content_type_frag.offset = record_type;
> +			content_type_pfrag = &dummy_content_type_frag;
> +		} else {
> +			unsigned char *content_type_addr;
> +
> +			content_type_addr = page_address(pfrag->page) +
> +					    pfrag->offset;
> +			*content_type_addr = record_type;
> +		}
> +		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

For TLS 1.3, tls_device_record_close() now appends two separate fragments
instead of one. Looking at the caller tls_push_data():

net/tls/tls_device.c:tls_push_data() {
    ...
    if (record->num_frags >= MAX_SKB_FRAGS - 1 ||
        record->len >= data_limit)
        tls_device_record_close(sk, tls_ctx, ctx, record,
                                copied ? sg_data : NULL,
                                copied, record_type);
}

The caller closes the record when num_frags >= MAX_SKB_FRAGS - 1, reserving
only one slot for the close operation.

Under memory pressure, when both skb_page_frag_refill() calls fail (the new
one for content type and the existing one for tag), both fragments use
dummy_page but at different offsets:

    dummy_content_type_frag.offset = record_type (e.g., 23 for app data)
    dummy_tag_frag.offset = 0

These two dummy frags cannot coalesce in tls_append_frag() because their
offsets aren't contiguous (record_type+1 != 0 for any valid record_type).

Can this overflow the frags[] array in struct tls_record_info? If num_frags
starts at 16, the content type increment makes it 17, and the tag increment
makes it 18, but frags[MAX_SKB_FRAGS] only has valid indices 0-16. The
second tls_append_frag() call would write to frags[17], which is past the
end of the array.

Sabrina Dubroca noted in v7 review that you should ensure all 5 in-tree
drivers explicitly reject TLS 1.3 sessions. In v3, Jakub Kicinski asked
'Are all existing drivers rejecting TLS 1.3 sessions? These days we prefer
for drivers to explicitly opt into new features to avoid surprises.'

Your response mentioned only 3 drivers (chelsio/chtls, fungible/funeth,
mellanox/mlx5), but we have 5 drivers in tree. Sabrina noted the 2 your
reply didn't mention don't seem to have version checks. Without explicit
TLS 1.3 rejection in all drivers, the new TLS 1.3 path may be attempted by
drivers that don't support it.

Which drivers are the missing 2, and do they need version checks added?

[ ... ]

> @@ -1089,11 +1110,6 @@ int tls_set_device_offload(struct sock *sk)
>  	}
>
>  	crypto_info = &ctx->crypto_send.info;
> -	if (crypto_info->version != TLS_1_2_VERSION) {
> -		rc = -EOPNOTSUPP;
> -		goto release_netdev;
> -	}
> -
>  	cipher_desc = get_cipher_desc(crypto_info->cipher_type);
>  	if (!cipher_desc || !cipher_desc->offloadable) {
>  		rc = -EINVAL;

In v7 review, Sabrina Dubroca suggested this version check can be removed
entirely:

    'You can get rid of this whole version check, it was only needed
    because HW offload supported less than SW. tls_main already ensures
    that the version is 1.2 or 1.3.'

Was there a reason to keep this check in tls_set_device_offload() and
tls_set_device_offload_rx()?

[ ... ]