Re: [PATCH net-next v6 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 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/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 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