Re: [PATCH net-next v17 06/15] tls: prep helpers and refactors for HW offload KeyUpdate

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Concern (validated as NOT actionable): moving
  crypto_aead_setauthsize() into the `if (!*aead)` block of…

--

tls: prep helpers and refactors for HW offload KeyUpdate

This patch does preparatory refactoring for TX and RX HW rekey support:
hoisting cipher_context / tls_crypto_context, adding tls_tx_cipher_ctx(),
splitting tls_set_device_offload() and tls_sw_splice_eof(), and moving
crypto_aead_setauthsize() into the !*aead block of tls_sw_ctx_init().

One question below about the relocated crypto_aead_setauthsize() call.

> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index 7b593dac2c31..5531303dd704 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c

[ ... ]

> @@ -2576,6 +2592,10 @@ int tls_sw_ctx_init(struct sock *sk, int tx,
>  
>  	key = crypto_info_key(src_crypto_info, cipher_desc);
>  
> +	/* A rekey normally reuses the existing tfm; the RX HW rekey hands over a
> +	 * NULL aead (the old one is retained for the drain), so allocate and
> +	 * configure authsize only when a fresh tfm is created here.
> +	 */
>  	if (!*aead) {
>  		*aead = crypto_alloc_aead(cipher_desc->cipher_name, 0, 0);
>  		if (IS_ERR(*aead)) {
> @@ -2583,6 +2603,10 @@ int tls_sw_ctx_init(struct sock *sk, int tx,
>  			*aead = NULL;
>  			goto free_priv;
>  		}
> +
> +		rc = crypto_aead_setauthsize(*aead, prot->tag_size);
> +		if (rc)
> +			goto free_aead;
>  	}

[Severity: Medium]
Could the contract for this new error path be spelled out in the comment?

This adds a fallible step inside the !*aead block, and its unwind label
does:

net/tls/tls_sw.c:tls_sw_ctx_init() {
	...
free_aead:
	crypto_free_aead(*aead);
	*aead = NULL;
free_priv:
	if (!new_crypto_info) {
	...
}

So when a caller combines new_crypto_info != NULL with *aead == NULL, a
setauthsize failure leaves the live sw context with aead_recv == NULL and
free_priv skipped.

The same shape applies to the setkey branch just below, where a freshly
allocated tfm is left installed but unkeyed:

	rc = crypto_aead_setkey(*aead, key, cipher_desc->key);
	if (rc) {
		if (new_crypto_info)
			goto out;

At this commit neither case looks reachable: the only rekey caller reaches
tls_sw_ctx_init() with an already-allocated *aead, and both device helpers
still return -EOPNOTSUPP when tx_conf / rx_conf is not TLS_BASE, so
!*aead is never taken together with new_crypto_info != NULL.

The later RX rekey work in this series is the first user that hands over a
NULL aead, and it does restore context->rekey.old_aead_recv on every
tls_sw_ctx_init() failure, so the ownership rule seems intentional. Would
it be worth stating in the comment that a caller passing *aead == NULL with
new_crypto_info != NULL owns restoring the previous tfm on error, so a
future caller does not inherit a NULL or unkeyed aead_recv?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917224355.2288021-1-rjethwani%40purestorage.com