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

[Sabrina Dubroca <sd@queasysnail.net>]

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

-- 
Sabrina