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

[Hyunwoo Kim <imv4bel@gmail.com>]

On Fri, Mar 06, 2026 at 12:35:48AM +0100, Sabrina Dubroca wrote:
> Sorry for the delay, I wanted to think about the race condition a bit
> more.
> 
> 2026-03-03, 10:50:05 +0900, Hyunwoo Kim wrote:
> > 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:
> > > > > "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?
> 
> Sure. But once espintcp is fixed in that way, can the original race
> condition with strparser still occur? release_sock() will wait for any

If the espintcp callback restoration patch is applied, the strparser
race should no longer occur in espintcp.

> espintcp_data_ready()/strp_data_ready() that's already running, and a
> sk_data_ready that starts after we've changed the callbacks will not
> end up in strp_data_ready() at all so it won't restart the works that
> are being stopped by strp_done()?
> 
> It's quite reasonable to use disable*_work_sync in strp_done, but I'm
> not sure there's a bug other than espintcp not terminating itself
> correctly on the socket.

That said, the _cancel APIs in strparser still appear to carry some
structural risk, so it might still make sense to switch to the _disable
APIs for the benefit of other strp users or potential future callers.

With that in mind, perhaps the direct fix for this race could be handled
in the espintcp callback restoration patch. For the strparser patch, I
could instead adjust the commit message to reflect that it removes a
potential hazard by replacing the _cancel APIs with the _disable
variants, and resubmit it in that form.

What do you think about proceeding in that direction?


Best regards,
Hyunwoo Kim