Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · 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). - [Medium] tls_sw_splice_read() (net/tls/tls_sw.c) now has a `goto retry` loop that a peer can keep running. -- tls: Consume empty data records in tls_sw_splice_read() Before this commit, tls_sw_splice_read() returned zero when it hit a zero-length application_data record, and callers read that as EOF. It now consumes the empty record and jumps back to fetch the next one. A signal_pending() check before each retry keeps the splicing task killable. Closes: https://lore.kernel.org/netdev/akaoXcfamBp8_mYe@krikkit/ > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c > index 6c21897b03ee5..ff82e2f4e9e52 100644 > --- a/net/tls/tls_sw.c > +++ b/net/tls/tls_sw.c [ ... ] > @@ -2038,12 +2039,13 @@ ssize_t tls_sw_splice_read(struct socket *sock, > loff_t *ppos, > if (err) > goto splice_read_end; > > +retry: > if (!skb_queue_empty(&ctx->rx_list)) { > skb = __skb_dequeue(&ctx->rx_list); > } else { > struct tls_decrypt_arg darg; > > - err = tls_rx_rec_wait(sk, nonblock, true, false); > + err = tls_rx_rec_wait(sk, nonblock, released, false); > if (err <= 0) > goto splice_read_end; > [ ... ] > @@ -2066,6 +2071,16 @@ ssize_t tls_sw_splice_read(struct socket *sock, > loff_t *ppos, > goto splice_requeue; > } > > + /* Splicing zero bytes reads as EOF to the caller. */ > + if (rxm->full_len == 0) { > + consume_skb(skb); > + if (signal_pending(current)) { > + err = sock_intr_errno(sock_rcvtimeo(sk, nonblock)); > + goto splice_read_end; > + } > + goto retry; > + } [Severity: Medium] Can a peer keep this goto retry loop running for as long as it likes? Nothing limits how many empty records one call consumes, and nonblock is not checked between retries. The loop only exits on one of these: - a non-empty data record - a control record - tls_rx_rec_wait() returning <= 0 - a pending signal Suppose the peer sends authenticated empty records at least as fast as they are decrypted. Then the next record is always ready, through one of two paths: - tls_rx_rec_done()->tls_strp_check_rcv() - sk_flush_backlog() followed by tls_strp_check_rcv() inside tls_rx_rec_wait() In that case tls_rx_rec_wait() either skips its wait loop or breaks out before it reaches the -EAGAIN exit: net/tls/tls_sw.c:tls_rx_rec_wait() { ... timeo = sock_rcvtimeo(sk, nonblock); while (!tls_strp_msg_ready(ctx)) { ... if (!skb_queue_empty(&sk->sk_receive_queue)) { tls_strp_check_rcv(&ctx->strp, false); if (tls_strp_msg_ready(ctx)) break; } ... if (!timeo) return -EAGAIN; ... } If so, would a splice(2) or sendfile(2) caller using SPLICE_F_NONBLOCK or O_NONBLOCK never see -EAGAIN, and never get back to its event loop while the stream continues? The earlier patch in this series, "tls: Honor O_NONBLOCK in tls_sw_splice_read()", was meant to prevent this kind of event loop stall. The task also holds lock_sock() and the TLS reader lock for the whole run. Other users of the socket, such as tls_sw_sendmsg(), setsockopt() and recvmsg(), would be blocked during that time. Blocking callers have a similar problem. Each call to tls_rx_rec_wait() computes a fresh timeo from sock_rcvtimeo(). Could one empty record arriving just before each timeout expires keep a splice with SO_RCVTIMEO waiting forever? Elsewhere in this series, tls_sw_read_sock() caps the same pattern: if (++nodata >= TLS_RX_NODATA_LIMIT) { At the end of the series, tls_sw_splice_read() still has no cap. Would it make sense to cap consecutive empty records here as well? One option is to reuse TLS_RX_NODATA_LIMIT and return -EAGAIN to nonblocking callers. Another is to carry a single remaining-timeout budget across retries. tls_sw_recvmsg() has the same unbounded behaviour. The patch "tls: Consume empty data records in tls_sw_recvmsg()" acknowledges this. -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007-tls-follow-on-v3-0-6d52d98389b3%40kernel.org
