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

[Hyunwoo Kim <imv4bel@gmail.com>]

On Tue, Feb 17, 2026 at 07:21:57PM +0100, Sabrina Dubroca wrote:
> 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?

Of course, the race could also occur when the message timeout was 
implemented using a timer.

> Your second scenario relies only on strp->work so I would think yes.

Using Fixes: bbb0302 ("strparser: Generalize strparser") should cover 
both cases.


> 
> > 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?

Any subsystem that calls strp_stop() and strp_done() outside of 
lock_sock() 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?

I am not fully sure about this part yet, so I think it should be 
discussed separately in another thread.

> 
> 
> > ```
> >                  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 (...))

Sorry for the confusion. I accidentally mixed up the call order in the 
strp_data_ready() path. 
More precisely, the scenario is that espintcp_data_ready() → strp_data_ready() 
runs first, passes the if (unlikely(strp->stopped)) check, and only after 
that strp->stopped = 1 is set.
```
                 cpu0                                cpu1

espintcp_close()
                                         espintcp_data_ready()
                                           strp_data_ready()
                                             if (unlikely(strp->stopped)) return;
  strp_stop()
    strp->stopped = 1;
  strp_done()
    cancel_delayed_work_sync(&strp->msg_timer_work);
                                             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()
                                            espintcp_data_ready()
                                              strp_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()
```


> 
> >                                              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)

If the worker is scheduled after the cancel call, then during 
espintcp_close() → tcp_close(), the sk will be freed and the ctx 
will be freed as well. 
As a result, the kworker will access the freed ctx->strp. 
This access to the freed ctx happens before the actual worker 
handler is called, so the problem occurs regardless of the 
condition checks in do_strp_work().
```
worker_thread()
  assign_work(work, &worker->scheduled)
    move_linked_works(work)    // work: ctx->strp->msg_timer_work or ctx->strp->work
```

> 
> > ```
> > 
> > Best regards,
> > Hyunwoo Kim
> 
> -- 
> Sabrina