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] When the new TLS_RX_NODATA_LIMIT break fires,
tls_sw_read_sock() queues data_ready_work only if…
--
tls: Bound consecutive no-data records in tls_sw_read_sock()
This patch stops tls_sw_read_sock() after TLS_RX_NODATA_LIMIT consecutive
zero-length data records. If a parsed record is ready when the loop stops,
a work item on tls_strp_wq runs the consumer's current sk_data_ready(),
because tls_rx_reader_release() only wakes saved_data_ready.
> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index 312e51270f29..0ef0edd51edd 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
[ ... ]
> @@ -2144,6 +2161,16 @@ int tls_sw_read_sock(struct sock *sk,
> read_descriptor_t *desc,
> */
> if (rxm->full_len == 0) {
> consume_skb(skb);
> + if (++nodata >= TLS_RX_NODATA_LIMIT) {
> + /* tls_rx_reader_release() wakes only the
> + * callback saved when TLS_RX was set up,
> + * not one installed since.
> + */
> + if (tls_strp_msg_ready(ctx))
> + queue_work(tls_strp_wq,
> + &ctx->data_ready_work);
> + break;
> + }
[Severity: Medium]
Can this break return 0 while the next record is still being parsed, with
nothing left to wake nvme-tcp?
data_ready_work is only queued if msg_ready is already set. For the 16th
empty record, tls_rx_rec_done() calls tls_strp_check_rcv(&ctx->strp, false).
If tls_strp_read_sock() returns -ENOMEM, the parse is handed off to
strp->work and msg_ready stays 0:
net/tls/tls_strp.c:tls_strp_check_rcv() {
...
if (tls_strp_read_sock(strp) == -ENOMEM)
queue_work(tls_strp_wq, &strp->work);
...
}
That -ENOMEM can come from alloc_page(sk->sk_allocation) in
tls_strp_read_copy(), and nvme-tcp sets sk_allocation to GFP_ATOMIC. The
copy path runs in copy_mode, or when tls_strp_check_queue_ok() fails on a
fully received record.
Once the loop breaks, tls_strp_msg_ready(ctx) is false and the work item is
not queued. tls_rx_reader_release() calls tls_rx_msg_maybe_announce(),
which also returns early:
if (!READ_ONCE(strp->msg_ready) || strp->msg_announced)
return;
Later, tls_strp_work() parses the record via tls_strp_check_rcv(strp, true).
It announces the record only through ctx->saved_data_ready, which for
nvme-tcp is the pre-handshake callback and not nvme_tcp_data_ready(). The
commit message makes the same point.
nvme_tcp_io_work() sees a 0 result from read_sock and does not requeue
itself. Does the fully received record then sit until another TCP segment
arrives or the I/O times out?
Before this patch, the loop went back into tls_rx_rec_wait(). With
sk_receive_queue non-empty, that retried tls_strp_check_rcv()
synchronously, so this state needed two allocation failures in a row. With
the early break, a single failure is enough.
A related case, with empty records left on ctx->rx_list by MSG_PEEK, can be
reached at this commit. It goes away later in the series, because "tls:
Consume empty data records in tls_sw_recvmsg()" keeps empty records off
rx_list.
> continue;
> }
>
[ ... ]
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261019-tls-follow-on-v6-0-ab01c4ecca0c%40kernel.org
