Re: [PATCH net-next v3 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 more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit "tls: Return copied data ahead of a run of empty
  records" fixes a delay a remote peer can cause in tls_sw_recvmsg(),…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the new
  early exit in tls_sw_recvmsg() only fires when `decrypted + copied >=…

--

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

This patch makes tls_sw_recvmsg() leave the receive loop at a zero-length
application_data record once the receive low-water mark has been met. Data
that has already been copied then goes back to the reader. Before this, it
stayed in the loop while a peer kept sending empty records.

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

> Suggested-by: sashiko-bot <netdev-bot+sashiko@kernel.org>
> Link: https://lore.kernel.org/r/179124254665.434549.12245680127262431659@kernel.org
> Signed-off-by: Chuck Lever <cel@kernel.org>

[Severity: Low]
Should this commit carry a Fixes: tag?

The commit message describes a delay that a remote peer can cause, but the
trailers only contain Suggested-by and Link.

Every other kernel patch in this series has a Fixes: tag. That includes
"tls: Check for a pending signal after an empty record", which is also
described as not reproduced and uses Fixes: c46234ebb4d1.

The behavior predates the series. Before "tls: Consume empty data records
in tls_sw_recvmsg()", an empty record went through the normal path with
decrypted += 0 and len -= 0. The || tls_strp_msg_ready(ctx) loop condition
then kept the reader in the loop.

That parent commit carries Fixes: 692d7b5d1f91. It also adds the
!chunk && control == TLS_RECORD_TYPE_DATA branch that this patch changes.

Without a tag here, would stable backports that take the parent commit
miss this follow-up?

> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index b7f3edf9828e0..fd6f944ffd677 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -1937,6 +1937,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]
This is a pre-existing issue and was not introduced by this patch. What
happens to a reader that has copied some data but is still below target?

In tls_sw_recvmsg(), target comes from:

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

Under MSG_WAITALL, including MSG_DONTWAIT | MSG_WAITALL, that is the full
len. With SO_RCVLOWAT set, it can also be larger than what was copied.

Such a reader still takes the continue on every empty DATA record.
Neither decrypted nor len advances.

The timeout doesn't seem to limit this. tls_rx_rec_wait() recomputes the
timeout on every call, but only checks it inside the wait loop:

	timeo = sock_rcvtimeo(sk, nonblock);

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

When a record is already parsed, the loop body never runs. So neither the
zero MSG_DONTWAIT timeout nor SO_RCVTIMEO is checked.

The backlog also keeps refilling. tls_read_flush_backlog() calls
sk_flush_backlog() whenever tcp_inq() is small:

	if (done - *flushed_at < SZ_128K && tcp_inq(sk) > max_rec)
		return false;

	*flushed_at = done;
	return sk_flush_backlog(sk);

Could a peer that keeps sending empty records hold a nonblocking
MSG_WAITALL reader with partial progress in tls_sw_recvmsg()
indefinitely?

It looks like the call only ends on one of these:

  - a pending signal (the nodata && signal_pending(current) check)
  - an error
  - the peer stops sending empty records

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