[PATCH net] kcm: defer receive parsing when the lower socket's read_sock may sleep

[PATCH net] kcm: defer receive parsing when the lower socket's read_sock may sleep

[Rishikesh Jethwani]

psock_data_ready() is the lower socket's data_ready callback and can
run in softirq context, under read_lock_bh(&sk->sk_callback_lock). It calls
strp_data_ready(), which, when the socket is not owned by a user,
invokes sock->ops->read_sock() inline to pull records out of the
receive queue. For a plain TCP socket that is tcp_read_sock(), which
does not block.

A ULP may install a different read_sock. kTLS installs
tls_sw_read_sock(), which can sleep: it waits for an asynchronous
AEAD to complete via crypto_wait_req(), flushes the socket backlog
through __sk_flush_backlog(), allocates with sk->sk_allocation, and,
on reader contention in tls_rx_reader_acquire(), enters
sk_wait_event()'s socket release/reacquire path despite the zero
timeout. It passes nonblock, so it does not wait for data, but these
paths can require sleepable context, which softirq and a BH read lock
do not provide. Attaching a kTLS socket to a KCM multiplexor can
therefore sleep in atomic context.

With CONFIG_DEBUG_ATOMIC_SLEEP, a loopback TCP connection using software
kTLS RX, with the receiving socket attached to a KCM mux, reports:

  BUG: sleeping function called from invalid context at include/linux/sched/mm.h:321
  in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 189, name: ksoftirqd/34
  Call Trace:
   __might_resched
   kmem_cache_alloc_node_noprof
   __alloc_skb
   alloc_skb_with_frags
   tls_rx_one_record
   tls_sw_read_sock
   strp_read_sock
   strp_data_ready
   psock_data_ready [kcm]

Keep the inline fast path for tcp_read_sock() and hand any other
read_sock to the parser work via strp_check_rcv(). The work runs
do_strp_work() under lock_sock(), where a sleeping read_sock is fine.
sk->sk_socket can be NULL here while the lower socket is being
released; strp_read_sock() already treats that as a reason not to
read, so take the deferred path in that case as well.

Tested with the reproducer above on a 6.18 kernel: without this change
the splat is reported on every run; with it, nothing is reported.

Fixes: 662fbcec32f4 ("net/tls: implement ->read_sock()")
Assisted-by: LLM
Signed-off-by: Rishikesh Jethwani <rjethwani@purestorage.com>
---
 net/kcm/kcmsock.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/net/kcm/kcmsock.c b/net/kcm/kcmsock.c
index 71af69d442f2..381a8f0b792e 100644
--- a/net/kcm/kcmsock.c
+++ b/net/kcm/kcmsock.c
@@ -30,6 +30,7 @@
 #include <net/kcm.h>
 #include <net/netns/generic.h>
 #include <net/sock.h>
+#include <net/tcp.h>
 #include <uapi/linux/kcm.h>
 #include <trace/events/sock.h>
 
@@ -358,8 +359,19 @@ static void psock_data_ready(struct sock *sk)
 	read_lock_bh(&sk->sk_callback_lock);
 
 	psock = (struct kcm_psock *)sk->sk_user_data;
-	if (likely(psock))
-		strp_data_ready(&psock->strp);
+	if (likely(psock)) {
+		struct socket *sock = sk->sk_socket;
+
+		/* strp_data_ready() calls sock->ops->read_sock() inline from
+		 * this softirq callback. A ULP's read_sock, such as kTLS's
+		 * tls_sw_read_sock(), may sleep, so hand anything other than
+		 * TCP's own to the parser work, which runs under lock_sock().
+		 */
+		if (likely(sock && sock->ops->read_sock == tcp_read_sock))
+			strp_data_ready(&psock->strp);
+		else
+			strp_check_rcv(&psock->strp);
+	}
 
 	read_unlock_bh(&sk->sk_callback_lock);
 }

base-commit: 8e2fbe0e75d38690d551456658194c632933cb57
-- 
2.50.1

Re: [PATCH net] kcm: defer receive parsing when the lower socket's read_sock may sleep

[netdev-bot+sashiko]

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] In net/kcm/kcmsock.c, psock_data_ready() now passes every
  data_ready event on a non-tcp_read_sock lower socket (for example, a…

--

kcm: defer receive parsing when the lower socket's read_sock may sleep

This commit changes psock_data_ready() so that strp_data_ready() runs
inline only when the lower socket's read_sock is tcp_read_sock(). Any
other read_sock, such as kTLS's tls_sw_read_sock(), is now deferred to
the strparser work via strp_check_rcv(). The goal is to avoid sleeping
in softirq context.

> diff --git a/net/kcm/kcmsock.c b/net/kcm/kcmsock.c
> index 71af69d442f21..381a8f0b792e3 100644
> --- a/net/kcm/kcmsock.c
> +++ b/net/kcm/kcmsock.c

[ ... ]

> @@ -358,8 +359,19 @@ static void psock_data_ready(struct sock *sk)
>  	read_lock_bh(&sk->sk_callback_lock);
>  
>  	psock = (struct kcm_psock *)sk->sk_user_data;
> -	if (likely(psock))
> -		strp_data_ready(&psock->strp);
> +	if (likely(psock)) {
> +		struct socket *sock = sk->sk_socket;
> +
> +		/* strp_data_ready() calls sock->ops->read_sock() inline from
> +		 * this softirq callback. A ULP's read_sock, such as kTLS's
> +		 * tls_sw_read_sock(), may sleep, so hand anything other than
> +		 * TCP's own to the parser work, which runs under lock_sock().
> +		 */
> +		if (likely(sock && sock->ops->read_sock == tcp_read_sock))
> +			strp_data_ready(&psock->strp);
> +		else
> +			strp_check_rcv(&psock->strp);

[Severity: Medium]
Will this serialize all KCM-over-kTLS receive processing on a single
system-wide worker?

strp_check_rcv() only does queue_work(strp_wq, &strp->work). strp_wq is
created in strp_dev_init() as an ordered workqueue:

net/strparser/strparser.c:strp_dev_init() {
    ...
	strp_wq = create_singlethread_workqueue("kstrp");
    ...
}

So only one strparser work item can run at a time across the whole
system. KCM psocks, the sockmap strparser in net/core/skmsg.c and
espintcp in net/xfrm/espintcp.c all share this queue.

With this change, every data_ready event on a kTLS-backed psock ends up
in this path:

strp_work()
  do_strp_work()
    lock_sock()
    strp_read_sock()
      tls_sw_read_sock()
        tls_rx_one_record()   /* sync AEAD decrypt or crypto_wait_req() */
        tls_read_flush_backlog()

While one psock is decrypting or waiting there, do_strp_work() for every
other strparser instance waits behind it on kstrp.

Before this patch, the common case ran inline in softirq on the
receiving CPU, so several sockets could be processed in parallel. kstrp
was only used for the sock_owned_by_user() and -ENOMEM fallbacks.

Could this add receive latency for unrelated KCM, sockmap and espintcp
users? Each individual wait is bounded, but a local user who controls
KCM+kTLS sockets could keep the shared worker busy. Would a per-psock or
unbound workqueue for this deferred path be a better fit?

> +	}
>  
>  	read_unlock_bh(&sk->sk_callback_lock);
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013033920.2069285-1-rjethwani%40purestorage.com