Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider.
Critical: 0 · High: 1 (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 only new exit from a run of empty records in tls_sw_recvmsg() is `if (nodata && signal_pending(current))`. Pre-existing issues: - [High] At this commit, a blocking recvmsg(MSG_PEEK) queues every zero- length application_data record on ctx->rx_list as a cleartext skb… -- tls: Check for a pending signal after an empty record This commit adds a signal_pending() test in tls_sw_recvmsg() before the next record is fetched after a zero-length application_data record. The goal is that a peer streaming empty records cannot hold the reader in recvmsg() indefinitely. Nonblocking readers with a signal pending get -EAGAIN, and empty records are always decrypted synchronously. Closes: https://sashiko.dev/#/patchset/[email protected]?part=1 > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c > index 0ef0edd51edd..e8a9f0c91e81 100644 > --- a/net/tls/tls_sw.c > +++ b/net/tls/tls_sw.c [ ... ] > @@ -1858,6 +1867,14 @@ int tls_sw_recvmsg(struct sock *sk, > struct tls_decrypt_arg darg; > int to_decrypt, chunk; > > + /* A run of empty records advances neither loop bound, and > + * tls_rx_rec_wait() tests for a signal only after sleeping. > + */ > + if (nodata && signal_pending(current)) { > + err = tls_rx_signal_err(sk, flags & MSG_DONTWAIT); > + goto recv_end; > + } [Severity: High] Does this close the hang for callers that never receive a signal? An empty record changes neither len nor decrypted, so the loop condition stays true: while (len && (decrypted + copied < target || tls_strp_msg_ready(ctx))) { When another record is already parsed, tls_rx_rec_wait() skips its wait loop entirely. Neither its nonblocking exit nor SO_RCVTIMEO is reached: while (!tls_strp_msg_ready(ctx)) { ... if (!timeo) return -EAGAIN; ... } That leaves signal_pending() as the only way out of a run of empty records. Two kinds of caller appear to still be held for as long as the records keep arriving: - nonblocking readers (MSG_DONTWAIT or O_NONBLOCK) with no signal pending - in-kernel callers that do not take signals. One is nfsd threads, via svc_tcp_sock_recvmsg()->sock_recvmsg(sock, msg, MSG_DONTWAIT) on kTLS sockets (there is no allow_signal() in fs/nfsd/). Another is rpciod workers, via xs_read_stream(transport, MSG_DONTWAIT). In the nfsd case the thread spins in tls_sw_recvmsg() with the socket lock held, so kthread_stop() is blocked as well. The commit message says tls_sw_read_sock() has "callers that cannot act on a signal". The earlier commit "tls: Bound consecutive no-data records in tls_sw_read_sock()" bounds that path with TLS_RX_NODATA_LIMIT. tls_sw_recvmsg() has callers of the same kind, but it gets no count bound and no nonblocking exit. The later commit "tls: Return copied data ahead of a run of empty records" adds a break when decrypted + copied >= target. That covers a reader that already has data. The case where nothing was copied remains, and the series says so: "A reader that has copied nothing still reads empty records for as long as they arrive". Could the run also be ended for nonblocking readers? One option is to return -EAGAIN after an empty record. Another is to stop after a bounded count, as tls_sw_read_sock() does. > + > err = tls_rx_rec_wait(sk, flags & MSG_DONTWAIT, > released, !!(decrypted + copied)); > if (err <= 0) [ ... ] > @@ -1910,6 +1930,7 @@ int tls_sw_recvmsg(struct sock *sk, > /* TLS 1.3 may have updated the length by more than overhead */ > rxm = strp_msg(darg.skb); > chunk = rxm->full_len; > + nodata = !chunk; > tls_rx_rec_done(ctx); > > if (!darg.zc) { [Severity: High] This is a pre-existing issue and was not introduced by this patch. Can a blocking recvmsg(MSG_PEEK) grow ctx->rx_list without bound here? MSG_PEEK disables zero-copy, so every empty application_data record takes the peek arm further down in tls_sw_recvmsg(): if (is_peek) { peeked += chunk; goto put_on_rx_list; } ... put_on_rx_list: decrypted += chunk; len -= chunk; __skb_queue_tail(&ctx->rx_list, skb); With chunk == 0, neither decrypted nor len advances. The loop keeps accepting records and queues a cleartext skb for each one. tcp_read_done() has already released the TCP-side data, so TCP flow control does not push back on the peer. The later commit "tls: Consume empty data records in tls_sw_recvmsg()" fixes this. Its message says "The peek arm and the async arm queue each empty record on rx_list, and a peer that streams empty records grows rx_list without bound". At the end of the series, the empty-record branch consumes the skb before the peek arm is reached. The async arm also cannot queue empty records at that point. TLS 1.2 empty records are decrypted synchronously, and async is disabled for TLS 1.3. -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261019-tls-follow-on-v6-0-ab01c4ecca0c%40kernel.org
