Re: [PATCH net v2 2/4] tls: device: fix next-record sequence in core-driven RX resync

[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 (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] In tls_device_core_ctrl_rx_resync() (net/tls/tls_device.c),
  this patch makes the next-record sequence sent to the device…

Pre-existing issues:
- [Medium] The other CORE_NEXT_HINT resync path, tls_rx_msg_size() ->
  tls_device_rx_resync_new_rec() (net/tls/tls_sw.c:2234-2235 at this…

--

tls: device: fix next-record sequence in core-driven RX resync

tls_device_core_ctrl_rx_resync() used to pass tcp_sk(sk)->copied_seq to
tls_device_resync_rx() as the TCP sequence of the next record. This patch
passes tls_strp_msg_seq() + rxm->full_len instead. The commit message
says the result is the start of the next record in both strparser modes.

> diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> index fbfe6ec3df048..a6c4d0cc54c73 100644
> --- a/net/tls/tls_device.c
> +++ b/net/tls/tls_device.c

[ ... ]

> @@ -877,8 +878,9 @@ static void tls_device_core_ctrl_rx_resync(struct tls_context *tls_ctx,
>  		memcpy(rcd_sn, tls_ctx->rx.rec_seq, prot->rec_seq_size);
>  		tls_bigint_increment(rcd_sn, prot->rec_seq_size);
>  
> -		tls_device_resync_rx(tls_ctx, sk, tcp_sk(sk)->copied_seq,
> -				     rcd_sn);
> +		tls_device_resync_rx(tls_ctx, sk,
> +				     tls_strp_msg_seq(sw_ctx) +
> +				     rxm->full_len, rcd_sn);

[Severity: Medium]
The sequence passed here no longer depends on the strparser mode. Does the
check that picks this branch still depend on it?

Earlier in tls_device_core_ctrl_rx_resync():

	/* head of next rec is already in, parser will sync for us */
	if (tcp_inq(sk) > rxm->full_len) {
		trace_tls_device_rx_resync_nh_schedule(sk);
		ctx->resync_nh_do_now = 1;
	} else {

tcp_inq() counts from copied_seq. In copy mode, tls_strp_read_copy() has
already moved copied_seq past the current record, so tcp_inq() counts only
the bytes of the next record.

Suppose between 1 and rxm->full_len bytes of the next record are already
queued. The else branch runs, and tls_dev_resync() is called for a TCP
position the device has already passed. resync_nh_do_now is never set, so
tls_device_rx_resync_new_rec() also returns early:

	case TLS_OFFLOAD_SYNC_TYPE_CORE_NEXT_HINT:
		if (likely(!rx_ctx->resync_nh_do_now))
			return;

Would this waste the resync attempt and keep the stream on software
decryption until the backed-off retry?

This tcp_inq() check was already there before this patch. The later commit
"tls: device: fix next-record queued checks in core-driven RX resync" in
this series appears to fix it. That commit replaces the check with
after(READ_ONCE(tcp_sk(sk)->rcv_nxt), next_seq).

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. The
commit message says the reported sequence "no longer depends on which mode
the strparser is in". Does that also hold for the other CORE_NEXT_HINT
path?

tls_rx_msg_size() in net/tls/tls_sw.c still computes the record start from
the anchor skb:

	tls_device_rx_resync_new_rec(strp->sk, data_len + TLS_HEADER_SIZE,
				     TCP_SKB_CB(skb)->seq + strp->stm.offset);

In copy mode, tls_strp_read_copy() resets stm.offset to 0 but keeps the
anchor's old cb. The value above can then be lower than the real record
start, while tls_device_core_ctrl_rx_resync() now uses strp->seq.

When tls_device_core_ctrl_rx_resync() sets resync_nh_do_now, it hands the
resync to this path. tls_device_rx_resync_new_rec() also still assumes
normal mode:

		/* head of next rec is already in, note that the sock_inq will
		 * include the currently parsed message when called from parser
		 */
		sock_data = tcp_inq(sk);
		if (sock_data > rcd_len) {

In copy mode, could the device then get the wrong next-record boundary for
CORE_NEXT_HINT? Could the request sequence also fail to match for
DRIVER_REQ and DRIVER_REQ_ASYNC?

Two later commits in this series look like they fix this:

  "tls: device: fix record start reported to RX resync in copy mode"
  changes the argument to strp->seq.

  "tls: device: fix next-record queued checks in core-driven RX resync"
  changes the tcp_inq() check in tls_device_rx_resync_new_rec() to
  READ_ONCE(tcp_sk(sk)->rcv_nxt) - seq.

>  	}
>  }

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