Re: [PATCH v2 3/3] nvme-tcp: consume TLS records without application data

[Martin Belanger <martin.nvme.dev@gmail.com>]

On 11/12/26 07:32, Hannes Reinecke wrote:
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
[...]
> +             if (buf[0] == TLS_HANDSHAKE_KEYUPDATE) {
> +                     dev_err(queue->ctrl->ctrl.device,
> +                             "queue %d: peer sent a TLS KeyUpdate,
which is not supported\n",
>
> A bit awkward; maybe 'TLS KeyUpdate is not supported'?

Yes. v3 will use "queue %d: TLS KeyUpdate is not supported".

[...]
> +     ret = nvme_tcp_drain_tls_record(queue, &msg, MSG_DONTWAIT);
>
> Hmm. Doesn't this assume that the KeyUpdate is always the first record?

It assumes that the KeyUpdate is the first handshake message in its
record. RFC 8446 also allows a KeyUpdate after another handshake message
in the same record. kTLS has the same gap: tls_check_pending_rekey()
reads only the first byte of the record. v7 of the kTLS KeyUpdate series
will check every handshake message in the record, so kTLS will detect
this case.

nvme-tcp needs no change for it, because it cannot answer a KeyUpdate in
any case. If kTLS misses the KeyUpdate, the peer's next record uses the
new key, decryption fails, and the queue goes into error recovery. The
queue fails in both cases. Only the log message differs.

> And we do have similar logic in nvme_tcp_skip_tls_record(); can't we
> merge both (ie just call nvme_tcp_drain_tls_record(), and drop
> this switch statement?

v3 will merge them. nvme_tcp_skip_tls_record() goes away, and the ICResp
path and the live path call one helper.

I would like to keep the switch, for the log messages. Draining alone
works: kTLS sets key_update_pending when it decrypts the KeyUpdate, so
the next read_sock() returns -EKEYEXPIRED. After a fatal alert, the peer
closes the connection. But then the log only shows "receive failed". The
switch logs the KeyUpdate and the alert description. The alert is often
the only hint why a target dropped the connection.

One change for the ICResp path: a warning alert ahead of the ICResp no
longer fails the connection. If the peer closes the connection after the
alert, the next read fails.

Thanks,
Martin