Re: [PATCH net-next v4 6/9] tls: Return copied data ahead of a run of empty records

[netdev-bot+sashiko@kernel.org]

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] This patch fixes only the case where the reader has already
  met the low-water mark.

--

tls: Return copied data ahead of a run of empty records

This commit makes tls_sw_recvmsg() leave its receive loop at a
zero-length application_data record once the bytes copied so far meet the
receive low-water mark. Before this change, a peer that sent a stream of
empty records after data could keep the reader in the loop. The reader
held the copied data until the stream stopped or a signal was pending.

Link: https://lore.kernel.org/r/179124254665.434549.12245680127262431659@kernel.org

> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index b7790b17f79c6..15a3a6d3f6380 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: Medium]

Can a non-blocking reader still be held here when it has copied some data
but is still below target?

target is computed without regard to MSG_DONTWAIT:

	target = sock_rcvlowat(sk, flags & MSG_WAITALL, len);

So with MSG_DONTWAIT|MSG_WAITALL, or with MSG_DONTWAIT and SO_RCVLOWAT set
above the bytes copied so far, a reader can end up with
0 < decrypted + copied < target.

In that state the new check is false, so the empty record goes to
continue. Empty records advance neither len nor decrypted, so the loop
condition stays true:

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

For a non-blocking reader, the usual way out with its partial data is the
-EAGAIN return in tls_rx_rec_wait():

	while (!tls_strp_msg_ready(ctx)) {
		...
		if (!timeo)
			return -EAGAIN;
		...
	}
	...
	return 1;

That check only runs when no record is ready. If the peer has an empty
record ready each time, tls_rx_rec_wait() returns 1. The empty record is
then consumed and the loop runs again. Without the empty records, the
same reader would get -EAGAIN right away, and recv_end would return the
copied bytes.

The reader keeps the copied data until the run of empty records stops or
the nodata && signal_pending() check fires. That is the same delay this
patch fixes for readers that have met target.

The commit message says:

    A reader that has copied nothing still reads empty records
    for as long as they arrive.

This reads as if readers that have copied nothing are the only case left.
The subject line also does not hold for the readers described above.

Later patches in this series do not appear to touch net/, so this case
would still be present after the series.

Would it make sense to also break for a non-blocking read that has
already copied some data? For example:

	if (decrypted + copied >= target ||
	    ((flags & MSG_DONTWAIT) && decrypted + copied))
		break;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012-tls-follow-on-v4-0-8cea429565da%40kernel.org