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

[Rishikesh Jethwani]

On Tue, Oct 13, 2026 at 9:03 PM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> 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);
> >  }

Thanks, the point about kstrp being ordered is correct, and it goes further
than this patch.

kstrp has been create_singlethread_workqueue() since strparser was
introduced, and the serialization already bites today. strp_work() takes
cb.lock, which is lock_sock() for every in-tree user (KCM, sockmap,
espintcp, ovpn), so the single kstrp worker already stalls for as long as
any process holds the socket lock of any strparser-attached socket. That
wait has no bound and is a bigger hazard than one AEAD decrypt. KCM
over plain TCP already queues to kstrp on attach and under receive-buffer
backpressure (kcm_rcv_ready() -> strp_check_rcv()), and strparser itself
queues there when the socket is owned or on -ENOMEM. This patch makes the
deferred path more common for KCM-over-kTLS, which currently sleeps in
atomic context. It changes nothing for plain TCP psocks.

Nothing in strparser relies on kstrp being ordered. The workqueue core
never runs one work item on two workers at once; strp_work() and
strp_msg_timeout() both run under cb.lock; strp_done() uses the
cancel_*_sync() variants; and separate strparsers were never ordered
against each other, since the common strp_data_ready() path runs inline in
softirq on whichever CPU received the data.

So rather than a per-psock workqueue, which would need a new strparser
API, v2 adds a follow-up patch that allocates kstrp with
alloc_workqueue("kstrp", WQ_PERCPU | WQ_MEM_RECLAIM, 0), per-CPU and
WQ_MEM_RECLAIM like kTLS's tls-strp. That removes the system-wide
serialization for all strparser users, so a slow or lock-blocked psock only
delays its own work item. v2 also reads sock->ops with READ_ONCE(), since
kTLS can replace it with WRITE_ONCE() after the socket is attached to KCM.

pw-bot: cr