Re: [PATCH] strparser: Use worker disable API instead of cancellation in strp_done()

[Sabrina Dubroca <sd@queasysnail.net>]

2026-02-16, 18:48:08 +0900, Hyunwoo Kim wrote:
> When strp_stop() and strp_done() are called without holding lock_sock(), 
> they can race with worker-scheduling paths such as the Delayed ACK handler 
> and ksoftirqd.
> Specifically, after cancel_delayed_work_sync() and cancel_work_sync() are 
> invoked from strp_done(), the workers may still be scheduled.
> As a result, the workers may dereference freed objects.
> 
> To prevent these races, the cancellation APIs are replaced with 
> worker-disabling APIs.
> 
> Fixes: 829385f08ae9 ("strparser: Use delayed work instead of timer for msg timeout")

That's the correct commit for msg_timer_work, but not for
strp->work. No race was possible when msg timeout was using a timer?
Your second scenario relies only on strp->work so I would think yes.

> Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
> ---
>  net/strparser/strparser.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/strparser/strparser.c b/net/strparser/strparser.c
> index fe0e76fdd1f1..15cd9cadbd1a 100644
> --- a/net/strparser/strparser.c
> +++ b/net/strparser/strparser.c
> @@ -503,8 +503,8 @@ void strp_done(struct strparser *strp)
>  {
>  	WARN_ON(!strp->stopped);
>  
> -	cancel_delayed_work_sync(&strp->msg_timer_work);
> -	cancel_work_sync(&strp->work);
> +	disable_delayed_work_sync(&strp->msg_timer_work);
> +	disable_work_sync(&strp->work);

The change itself looks reasonable.

>  	if (strp->skb_head) {
>  		kfree_skb(strp->skb_head);
> -- 
> 2.43.0
> 
> ---
> Dear,
> 
> The following is a simplified scenario illustrating how each race can occur. Since espintcp_close() does not hold lock_sock(), the race is possible.
> Although cancel_work_sync(&strp->work) does not appear to be easy to trigger in practice here, it still seems better to fix it as well.

What about the other users of strp? Only espintcp is racy?

If strp_done can run concurrently with __strp_recv, it seems we could
also end up leaking strp->skb_head, if __strp_recv stores a new one
after we've cleared the old?


> ```
>                  cpu0                                cpu1
> 
> espintcp_close()
>                                          espintcp_data_ready()
>                                            if (unlikely(strp->stopped)) return;
>   strp_stop()
>     strp->stopped = 1;
>   strp_done()
>     cancel_delayed_work_sync(&strp->msg_timer_work);
>                                            strp_data_ready()

In this order, strp_data_ready will see strp->stopped and return
without doing anything.

(I'm confused by the "if (unlikely(strp->stopped))" above though,
maybe you meant espintcp_data_ready -> strp_data_ready -> if (...))

>                                              strp_read_sock()
>                                                tcp_read_sock()
>                                                  __tcp_read_sock()
>                                                    strp_recv()
>                                                      __strp_recv()
>                                                        strp_start_timer()
>                                                          mod_delayed_work(strp_wq, &strp->msg_timer_work, timeo);  
> ```
> ```
>                  cpu0                                cpu1
> 
> espintcp_close()
>                                           sk->sk_data_ready()
>                                             espintcp_data_ready()
>                                               if (unlikely(strp->stopped)) return;
>   strp_stop()
>     strp->stopped = 1;
>   strp_done()
>     cancel_work_sync(&strp->work);
>                                               if (strp_read_sock(strp) == -ENOMEM)
>                                               queue_work()

Here the problem would be if we enter do_strp_work after all the
socket data has already been freed? Otherwise again the test on
strp->stopped will make do_strp_work return early. (this would be
unexpected but should be safe)

> ```
> 
> Best regards,
> Hyunwoo Kim

-- 
Sabrina