Re: [RFC PATCH v7 2/4] tls: add hardware offload key update support

[Sabrina Dubroca <sd@queasysnail.net>]

2026-02-05, 16:15:56 -0700, Rishikesh Jethwani wrote:
> Add TLS KeyUpdate (rekey) support for hardware offload connections,
> enabling key rotation on established TLS 1.3 connections without
> tearing down the hardware offload.
> 
> Key changes:
> 
> 1. Rekey API: Extended tls_set_device_offload() and
>    tls_set_device_offload_rx() with new_crypto_info parameter to
>    distinguish initial setup from key updates. During rekey, the old
>    HW context is deleted (tls_dev_del) and a new one is added
>    (tls_dev_add) with the updated key material.

I don't think the commit message needs that level of detail. You'll
probably want to look through the git log for net/ to get an idea of
what commit messages typically look like for kernel networking.


> 2. Graceful degradation: If hardware key update fails, the connection
>    gracefully degrades to software.

In do_tls_setsockopt_conf you have:

+		} else if (update && ctx->tx_conf == TLS_HW) {
+			/* HW rekey failed - return the actual error.
+			 * Cannot fall back to SW for an existing HW connection.
+			 */
 			goto err_crypto_info;

which doesn't look very graceful.

>  For TX, TLS_TX_DEV_CLOSED is set
>    and sk_validate_xmit_skb switches to tls_validate_xmit_skb_sw for
>    software encryption. For RX, TLS_RX_DEV_DEGRADED and TLS_RX_DEV_CLOSED
>    are set for software decryption. tx_conf/rx_conf remains TLS_HW.
> 
> 3. Record sequence management: During TX rekey, old pending records
>    are deleted and unacked_record_sn is reset to the new rec_seq.

So what happens if we need to retransmit them? We just give up?

There was quite some discussion about how rekey would work with HW
offload back when I implemented rekey for SW, which got recorded in
the cover letter and committed as da3e3186ef13 ("Merge branch
'tls1.3-key-updates'"). I would expect to either see those things
implemented in this series, or a justification for why they need to be
done differently (it's possible that things turn out different in
practice from what we discussed back then).

> 4. SW context refactoring: Split tls_set_sw_offload() into
>    tls_sw_ctx_init() and tls_sw_ctx_finalize() to allow the HW offload
>    RX path to initialize SW context first, attempt HW setup, then
>    finalize (memzero new_crypto_info, call tls_finish_key_update).

To ease reviews, maybe extract the refactoring into another patch to
separate it from the actual rekey implementation.

> 5. Added TLS_TX_DEV_CLOSED flag to track TX hardware context state,
>    to avoid double tls_dev_del call, symmetric with existing
>    TLS_RX_DEV_CLOSED.

AFAICT this bit is equivalent to having
    sk->sk_validate_xmit_skb == tls_validate_xmit_skb_sw

Do we need the bit or can we just check sk_validate_xmit_skb?

> This removes the rekey rejection checks added in the previous patch,
> replacing them with full rekey support including graceful degradation.

(nit: this precision is probably not needed)


[...]
> diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> index 459963c254f4..4aaa51a35f1f 100644
[...]
>  	crypto_info = &ctx->crypto_send.info;
> -	if (crypto_info->version != TLS_1_2_VERSION &&
> -	    crypto_info->version != TLS_1_3_VERSION) {
> +	src_crypto_info = new_crypto_info ?: crypto_info;
> +	if (src_crypto_info->version != TLS_1_2_VERSION &&
> +	    src_crypto_info->version != TLS_1_3_VERSION) {
>  		rc = -EOPNOTSUPP;
>  		goto release_netdev;
>  	}
>  
> -	cipher_desc = get_cipher_desc(crypto_info->cipher_type);
> +	cipher_desc = get_cipher_desc(src_crypto_info->cipher_type);

This shouldn't be necessary, since do_tls_setsockopt_conf checks that
we're not changing cipher type during a rekey.


>  	/* Avoid offloading if the device is down
>  	 * We don't want to offload new flows after
> @@ -1182,29 +1194,91 @@ int tls_set_device_offload(struct sock *sk)
>  		goto release_lock;
>  	}
>  
> -	ctx->priv_ctx_tx = offload_ctx;
> +	if (!new_crypto_info) {
> +		ctx->priv_ctx_tx = offload_ctx;
> +	} else {
> +		char *key = crypto_info_key(src_crypto_info, cipher_desc);
> +
> +		offload_ctx = tls_offload_ctx_tx(ctx);
> +
> +		rc = crypto_aead_setkey(offload_ctx->aead_send, key,
> +					cipher_desc->key);
> +		if (rc)
> +			goto release_lock;
> +
> +		/* For rekey, delete old HW context before adding new one. */

As in the other patch, some unnecessary comments.

[...]
> +		memcpy(ctx->tx.iv + cipher_desc->salt, iv, cipher_desc->iv);
> +		memcpy(ctx->tx.rec_seq, rec_seq, cipher_desc->rec_seq);
> +
> +		spin_lock_irqsave(&offload_ctx->lock, flags);
> +		/* Delete old records, can't be retransmitted with new key */
> +		delete_all_records(offload_ctx);
> +
> +		/* Update unacked_record_sn for the new key's rec_seq.
> +		 * This is critical for SW fallback encryption to use
> +		 * the correct record sequence number after rekey.

The whole comment should probably be removed, but if it stays, it
should explain why this is critical, rather than just asserting that
it is. (but I guess this code will need to be changed to handle
unacked records anyway)

[...]
>  	dev_put(netdev);
>  
>  	return 0;
>  
>  release_lock:
>  	up_read(&device_offload_lock);
> +	if (new_crypto_info)
> +		goto release_netdev;

nit: not super elegant, but I'm not sure we can do much better :/

>  	clean_acked_data_disable(tcp_sk(sk));
>  	crypto_free_aead(offload_ctx->aead_send);
>  free_offload_ctx:
> @@ -1217,17 +1291,33 @@ int tls_set_device_offload(struct sock *sk)
>  	return rc;
>  }
>  

-- 
Sabrina