Re: [PATCH net-next v3 1/9] tls: Bound consecutive no-data records in tls_sw_read_sock()

[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: 0 · Low: 1

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 net/tls/tls_sw.c:tls_sw_read_sock(), the new budget exit
  rests on a false premise.
- [Low] When tls_sw_read_sock() hits TLS_RX_NODATA_LIMIT, the only thing
  that restarts the read_sock consumer is a work item queued with…

--

tls: Bound consecutive no-data records in tls_sw_read_sock()

This commit limits how many zero-length application_data records in a
row tls_sw_read_sock() will consume in one call. The limit is
TLS_RX_NODATA_LIMIT. When the limit is reached, the loop breaks and
schedules a work item that calls sk->sk_data_ready() under lock_sock(),
so the read_sock consumer can resume.

> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index 312e51270f293..275a6047da9e3 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -2070,6 +2070,20 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
>  	goto splice_read_end;
>  }
>  
> +#define TLS_RX_NODATA_LIMIT	16
> +
> +static void tls_rx_data_ready_work(struct work_struct *w)
> +{
> +	struct tls_sw_context_rx *ctx =
> +		container_of(w, struct tls_sw_context_rx, data_ready_work);
> +	struct sock *sk = ctx->strp.sk;
> +
> +	/* A callback that finds the socket unowned reads from it. */
> +	lock_sock(sk);
> +	READ_ONCE(sk->sk_data_ready)(sk);
> +	release_sock(sk);
> +}

[ ... ]

> @@ -2136,14 +2151,22 @@ int tls_sw_read_sock(struct sock *sk, read_descriptor_t *desc,

[ ... ]

>  		if (rxm->full_len == 0) {
> +			err = 0;
>  			consume_skb(skb);
> +			if (++nodata >= TLS_RX_NODATA_LIMIT) {
> +				/* tls_rx_reader_release() does not call the
> +				 * consumer's sk_data_ready, which can re-enter
> +				 * this function.
> +				 */

[Severity: High]
Is this comment accurate? tls_rx_reader_release() begins by calling
tls_rx_msg_maybe_announce(). That function calls the consumer's callback
synchronously, and reader_present is still set when it does:

net/tls/tls_sw.c:tls_rx_reader_release() {
	tls_rx_msg_maybe_announce(&ctx->strp);
	...
	WRITE_ONCE(ctx->reader_present, 0);
}

net/tls/tls_sw.c:tls_rx_msg_maybe_announce() {
	if (!READ_ONCE(strp->msg_ready) || strp->msg_announced)
		return;
	strp->msg_announced = 1;
	...
	ctx->saved_data_ready(strp->sk);
}

The same concern applies to this part of the commit message:

  "Records left queued do not raise another sk_data_ready(), so the
  consumer needs one."

The new break seems to produce exactly that state. By the 16th empty
record, tls_rx_rec_done() has already run tls_strp_msg_consume(), which
clears msg_ready and msg_announced. It has also run
tls_strp_check_rcv(strp, false), which can parse the next fully received
record without announcing it. The loop then breaks while the consumer's
strparser is not paused.

Take a consumer such as KCM that installed psock_data_ready before TLS
RX was armed. tls_sw_strparser_arm() saves psock_data_ready as
saved_data_ready and installs tls_data_ready. In softirq, under
bh_lock_sock(), the outer reader is reached like this:

tcp_data_ready()
  tls_data_ready()
    tls_strp_data_ready()
      tls_strp_check_rcv()
        tls_rx_msg_maybe_announce()
          psock_data_ready()
            strp_data_ready()
              strp_read_sock()
                tls_sw_read_sock()

When that call takes the new break:

tls_rx_reader_release()
  tls_rx_msg_maybe_announce()
    psock_data_ready()
      strp_data_ready()          /* socket unowned, strp not paused */
        strp_read_sock()
          tls_sw_read_sock()     /* nested */
            tls_rx_reader_acquire()   /* reader_present == 1 */
              sk_wait_event()
                release_sock()   /* spin_lock_bh(&sk->sk_lock.slock) */

Can this self-deadlock on sk_lock.slock, which this CPU already holds
through bh_lock_sock()? It looks like a peer only has to send 17 or more
consecutive empty data records that arrive together in the receive
queue.

Before this patch the loop ran until no record was ready, so msg_ready
was clear when the reader was released.

In this configuration, does tls_rx_data_ready_work() reach the consumer
at all? There sk->sk_data_ready is tls_data_ready, not psock_data_ready.
Under lock_sock(), tls_data_ready only queues the tls_strp work, and
tls_strp_check_rcv() then returns early because msg_ready is already
set. The release-time announce goes to saved_data_ready while the work
item goes to sk->sk_data_ready, so the two notification paths seem to
disagree.

> +				schedule_work(&ctx->data_ready_work);

[Severity: Low]
schedule_work() queues this on system_wq, which lacks WQ_MEM_RECLAIM.
The existing TLS RX deferral uses tls_strp_wq instead:

net/tls/tls_strp.c:tls_strp_dev_init() {
	tls_strp_wq = create_workqueue("tls-strp");
	...
}

Some read_sock consumers, such as nvme-tcp over kTLS, sit in the memory
reclaim I/O path. After a burst of empty records, this work item is the
only thing that restarts the consumer. Could RX stall under memory
pressure until another event, such as new TCP data, arrives?

Would tls_strp_wq, or another WQ_MEM_RECLAIM workqueue, be a better
place for this work?

> +				break;
> +			}
>  			continue;
>  		}

[ ... ]

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