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

[Hyunwoo Kim <imv4bel@gmail.com>]

On Tue, Mar 03, 2026 at 12:10:33AM +0100, Sabrina Dubroca wrote:
> 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.

In my opinion, it might be cleaner to split the espintcp callback restoration work into 
a separate patch, rather than merging it into the strparser v3 patch. What do you think?

It seems that the two changes address slightly different kinds of issues.

If you agree, I can prepare and submit the espintcp callback restoration patch 
separately shortly.


Best regards,
Hyunwoo Kim