Thank you for your contribution! Sashiko AI review found more severe issue(s) in other patches of this series. This mail is only an FYI, in case you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration: - [Low] The commit message gives a reason for the new MSG_EOR: "The new branch sets MSG_EOR itself, because the record no longer reaches the… Pre-existing issues: - [High] This problem wasn't introduced by this patch, but tls_sw_recvmsg() still has no limit on how many consecutive empty… - [Medium] This problem wasn't introduced by this patch, but tls_sw_recvmsg() still returns 0 (read as EOF) when empty data records… -- tls: Consume empty data records in tls_sw_recvmsg() This commit adds a branch to tls_sw_recvmsg() that frees a zero-length application_data record right after it is decrypted. The branch runs before the zero-copy, peek and async paths diverge, so empty records are no longer queued on rx_list. The branch sets MSG_EOR itself and then continues the receive loop. Closes: https://sashiko.dev/#/patchset/[email protected]?part=1 > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c > index 53a4af9eeeefa..153ff135a395e 100644 > --- a/net/tls/tls_sw.c > +++ b/net/tls/tls_sw.c [ ... ] > @@ -1929,6 +1929,19 @@ int tls_sw_recvmsg(struct sock *sk, > nodata = !chunk; > tls_rx_rec_done(ctx); > > + /* Keep an empty record off rx_list. On the zero-copy path > + * the strparser owns darg.skb, and tls_rx_rec_done() has > + * released it. > + */ > + if (!chunk && control == TLS_RECORD_TYPE_DATA) { [Severity: Medium] This isn't a bug introduced by this patch, but can tls_sw_recvmsg() still return 0 when an empty data record is followed by a non-data record? When this branch runs, tls_record_content_type() has already set control to TLS_RECORD_TYPE_DATA for the empty record and emitted a TLS_GET_RECORD_TYPE cmsg. The next record might be an alert such as close_notify, or a TLS 1.3 handshake record such as KeyUpdate or NewSessionTicket. In that case tls_record_content_type() hits this: net/tls/tls_sw.c:tls_record_content_type() { ... } else if (*control != tlm->control) { return 0; } ... } and tls_sw_recvmsg() takes the err <= 0 branch with err == 0: err = tls_record_content_type(msg, tls_msg(darg.skb), &control); if (err <= 0) { DEBUG_NET_WARN_ON_ONCE(darg.zc); tls_rx_rec_done(ctx); put_on_rx_list_err: __skb_queue_tail(&ctx->rx_list, darg.skb); goto recv_end; } Nothing has been copied and async is false, so "return copied ? : err;" returns 0 with a DATA cmsg attached. Callers such as OpenSSL treat that as EOF. The control record stays on rx_list, so the next recvmsg() does return it. The sibling patch "tls: Consume empty data records in tls_sw_splice_read()" removes this false EOF for splice. The upstream selftest variant zero_len/0data_0data_ctrl currently expects recv_ret {0, 11, -EAGAIN}, so the recvmsg behaviour is encoded there. Could the empty record be consumed before control is latched? If not, could the commit message say that recvmsg keeps this behaviour? > + if (!darg.zc) > + consume_skb(darg.skb); > + > + /* An empty record still marks a boundary. */ > + msg->msg_flags |= MSG_EOR; [Severity: Low] The commit message says: "The new branch sets MSG_EOR itself, because the record no longer reaches the assignment at the bottom of the loop." Is that accurate for the peek and async arms? Before this patch, an empty record on the peek arm went through this: if (is_peek) { peeked += chunk; goto put_on_rx_list; } The async arm also queued the skb and hit continue. Neither arm reached the msg->msg_flags |= MSG_EOR assignment at the bottom of the loop. This branch now runs before those arms. So a MSG_PEEK recvmsg() with synchronous crypto that reaches an empty data record now reports MSG_EOR, which it did not do before. On the async arm there is little visible change, because process_rx_list() at recv_end already sets MSG_EOR for fully copied records. Could the commit message mention this flag change on the peek path? > + continue; [Severity: High] This is a pre-existing issue, but should this branch limit the number of consecutive empty records, like the other readers in this series do? There is no counter before this continue. TLS_RX_NODATA_LIMIT is defined after tls_sw_recvmsg(), and only tls_sw_splice_read() and tls_sw_read_sock() use it: if (++nodata >= TLS_RX_NODATA_LIMIT) { err = -EAGAIN; goto splice_read_end; } The later patch "tls: Return copied data ahead of a run of empty records" adds a break once decrypted + copied >= target. That patch also says "A reader that has copied nothing still reads empty records for as long as they arrive." Empty records advance neither len nor decrypted. While decrypted + copied < target, this loop condition stays true: while (len && (decrypted + copied < target || tls_strp_msg_ready(ctx))) { The only exit from the run is then: if (nodata && signal_pending(current)) { While tls_strp_msg_ready() is true, tls_rx_rec_wait() returns 1 without sleeping or checking for signals. It also recomputes timeo on every call. Because decrypted + copied never advances, tls_read_flush_backlog() does not see progress either, and sk_flush_backlog() keeps sk_lock owned. As a result, a peer that streams empty records keeps the reader inside recvmsg() with the socket lock owned. That defeats MSG_DONTWAIT and SO_RCVTIMEO, and blocks sendmsg, setsockopt and close on the socket. Kernel callers cannot use the signal exit. For example, nfsd reaches this loop through: svc_tcp_sock_recvmsg() sock_recvmsg(sock, msg, MSG_DONTWAIT) tls_sw_recvmsg() The read_sock patch in this series treats the same behaviour as a bug: "The caller cannot bound the run because read_sock() has not returned". Would the same TLS_RX_NODATA_LIMIT cap make sense in tls_sw_recvmsg()? > + } > + > if (!darg.zc) { [ ... ] -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014-tls-follow-on-v5-0-e76c301d4f05%40kernel.org
