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