Re: [PATCH net-next v2 5/8] tls: consume empty data records in tls_sw_recvmsg()

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] The patch stops empty records from piling up on rx_list, but the
  receive loop in tls_sw_recvmsg() (net/tls/tls_sw.c) still never…

--

tls: consume empty data records in tls_sw_recvmsg()

This patch makes tls_sw_recvmsg() free a zero-length application_data
record as soon as it is decrypted. Before, the peek and async arms queued
it on rx_list. Empty records are no longer decrypted asynchronously, and
the new branch sets MSG_EOR itself.

[ ... ]

[Severity: High]
This keeps empty records off rx_list. Does the receive loop still fail to
make progress on them, though? The commit message describes this part of
the problem as well:

    len never reaches zero, and tls_strp_msg_ready() keeps the second
    loop term true while records keep arriving.

The new branch does not change len, decrypted or copied. It also places no
limit on how many empty records can arrive in a row. Once
decrypted + copied >= target, only the strparser keeps the loop going:

    while (len && (decrypted + copied < target || tls_strp_msg_ready(ctx))) {

Could a peer that keeps sending zero-length application_data records hold
the reader in this loop forever? Three things keep the loop supplied:

  tls_rx_rec_done()->tls_strp_check_rcv() parses the next record right
  away.

  tls_read_flush_backlog() flushes the backlog on every pass whenever
  tcp_inq(sk) <= max_rec, because done never moves:

      if (done - *flushed_at < SZ_128K && tcp_inq(sk) > max_rec)
          return false;

      *flushed_at = done;
      return sk_flush_backlog(sk);

  tls_rx_rec_wait() returns 1 at once when a record is ready, so the
  loop never reaches its -EAGAIN exit for nonblocking callers.

So during such a flood, a MSG_DONTWAIT caller would not get -EAGAIN. Bytes
already copied would not be returned until the flood stops.

SO_RCVTIMEO readers have a related problem. tls_rx_rec_wait() starts a new
timeout on every call:

    timeo = sock_rcvtimeo(sk, nonblock);

    while (!tls_strp_msg_ready(ctx)) {

If a reader has copied nothing yet, can a peer that sends one empty record
per timeout period stop it from ever timing out?

The only way out seems to be the check added earlier in this series:

    if (nodata && signal_pending(current)) {

In-kernel MSG_DONTWAIT callers never receive a signal, so they cannot use
this exit. One example is svc_tcp_sock_recvmsg() in net/sunrpc/svcsock.c:

    ret = sock_recvmsg(sock, msg, MSG_DONTWAIT);

This same series already limits tls_sw_read_sock() with TLS_RX_NODATA_LIMIT
in "tls: bound consecutive no-data records in tls_sw_read_sock()". The
later patches in the series only change selftests, so tls_sw_recvmsg()
still has no limit at the end of the series.

The zero-copy arm could already spin like this before this patch. Since
the commit message names this loop behavior as the defect, should the
empty-record branch break out once decrypted + copied >= target? The other
option would be a limit on consecutive empty records, as tls_sw_read_sock()
has.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261001-tls-follow-on-v2-0-2dd1947bb642%40kernel.org