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 new early exit in tls_sw_recvmsg() (net/tls/tls_sw.c:1946, `if (decrypted + copied >= target) break;`) only fires once the low-… -- tls: Return copied data ahead of a run of empty records This patch makes tls_sw_recvmsg() leave its receive loop at a zero-length application_data record once the receive low-water mark has been met. The goal is to stop a stream of empty records from holding back data that has already been copied. A reader that has copied nothing keeps reading empty records for as long as they arrive. Link: https://lore.kernel.org/r/[email protected] > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c > index 153ff135a395e..9b4e66567ee8d 100644 > --- a/net/tls/tls_sw.c > +++ b/net/tls/tls_sw.c > @@ -1939,6 +1939,12 @@ int tls_sw_recvmsg(struct sock *sk, > > /* An empty record still marks a boundary. */ > msg->msg_flags |= MSG_EOR; > + > + /* Return data already copied instead of holding it > + * through a run of empty records. > + */ > + if (decrypted + copied >= target) > + break; > continue; [Severity: High] Does this leave the same stall in place for a reader that is still below target? When SO_RCVLOWAT is above the bytes copied so far, or MSG_WAITALL makes target equal len, the check above is false. The empty record then takes the continue. An empty record changes neither len nor decrypted, so the loop condition stays true: while (len && (decrypted + copied < target || tls_strp_msg_ready(ctx))) { Each pass calls tls_rx_rec_wait(). That function recomputes the timeout on every call and skips its wait loop while a record is ready: timeo = sock_rcvtimeo(sk, nonblock); while (!tls_strp_msg_ready(ctx)) { ... if (!timeo) return -EAGAIN; ... } As a result, MSG_DONTWAIT, O_NONBLOCK and SO_RCVTIMEO never come into play. The only way out is this check at the top of the loop: if (nodata && signal_pending(current)) { So if a peer keeps validly encrypted empty application_data records ready, can a nonblocking or timed reader with partial data stay in the kernel? It would hold the socket and reader locks and withhold the data it has already copied. The commit message describes that outcome as "the reader does not get the data already copied until the stream stops or a signal is pending". The same applies to a reader that has copied nothing, which the commit message mentions. tls_sw_splice_read() and tls_sw_read_sock() in the same file already stop a run of empty records with a counter: if (++nodata >= TLS_RX_NODATA_LIMIT) { err = -EAGAIN; goto splice_read_end; } Would a consecutive-empty-record cap like TLS_RX_NODATA_LIMIT in tls_sw_recvmsg() also cover the below-target and zero-copied readers? It would also cover the case this patch handles. TLS_RX_NODATA_LIMIT is currently defined after tls_sw_recvmsg(), so it would need to move up. > } > -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014-tls-follow-on-v5-0-e76c301d4f05%40kernel.org
