Re: [PATCH net] tls: only install push_pending_record for a TX context

[Sabrina Dubroca <sd@queasysnail.net>]

2026-10-15, 22:26:04 -0600, Rishikesh Jethwani wrote:
> tls_set_sw_offload() assigns ctx->push_pending_record for both
> directions, but the hook is a TX-side dispatcher. On a socket whose TX
> side is HW offloaded, tls_set_device_offload() installs
> tls_device_push_pending_record(); any later TLS_RX setsockopt overwrites
> it with tls_sw_push_pending_record(), because tls_set_device_offload_rx()
> itself calls tls_set_sw_offload(sk, 0, NULL) for the software half of a
> successful HW RX offload, and so does the fallback when the RX offload
> is declined (NIC with TX-only offload, tls_dev_add() failure).

This makes sense. Some comments:

> From then on tls_handle_open_record() (a TLS_SET_RECORD_TYPE cmsg after
> a MSG_MORE send, or the close path with a pending writer) runs
> tls_sw_push_pending_record() against priv_ctx_tx, which is a
> struct tls_offload_context_tx, not a struct tls_sw_context_tx, and
> reads open_rec from inside the offload context. On x86_64 without
> lock debugging that field overlays sg_tx_data[2].dma_address,
> which is never set, so the push is silently skipped and the data

Do we have arches that support TLS offload but use dma_address, or
where the struct layout would allow something more harmful than "wrong
payload"? I'm worried that we could access some random address for the
open record.

But on its own, the wrong payload within a TLS stream seems bad
enough, I think it's worth a Cc: stable tag.

> buffered by the MSG_MORE send is later sent under the cmsg's
> record type, e.g. application data in a handshake
> or alert record. With a layout where the field lands on a non-zero
> value it is dereferenced and fed to bpf_exec_tx_verdict().
> 
> Only set the hook when initialising the TX side.
> 
> Found by code review while developing TLS 1.3 HW offload
> rekey support.
> 
> Tested with HW TLS TX and RX offload on Mellanox ConnectX-6 Dx
> (Crypto Enabled): after setting TLS_RX,
> send() "AAAA" with MSG_MORE, then sendmsg() "BBBB" with a
> TLS_SET_RECORD_TYPE cmsg for a handshake record. Without this patch
> the peer receives a single handshake record "AAAABBBB"; with it, an
> application data record "AAAA" followed by a handshake record "BBBB".

If we already had selftests for offload, I'd request a selftest for
this.

Would it be feasible to extract the non-1.3/non-rekey bits of the
selftests you have in your 1.3 offload series? Those would be valuable
on their own.

> Fixes: e8f69799810c ("net/tls: Add generic NIC offload infrastructure")
> Signed-off-by: Rishikesh Jethwani <rjethwani@purestorage.com>
> ---
> This is independent of the TLS 1.3 HW offload KeyUpdate series, but
> that series' TX rekey path (tls_device_start_rekey()) calls
> ctx->push_pending_record() and depends on this fix.
> ---
>  net/tls/tls_sw.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index d1ad31986cf2..b3fe153c3c68 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -2591,7 +2591,12 @@ int tls_set_sw_offload(struct sock *sk, int tx,
>  		}
>  	}
>  
> -	ctx->push_pending_record = tls_sw_push_pending_record;
> +	/* The RX path shares tls_context with a TX side that may be HW
> +	 * offloaded and rely on tls_device_push_pending_record(); only the TX
> +	 * setup owns this hook.
> +	 */

Please, no. If I see "if (tx) install_push_callback;", I don't wonder
why we install the callback or why we install it only for TX.

> +	if (tx)
> +		ctx->push_pending_record = tls_sw_push_pending_record;

-- 
Sabrina