Re: [PATCH net-next v17 08/15] tcp: fence collapse against rtx-queue tail when write queue is empty

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Changelog claim about TLS 1.3 device-offload KeyUpdate: the
  concern states that the final paragraph ("TLS 1.3 device-offload…
- [Low] Stale call-site comment: net/tls/tls_device.c:169-173
  (tls_device_commit_start_marker) states "So mark the last skb in the…

--

tcp: fence collapse against rtx-queue tail when write queue is empty

This makes tcp_write_collapse_fence() fall back to the retransmit queue
tail when the write queue is empty, so the end-of-record boundary is still
marked at the point encryption keys are installed.  The stated goal is to
stop tcp_retrans_try_collapse() and tcp_shift_skb_data() from merging a
post-key skb into a pre-key skb.

No Link: tags are present in the commit header.

Two small notes, both about wording rather than behaviour.

> tcp: fence collapse against rtx-queue tail when write queue is empty

[Severity: Low]
The changelog ends with:

    "TLS 1.3 device-offload KeyUpdate additionally relies on this behavior
     to keep old-key and new-key records in distinct skbs for
     re-encryption on RX."

At this commit there is no device-offload KeyUpdate path that can reach
the helper, since tls_set_device_offload() and tls_set_device_offload_rx()
in net/tls/tls_device.c still reject a rekey on an already-configured
socket:

net/tls/tls_device.c:tls_set_device_offload() {
	/* A rekey (setsockopt on an already-configured socket) is not
	 * supported on the device offload path yet; ...
	 */
	if (ctx->tx_conf != TLS_BASE)
		return -EOPNOTSUPP;

The KeyUpdate consumer that does call tcp_write_collapse_fence() arrives
in the later patches of the series.  Would it be worth phrasing that
paragraph as a forward reference to the upcoming KeyUpdate patches, so
the dependency ordering is clear to someone reading this commit alone?

> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 5e5f5f9b89a3..8c6d90e962c4 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -2340,6 +2340,15 @@ static inline void tcp_write_collapse_fence(struct sock *sk)
>  {
>  	struct sk_buff *skb = tcp_write_queue_tail(sk);
>  
> +	/* When nothing is queued for transmit, the last skb of the current
> +	 * state is the rtx queue tail (its end_seq == snd_nxt == write_seq).
> +	 * Fence that instead, otherwise the boundary is left unmarked and a
> +	 * later tcp_retrans_try_collapse()/tcp_shift_skb_data() can merge it
> +	 * with the first skb of the next state across the fence (they only test
> +	 * the tail's EOR, not skb->decrypted).
> +	 */
> +	if (!skb)
> +		skb = tcp_rtx_queue_tail(sk);

[Severity: Low]
With this fallback the helper can now set eor on an already-transmitted,
unacked skb from the retransmit queue, not just on a write-queue skb.

The comment at the tls_device.c call site still describes the old
behaviour:

net/tls/tls_device.c:tls_device_commit_start_marker() {
	/* TLS offload is greatly simplified if we don't send
	 * SKBs where only part of the payload needs to be encrypted.
	 * So mark the last skb in the write queue as end of record.
	 */
	tcp_write_collapse_fence(sk);
}

That text is unchanged through the end of the series.  Since the commit
message describes the empty-write-queue case as the common state at key
install time, the fallback is the path most readers will hit.

Should that comment be updated to mention the retransmit-queue tail as
well?

>  	if (skb)
>  		TCP_SKB_CB(skb)->eor = 1;
>  }

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