Re: [PATCH net v2 2/2] strparser: don't serialize all parser work on one ordered workqueue

[netdev-bot+sashiko@kernel.org]

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] Switching kstrp from an ordered workqueue to a per-CPU one
  lets strp_work() and strp_msg_timeout() for the same strparser run on…

--

strparser: don't serialize all parser work on one ordered workqueue

This patch changes the kstrp workqueue from an ordered workqueue (made
with create_singlethread_workqueue()) to a per-CPU WQ_MEM_RECLAIM
workqueue with the default max_active. The aim is to let strparser work
for different sockets run at the same time, so that every strparser
user no longer waits on a single worker.

> diff --git a/net/strparser/strparser.c b/net/strparser/strparser.c
> index d5b17d099a0e7..32fe96ba10d61 100644
> --- a/net/strparser/strparser.c
> +++ b/net/strparser/strparser.c
> @@ -542,7 +542,7 @@ static int __init strp_dev_init(void)
>  	BUILD_BUG_ON(sizeof(struct sk_skb_cb) >
>  		     sizeof_field(struct sk_buff, cb));
>  
> -	strp_wq = create_singlethread_workqueue("kstrp");
> +	strp_wq = alloc_workqueue("kstrp", WQ_PERCPU | WQ_MEM_RECLAIM, 0);

[Severity: Medium]
With this change, strp->work and strp->msg_timer_work for the same
strparser can run on two different kworkers at once. Can a message that
has already completed then be followed by a timeout abort?

The commit message says:

    Nothing relies on the ordering:
    [ ... ]
     - strp_work() and strp_msg_timeout() both run under cb.lock, so they
       are serialized against each other per socket by the socket lock,

The socket lock gives mutual exclusion. However, the cancel in
__strp_recv() when a message completes is not a sync cancel:

net/strparser/strparser.c:__strp_recv() {
    ...
		/* Hurray, we have a new message! */
		cancel_delayed_work(&strp->msg_timer_work);
		strp->skb_head = NULL;
    ...
}

strp_msg_timeout() then aborts unconditionally once it gets the lock:

net/strparser/strparser.c:strp_msg_timeout() {
    ...
	STRP_STATS_INCR(strp->stats.msg_timeouts);
	strp->cb.lock(strp);
	strp->cb.abort_parser(strp, -ETIMEDOUT);
	strp->cb.unlock(strp);
}

Assume SO_RCVTIMEO is set on the lower socket, so strp_start_timer()
arms the timer. Assume also that the timer expires while strp->work is
holding lock_sock(), for example during kTLS decryption for KCM:

kworker B (strp->work)               kworker A (msg_timer_work)
do_strp_work()
  strp->cb.lock()
  read_sock() ...
                                     strp_msg_timeout()
                                       msg_timeouts++
                                       strp->cb.lock()  /* sleeps */
  __strp_recv()
    cancel_delayed_work()  /* not pending, no-op */
    strp->cb.rcv_msg()
    new message started
    strp_start_timer()
      mod_delayed_work()
  strp->cb.unlock()
                                       strp->cb.abort_parser(-ETIMEDOUT)

No in-tree user overrides abort_parser, so this calls strp_abort_strp().
That function sets strp->stopped, frees the newly started skb_head, and
reports ETIMEDOUT on the lower socket. The KCM psock, sockmap or
espintcp stream is torn down right after a successful delivery.

With the old ordered workqueue, the expired timeout work stayed on the
pwq inactive list behind strp->work. The cancel_delayed_work() in
__strp_recv() removed it, because try_to_grab_pending() handles
WORK_STRUCT_INACTIVE. So on the workqueue path the message survived.

A similar but much narrower window, spinning on sk_lock.slock, already
exists on the inline softirq strp_data_ready() path. This patch widens
it to the kstrp path, which after the previous patch in the series is
the only path KCM over kTLS uses.

Should strp_msg_timeout() check under cb.lock that the message it was
timing is still in progress before it aborts? A generation counter, or
a check that skb_head and accum_len still match, would do this.

>  	if (unlikely(!strp_wq))
>  		return -ENOMEM;
>

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