Re: [PATCH net-next v17 10/15] net/mlx5e: flag TLS RX records that failed device decryption

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 · 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).

- [High] mlx5e_ktls_handle_rx_skb()
  (drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_rx.c:614) sets…

--

net/mlx5e: flag TLS RX records that failed device decryption

The patch splits CQE_TLS_OFFLOAD_ERROR out of the default case in
mlx5e_ktls_handle_rx_skb() and sets the new skb->decrypt_failed bit while
leaving skb->decrypted clear, then falls through to the existing tls_err
accounting. The bit is intended to be consumed by the TLS 1.3 device-offload
RX KeyUpdate support added later in the series.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_rx.c
> index bca45679e201..8ec40f5fd5b5 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_rx.c
> @@ -602,7 +602,18 @@ void mlx5e_ktls_handle_rx_skb(struct mlx5e_rq *rq, struct sk_buff *skb,
>  		stats->tls_resync_req_pkt++;
>  		resync_update_sn(rq, skb);
>  		break;
> -	default: /* CQE_TLS_OFFLOAD_ERROR: */
> +	case CQE_TLS_OFFLOAD_ERROR:
> +		/* The device could not authenticate the payload. Depending on
> +		 * where the failure occurred the bytes may have been transformed
> +		 * (XORed) or left as wire ciphertext. Flag it so that, during a
> +		 * TLS 1.3 rekey transition, the re-encrypt path undoes the
> +		 * transform on any XORed frag of a mixed record while software
> +		 * re-authenticates; a non-mixed record stays wire ciphertext and
> +		 * is decrypted directly.
> +		 */
> +		skb->decrypt_failed = 1;

[Severity: High]

Can this per-skb distinction survive coalescing?  The bit is set here with
skb->decrypted left at 0, but the only comparator the stack uses to decide
whether two skbs may be merged still looks at decrypted alone:

include/linux/skbuff.h:skb_cmp_decrypted() {
	...
	return skb2->decrypted - skb1->decrypted;
	...
}

So a segment flagged here and a segment the device never touched compare
equal.  That comparator gates the merge in tcp_gro_receive():

	flush |= skb_cmp_decrypted(p, skb);

and in tcp_skb_can_collapse_rx(), used by tcp_try_coalesce(),
tcp_collapse() and tcp_add_backlog():

	return likely(mptcp_skb_can_collapse(to, from) &&
		      !skb_cmp_decrypted(to, from));

After such a merge only one flag value survives for bytes contributed by
several skbs, since tcp_collapse() copies it from the first skb only:

		skb_copy_decrypted(nskb, skb);

Note that the prerequisite patch extended skb_copy_decrypted() to propagate
decrypt_failed but left skb_cmp_decrypted() unchanged.  Is that asymmetry
intentional?

The same comparator feeds the mixed-record notion this commit message relies
on:

net/tls/tls_strp.c:tls_strp_copyin() {
	...
	strp->mixed_decrypted |= !!skb_cmp_decrypted(skb, in_skb);
	...
}

If two segments of one record differ only in decrypt_failed, the record is
never classified as mixed.  Does the consumer added later in the series then
act on the wrong value?  It restores a whole frag based on the single bit:

net/tls/tls_device.c:tls_device_reencrypt() {
	...
		if (skb_iter->decrypted || skb_iter->decrypt_failed) {
			err = skb_store_bits(skb_iter, frag_pos, buf, copy);
	...
}

For a merged skb covering both XORed bytes and untouched wire ciphertext,
does this either undo the transform on bytes the device never touched, or
leave the transform in place on bytes it did XOR, so that software AEAD
authentication fails with -EBADMSG?

A second consequence is in the rekey boundary tracking:

net/tls/tls_device.c:tls_device_decrypted() {
	...
		bool nic_touched = !is_encrypted || skb->decrypt_failed;
	...
}

If a record the device never processed inherits decrypt_failed from a
GRO- or collapse-merged neighbour, does ctx->rekey.old_nic_boundary slide
past that record, keeping the retired old AEAD alive and postponing the
deferred tls_dev_add()?

The series documentation appears to describe exactly this mismatch while at
the same time permitting the merge:

Documentation/networking/tls-offload.rst
	Coalescing (GRO, socket layer) and record classification are keyed on
	decrypted alone, so decrypt_failed segments may be merged with unmarked
	ones. ... A transformed segment delivered without decrypt_failed, or an
	untransformed segment of a mixed record delivered with it, is restored
	incorrectly and the record fails software authentication.

Since GRO and TCP coalescing run after mlx5e_ktls_handle_rx_skb() has set
the bit, can the driver honour that contract at all?  Would extending
skb_cmp_decrypted() to also compare decrypt_failed (and thus forbidding
coalescing across the new bit) be needed before this flag can be relied on?

The segmentation and arrival pattern is driven by peer traffic and a
peer-initiated KeyUpdate, so the merge condition is reachable from a remote
endpoint.

For reference, skb_cmp_decrypted() is unchanged at the end of the series, so
the gap does not appear to be closed by a later patch.

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