Re: [PATCH net v2] strparser: Fix race condition in strp_done()

[Sabrina Dubroca <sd@queasysnail.net>]

2026-02-27, 06:51:10 +0900, Hyunwoo Kim wrote:
> On Mon, Feb 23, 2026 at 06:20:58PM +0100, Sabrina Dubroca wrote:
> > 2026-02-20, 18:29:55 +0900, Hyunwoo Kim wrote:
> > > This issue was discovered during a code audit.
> > > 
> > > 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.
> > > 
> > > The following is a simple race scenario:
> > > 
> > >           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->msg_timer_work);
> > > 
> > > To prevent these races, the cancellation APIs are replaced with 
> > > worker-disabling APIs.
> > 
> > I'm still not totally convinced by this patch. The comment for
> > strp_done says the function expects to be called at a time when
> > strp_recv cannot happen in parallel:
> > 
> >     strp must already be stopped so that strp_recv will no longer be called
> 
> OK, I understand.
> More specifically, it seems that an issue could occur if strp->skb_head is 
> accessed under the following scenario.

Yes.

> ```
>                  cpu0                                cpu1
> 
> espintcp_close()
>                                          espintcp_data_ready()
>                                            strp_data_ready()
>                                              if (unlikely(strp->stopped)) return;
>   strp_stop()
>     strp->stopped = 1;
>   strp_done()
>     disable_delayed_work_sync(&strp->msg_timer_work);
>     kfree_skb(strp->skb_head);
>                                              strp_read_sock()
>                                                tcp_read_sock()
>                                                  __tcp_read_sock()
>                                                    strp_recv()
>                                                      __strp_recv()
>                                                        head = strp->skb_head;
>                                                        ...
> ```
> 
> > 
> > "strp stopped" is not really enough, I think we'd also need to reset
> > the CBs, and then grab bh_lock_sock to make sure a previously-running
> > ->sk_data_ready has completed. This is what kcm does, at least.
> 
> It seems that this is not something that should be handled inside strp itself, 
> but rather something that each caller of strp_stop() is expected to take care 
> of individually. Would that be the right direction?

Agree.

> It also appears that ovpn and kcm handle this by implementing their own callback 
> restoration logic.

Right. I tried to look at skmsg/psock (the other user of strp), but
didn't get far enough to verify if it's handling this correctly.

> > Without that, if strp_recv runs in parallel (not from strp->work) with
> > strp_done, cleaning up skb_head in strp_done seems problematic.
> 
> From the espintcp perspective, how about applying a patch along the following lines?

This is what I was thinking about, yes.

Thanks.

-- 
Sabrina