Re: [PATCH v16 09/10] tls: device: add tracepoints for the KeyUpdate path

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
tls: device: add tracepoints for the KeyUpdate path

This adds four trace events to net/tls/trace.h and wires them into the
RX and TX rekey paths in net/tls/tls_device.c. The events are intended to
let an operator follow the KeyUpdate state machine: rekey start, the
old-key reencrypt pass, boundary crossing, and TX rekey completion
failure in sendmsg.

> diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> index c34268cd9531..9a4c121dbebb 100644
> --- a/net/tls/tls_device.c
> +++ b/net/tls/tls_device.c
> @@ -760,8 +760,14 @@ 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);
> +	if (test_bit(TLS_TX_REKEY_READY, &tls_ctx->flags)) {
> +		rc = tls_device_complete_rekey(sk, tls_ctx, true);
> +		/* On failure the READY bit is left set; the next sendmsg
> +		 * retries.
> +		 */
> +		if (rc)
> +			trace_tls_device_complete_rekey_fail(sk, rc);
> +	}

[Severity: Low]
Can this event ever fire for an actual HW rekey failure?

tls_device_complete_rekey() has only one non-zero return, the drain at
the top:

net/tls/tls_device.c:tls_device_complete_rekey() {
	...
	rc = tls_sw_drain_tx(sk, ctx);
	if (rc)
		return rc;
	...
}

Every other failure (netdev gone, crypto_alloc_aead(),
crypto_aead_setkey(), crypto_aead_setauthsize(), tls_device_dev_add_tx())
goes to release_lock and then rekey_fail, which returns success:

net/tls/tls_device.c:tls_device_complete_rekey() {
rekey_fail:
	...
	set_bit(TLS_TX_REKEY_FAILED, &ctx->flags);
	...
	return 0;
}

So the case that permanently drops the connection to SW encryption emits
nothing, while the case that does emit the event named _fail is
tls_sw_drain_tx() returning -EAGAIN:

net/tls/tls_sw.c:tls_sw_drain_tx() {
	if (rc < 0 || tls_is_partially_sent_record(ctx) ||
	    tls_is_pending_open_record(ctx))
		return rc < 0 ? rc : -EAGAIN;
}

which is a transient retry rather than a failure. Should the tracepoint
be emitted from the rekey_fail path instead, or in addition?

> @@ -1267,6 +1273,9 @@ 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);
> +
>  			/* rekey_fixup sets decrypted flags in case the NIC clears
>  			 * decrypted flags on auth failure
>  			 */
> @@ -1277,6 +1286,8 @@ int tls_device_decrypted(struct sock *sk, struct tls_context *tls_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;
>  
> @@ -1923,6 +1934,8 @@ int tls_set_device_offload_rx(struct sock *sk, struct tls_context *ctx,
>  					netdev->tlsdev_ops->tls_dev_rx_rekey_fixup;
>  				context->dev_add_pending = 1;
>  			}
> +			trace_tls_device_rekey_start(sk, copied_seq, rcv_nxt,
> +						     before(copied_seq, rcv_nxt));
>  		}
>  	}

[Severity: Low]
Is this the only arm that should be traced?

The call sits inside the else arm of the RX rekey branch. The other arm
in tls_set_device_offload_rx() also accepts a rekey and defers dev_add,
and emits nothing:

net/tls/tls_device.c:tls_set_device_offload_rx() {
		if (context->rekey.old_aead_recv &&
		    before(copied_seq, context->rekey.old_nic_boundary)) {
			/* Previous rekey still draining. ... */
			context->dev_add_pending = 1;
		} else {
			...
			trace_tls_device_rekey_start(...);
		}
}

That first arm is the "old-key data is still queued, dev_add deferred"
state the commit message attributes to inflight=1. Since
tls_device_decrypted() emits trace_tls_device_rekey_done() for any
non-NULL ctx->rekey.old_aead_recv, wouldn't a second KeyUpdate arriving
while the previous one is still draining produce a rekey_done with no
preceding rekey_start?

Also, inside the traced arm the event fires even when
before(copied_seq, rcv_nxt) is false, in which case no old-key state is
created and rcv_nxt is never stored into ctx->rekey.old_nic_boundary.
Does the reported nic_boundary describe anything in that case?

[Severity: Low]
Are the start and done events meant to pair up?

trace_tls_device_rekey_start() is emitted here, before the rekey can
still be aborted. If the following tls_sw_ctx_init() fails, the
release_ctx path undoes the state that was just traced:

net/tls/tls_device.c:tls_set_device_offload_rx() {
release_ctx:
	...
		context->dev_add_pending = was_dev_add_pending;
		if (moved_aead_recv) {
			...
			sw_ctx->aead_recv = context->rekey.old_aead_recv;
			context->rekey.old_aead_recv = NULL;
		}
}

so a rekey_start is recorded for a rekey that did not take effect, with
no cancelling event.

In the other direction, rekey_done is emitted only at the free site in
tls_device_decrypted(). old_aead_recv is also freed because the boundary
was already crossed here:

net/tls/tls_set_device_offload_rx() {
			if (context->rekey.old_aead_recv) {
				crypto_free_aead(context->rekey.old_aead_recv);
				context->rekey.old_aead_recv = NULL;
			}
}

and on the RX teardown and context-free paths, none of which emit
rekey_done. Should those sites emit the event too, or should the commit
message avoid describing the events as a start/done pair?

> diff --git a/net/tls/trace.h b/net/tls/trace.h
> index 2d8ce4ff3265..2a90b77d75e8 100644
> --- a/net/tls/trace.h
> +++ b/net/tls/trace.h
> @@ -192,6 +192,104 @@ 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	)
> +	),

[ ... ]