Re: [PATCH net-next v17 13/15] tls: device: add tracepoints for the KeyUpdate path

[netdev-bot+sashiko@kernel.org]

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

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

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] New tracepoint `tls_device_rekey_start`
  (net/tls/trace.h:195-221) declares and prints its sequence field as…
- [Low] The new comment added at the tls_device_sendmsg() call site
  (net/tls/tls_device.c:869-877) states "Non-zero here is the transient…

--

tls: device: add tracepoints for the KeyUpdate path

This adds five trace events to net/tls/trace.h covering the device
KeyUpdate/rekey state machine: rekey_start, rekey_reencrypt and
rekey_done on the RX side, plus complete_rekey_retry and
complete_rekey_fail on the TX side. The RX events are emitted from
tls_set_device_offload_rx() and tls_device_decrypted(), the TX ones from
tls_device_sendmsg() and the fallback path of
tls_device_complete_rekey().

No Link: tags were present in the commit header.

> diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> index ac09f356cff9..5f45c097bad3 100644
> --- a/net/tls/tls_device.c
> +++ b/net/tls/tls_device.c
> @@ -866,8 +866,16 @@ int tls_device_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
>  	lock_sock(sk);
>  
>  	/* Old-key records all ACKed; switch back to HW. */
> -	if (test_bit(TLS_TX_REKEY_READY, &tls_ctx->flags))
> -		tls_device_complete_rekey(sk, tls_ctx, true, msg->msg_flags);
> +	if (test_bit(TLS_TX_REKEY_READY, &tls_ctx->flags)) {
> +		rc = tls_device_complete_rekey(sk, tls_ctx, true, msg->msg_flags);
> +		/* Non-zero here is the transient -EAGAIN retry,
> +		 * the next sendmsg retries. Hard failures return 0 after
> +		 * falling back to SW and emit tls_device_complete_rekey_fail
> +		 * from the fallback path.
> +		 */
> +		if (rc)
> +			trace_tls_device_complete_rekey_retry(sk);
> +	}

[Severity: Low]
Is "Non-zero here is the transient -EAGAIN retry" the full set of
transient returns here?

tls_device_complete_rekey() passes three errors back to this caller:

net/tls/tls_device.c:tls_device_complete_rekey() {
	...
	/* -EAGAIN (sndbuf full) and a signal (-EINTR/-ERESTARTSYS from
	 * sk_stream_wait_memory()) are transient: leave the rekey PENDING and
	 * retry on the next sendmsg rather than permanently dropping HW offload.
	 */
	if (rc == -EAGAIN || rc == -EINTR || rc == -ERESTARTSYS)
		return rc;
	...
}

So a signal delivered to a sender blocked in sk_stream_wait_memory()
during a KeyUpdate also emits trace_tls_device_complete_rekey_retry(),
not only sndbuf pressure. Should the comment here (and the matching
"TX rekey completion hit the transient -EAGAIN retry in sendmsg" text in
the changelog) name -EINTR/-ERESTARTSYS too?

Related: the retry event carries only sk, while its sibling
tls_device_complete_rekey_fail(sk, rc) carries rc. Without rc on the
retry event, can a trace distinguish repeated sndbuf exhaustion from
repeated signal delivery when a TX rekey stays PENDING? Would adding rc
to tls_device_complete_rekey_retry make that visible?

>  
>  	if (tls_device_tx_uses_sw(tls_ctx)) {
>  		rc = tls_sw_sendmsg_locked(sk, msg, size);
> @@ -1430,10 +1438,15 @@ int tls_device_decrypted(struct sock *sk, struct tls_context *tls_ctx)
>  				return 0;
>  			}
>  
> +			trace_tls_device_rekey_reencrypt(sk, rec_start_seq,
> +							 ctx->rekey.old_nic_boundary);
> +
>  			return tls_device_reencrypt_old_key(sk, ctx,
>  							    sw_ctx, tls_ctx);
>  		}
>  
> +		trace_tls_device_rekey_done(sk, rec_start_seq,
> +					    ctx->rekey.old_nic_boundary);
>  		crypto_free_aead(ctx->rekey.old_aead_recv);
>  		ctx->rekey.old_aead_recv = NULL;
>  

[ ... ]

> @@ -1890,6 +1903,13 @@ static int tls_device_complete_rekey(struct sock *sk, struct tls_context *ctx,
>  	TLS_DEC_STATS(sock_net(sk), LINUX_MIB_TLSCURRTXDEVICE);
>  	TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSCURRTXSW);
>  
> +	/* Hard failure: HW rekey gave up and the connection is now pinned to
> +	 * SW encryption. The call site only sees the transient -EAGAIN retry
> +	 * (rc is not propagated here), so emit the trace from the fallback
> +	 * path itself; rc still holds the originating error.
> +	 */
> +	trace_tls_device_complete_rekey_fail(sk, rc);
> +
>  	return 0;
>  }
>  
> @@ -2195,11 +2215,21 @@ int tls_set_device_offload_rx(struct sock *sk, struct tls_context *ctx,
>  			 * is installed once drain_start crosses rekey.old_nic_boundary.
>  			 */
>  			context->dev_add_pending = 1;
> +			trace_tls_device_rekey_start(sk, drain_start,
> +						     context->rekey.old_nic_boundary,
> +						     true);
>  		} else {
>  			struct tcp_sock *tp = tcp_sk(sk);
>  			u32 nic_end;
>  
>  			if (context->rekey.old_aead_recv) {
> +				/* Prior rekey's era already drained (drain_start is
> +				 * past old_nic_boundary), so retiring its key here
> +				 * is a boundary crossing, same as the free in
> +				 * tls_device_decrypted(); mark it done.
> +				 */
> +				trace_tls_device_rekey_done(sk, drain_start,
> +							    context->rekey.old_nic_boundary);
>  				crypto_free_aead(context->rekey.old_aead_recv);
>  				context->rekey.old_aead_recv = NULL;
>  			}

[ ... ]

> @@ -2247,6 +2277,8 @@ int tls_set_device_offload_rx(struct sock *sk, struct tls_context *ctx,
>  				context->dev_add_pending = 0;
>  				retired_pending = true;
>  			}
> +			trace_tls_device_rekey_start(sk, drain_start, nic_end,
> +						     before(drain_start, nic_end));
>  		}
>  	}
>  
> diff --git a/net/tls/trace.h b/net/tls/trace.h
> index 2d8ce4ff3265..5b9c1f86d82d 100644
> --- a/net/tls/trace.h
> +++ b/net/tls/trace.h
> @@ -192,6 +192,124 @@ TRACE_EVENT(tls_device_tx_resync_send,
>  	)
>  );
>  
> +TRACE_EVENT(tls_device_rekey_start,
> +
> +	TP_PROTO(struct sock *sk, u32 copied_seq, u32 nic_boundary,
> +		 bool inflight),
> +
> +	TP_ARGS(sk, copied_seq, nic_boundary, inflight),
> +
> +	TP_STRUCT__entry(
> +		__field(	struct sock *,	sk		)
> +		__field(	u32,		copied_seq	)
> +		__field(	u32,		nic_boundary	)
> +		__field(	bool,		inflight	)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->sk = sk;
> +		__entry->copied_seq = copied_seq;
> +		__entry->nic_boundary = nic_boundary;
> +		__entry->inflight = inflight;
> +	),
> +
> +	TP_printk(
> +		"sk=%p copied_seq=%u nic_boundary=%u inflight=%d",
> +		__entry->sk, __entry->copied_seq, __entry->nic_boundary,
> +		__entry->inflight
> +	)
> +);

[Severity: Medium]
Is copied_seq the right name for this field? Both producers in
tls_set_device_offload_rx() pass drain_start, which comes from
tls_device_rx_rec_start():

net/tls/tls_device.c:tls_device_rx_rec_start() {
	u32 copied_seq = tcp_sk(sk)->copied_seq;

	if (sw_ctx->strp.copy_mode)
		return copied_seq - sw_ctx->strp.anchor->len;

	return copied_seq;
}

In strparser copy mode that is the record start, not tp->copied_seq,
which is also what the comment above drain_start says:

	/* Classify against the record start, not the raw copied_seq: ...

The same quantity is exported as tcp_seq by the pre-existing
tls_device_decrypted event and by the two RX events added here
(tls_device_rekey_reencrypt/tls_device_rekey_done, both fed
rec_start_seq). Since one header would then label one value two
different ways, would naming this field tcp_seq keep the interface
self-consistent for anyone correlating rekey_start with
tls_device_decrypted?

Separately, would the rx_/tx_ direction qualifier used by the existing
events in this header (tls_device_rx_resync_send,
tls_device_tx_resync_req, tls_device_tx_resync_send) be worth applying
here? rekey_start, rekey_done and rekey_reencrypt are only emitted from
the RX path (tls_set_device_offload_rx() and tls_device_decrypted()),
while complete_rekey_retry/_fail are only emitted from the TX path, so
enabling tls_device_rekey_start while chasing a TX KeyUpdate yields no
events.

> +
> +TRACE_EVENT(tls_device_rekey_reencrypt,
> +
> +	TP_PROTO(struct sock *sk, u32 tcp_seq, u32 nic_boundary),
> +
> +	TP_ARGS(sk, tcp_seq, nic_boundary),

[ ... ]

> +TRACE_EVENT(tls_device_complete_rekey_fail,
> +
> +	TP_PROTO(struct sock *sk, int rc),
> +
> +	TP_ARGS(sk, rc),

[ ... ]

> +TRACE_EVENT(tls_device_complete_rekey_retry,
> +
> +	TP_PROTO(struct sock *sk),
> +
> +	TP_ARGS(sk),
> +
> +	TP_STRUCT__entry(
> +		__field(	struct sock *,	sk	)
> +	),

[ ... ]

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