Re: [PATCH net-next v4 4/9] tls: Consume empty data records in tls_sw_splice_read()

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · 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] In tls_sw_splice_read() (net/tls/tls_sw.c), the new `goto
  retry` loop after an empty application_data record calls…
- [Medium] The new `goto retry` loop in tls_sw_splice_read() keeps
  lock_sock() ownership and the TLS reader lock (taken by…

--

tls: Consume empty data records in tls_sw_splice_read()

This commit makes tls_sw_splice_read() consume zero-length
application_data records and loop back for the next record. Before, it
returned zero, which the caller treats as EOF. A signal_pending() check
before each retry keeps the splicing task killable.

Closes: https://lore.kernel.org/netdev/akaoXcfamBp8_mYe@krikkit/

> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index f5e329b75976b..0fea14224cec4 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c

[ ... ]

> @@ -2057,6 +2059,9 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
>  
>  		tls_rx_rec_done(ctx);
>  		skb = darg.skb;
> +
> +		/* The retry's wait runs with the socket lock still held. */
> +		released = false;

[Severity: Medium]
The socket lock is held across the retry. If the peer keeps sending empty
records, how long can this loop keep lock_sock() ownership and the TLS
reader lock?

After each empty record, tls_rx_rec_done() calls tls_strp_check_rcv().
That parses the next record from sk_receive_queue. On the retry,
tls_rx_rec_wait() sees tls_strp_msg_ready() as true and skips its whole
wait loop:

tls_rx_rec_wait() {
    ...
	while (!tls_strp_msg_ready(ctx)) {
		...
		if (sk_flush_backlog(sk))
			released = true;
		...
		ret = sk_wait_event(sk, &timeo,
				    tls_strp_msg_ready(ctx), &wait);
		...
	}
    ...
}

So the loop never reaches sk_flush_backlog() or the lock release inside
sk_wait_event().

Each pass does a synchronous decrypt, with an skb allocation, a
kmalloc() and an AEAD operation, and there is no cond_resched(). The
only scheduling point is the cond_resched() in __release_sock() during
a backlog flush. That flush runs only once the receive queue no longer
holds a complete record.

While this runs, other lock_sock() users of the socket are held off,
such as a tls_sw_sendmsg() thread or setsockopt(). So are TCP timers
deferred to the socket owner. On PREEMPT_NONE kernels the CPU may also
go a long time without rescheduling. The signal_pending() check makes
the task killable, but it does not limit how long the lock is held.

Another commit in the same series, "tls: Bound consecutive no-data
records in tls_sw_read_sock()", fixes the same loop shape in the sibling
reader:

tls_sw_read_sock() {
    ...
			if (++nodata >= TLS_RX_NODATA_LIMIT)
				break;
			continue;
    ...
}

Would something similar help here? One option is a cap on consecutive
empty records that returns -EAGAIN or -EINTR. Another is to release the
socket and call cond_resched() before the goto retry.

>  	}
>  
>  	rxm = strp_msg(skb);
> @@ -2068,6 +2073,20 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
>  		goto splice_requeue;
>  	}
>  
> +	/* Splicing zero bytes reads as EOF to the caller. */
> +	if (rxm->full_len == 0) {
> +		consume_skb(skb);
> +		if (signal_pending(current)) {
> +			long timeo = sock_rcvtimeo(sk, nonblock);
> +
> +			err = -EAGAIN;
> +			if (timeo)
> +				err = sock_intr_errno(timeo);
> +			goto splice_read_end;
> +		}
> +		goto retry;

[Severity: Medium]
Can this retry make SO_RCVTIMEO restart with every empty record?

Each goto retry calls tls_rx_rec_wait() again. tls_rx_rec_wait()
computes a new timeout on every call, so time already spent waiting is
lost:

tls_rx_rec_wait() {
    ...
	timeo = sock_rcvtimeo(sk, nonblock);
    ...
}

Suppose the peer sends an empty application_data record a little more
often than every SO_RCVTIMEO interval. Each record wakes sk_wait_event()
and decrypts to full_len == 0. It is then consumed, and the wait starts
over with a full timeout. A blocking splice(2) or sendfile(2) that never
moves any data would then never return -EAGAIN.

A nonblocking caller only gets -EAGAIN from inside the
!tls_strp_msg_ready() loop in tls_rx_rec_wait():

		if (!timeo)
			return -EAGAIN;

What if empty records keep arriving at least as fast as they are
decrypted? Could a SPLICE_F_NONBLOCK or O_NONBLOCK splice then stay in
the kernel until a signal arrives? That looks like the event-loop stall
that "tls: Honor O_NONBLOCK in tls_sw_splice_read()", earlier in the
series, set out to prevent.

tls_sw_recvmsg() also restarts the timeout for each record. Before this
patch, though, splice handled at most one record per call.

The loop has no iteration cap. tls_sw_read_sock() already limits this
case with TLS_RX_NODATA_LIMIT. Would a matching limit make sense in
tls_sw_splice_read()?

> +	}
> +
>  	chunk = min_t(unsigned int, rxm->full_len, len);
>  	copied = skb_splice_bits(skb, sk, rxm->offset, pipe, chunk, flags);
>  	if (copied < 0)

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