Re: [PATCH net-next v3 2/9] tls: Check for a pending signal after an empty record

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · 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).

- [High] In tls_sw_recvmsg() (net/tls/tls_sw.c), the only new way out of
  a run of empty application_data records is `if (nodata &&…
- [Medium] The new check in tls_sw_recvmsg() runs before
  tls_rx_rec_wait() and before any `!timeo` test.

--

tls: Check for a pending signal after an empty record

This commit adds a signal_pending() check to tls_sw_recvmsg(). The check
runs before the next record is fetched after a zero-length
application_data record, so a stream of empty records cannot hold the
caller in recvmsg() regardless of signals. The commit also decrypts empty
records synchronously, so the error from that check is not overwritten at
recv_end.

Closes: https://sashiko.dev/#/patchset/20260720-tcp-read-sock-v2-0-29545d034f3c@kernel.org?part=1

> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index 275a6047da9e3..32be5a8f80ed9 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c

[ ... ]

> @@ -1858,6 +1859,16 @@ 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 it sleeps.
> +		 */
> +		if (nodata && signal_pending(current)) {
> +			long timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
> +
> +			err = sock_intr_errno(timeo);
> +			goto recv_end;
> +		}

[Severity: Medium]

Should a nonblocking reader get -EAGAIN here instead of -EINTR? With
MSG_DONTWAIT, sock_rcvtimeo(sk, true) returns 0, so sock_intr_errno(0)
returns -EINTR. This check also runs before any !timeo test.

Elsewhere the nonblocking -EAGAIN check comes first. For example,
tls_rx_reader_acquire() does:

    if (timeo <= 0)
        return -EAGAIN;
    if (signal_pending(current))
        return sock_intr_errno(timeo);

tls_rx_rec_wait() and tcp_recvmsg_locked() also test !timeo before they
check for signals.

signal_pending() is also true when only TIF_NOTIFY_SIGNAL is set. io_uring
rings without DEFER_TASKRUN queue task_work with TWA_SIGNAL or
TWA_SIGNAL_NO_IPI, and that sets TIF_NOTIFY_SIGNAL on the submitting task.
io_recv() and io_recvmsg() issue inline with MSG_DONTWAIT and retry via
poll only on -EAGAIN. Any other error fails the request:

    if (ret == -ERESTARTSYS)
        ret = -EINTR;
    req_set_fail(req);

Suppose an empty record arrives from the peer while task_work is queued to
the submitter. Could that now fail a nonblocking recv, or end a multishot
recv, with -EINTR even though no real signal was delivered? Before this
patch, the record loop did not return -EINTR to a nonblocking reader.

The same pattern also shows up in tls_sw_splice_read() later in this
series.

> +
>  		err = tls_rx_rec_wait(sk, flags & MSG_DONTWAIT,
>  				      released, !!(decrypted + copied));
>  		if (err <= 0)

[ ... ]

> @@ -1910,6 +1924,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);

[Severity: High]

Does this put a limit on a run of empty records when the reader has copied
nothing? nodata is only used together with signal_pending(). Nothing
counts consecutive empty records or checks a timeout. With
decrypted + copied == 0, the loop condition stays true:

    while (len && (decrypted + copied < target || tls_strp_msg_ready(ctx))) {

For MSG_DONTWAIT or O_NONBLOCK readers, tls_rx_rec_wait() returns 1
immediately whenever tls_strp_msg_ready() is true, so it never reaches
this check:

    if (!timeo)
        return -EAGAIN;

Because done stays 0, tls_read_flush_backlog() also keeps flushing the
backlog while tcp_inq(sk) <= max_rec, which pulls in more records during
the same call.

If a peer sends empty records faster than they can be decrypted, could it
keep an event-loop thread inside recvmsg() indefinitely, with no signal
ever arriving?

For blocking readers with SO_RCVTIMEO, tls_rx_rec_wait() recomputes
timeo = sock_rcvtimeo(sk, nonblock) on every call. Each empty record
therefore restarts the full timeout. By contrast, tcp_recvmsg_locked()
uses one timeo for the whole call.

Could one empty record per timeout period stop SO_RCVTIMEO from ever
expiring?

The commit message says "a nonblocking reader gets -EINTR". It seems a
nonblocking reader is still held unless a signal happens to arrive.

tls_sw_read_sock() in the same file already caps this case:

    if (++nodata >= TLS_RX_NODATA_LIMIT) {

Would tls_sw_recvmsg() need a similar limit, or a check against an overall
timeout?

A later commit in this series, "tls: Return copied data ahead of a run of
empty records", breaks out of the loop when decrypted + copied >= target.
That covers readers that have already met their target. Another later
commit message in the series says "A reader that has copied nothing still
reads empty records for as long as they arrive.", so the copied-nothing
cases above still look open at the end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007-tls-follow-on-v3-0-6d52d98389b3%40kernel.org