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

[Sabrina Dubroca <sd@queasysnail.net>]

2026-03-06, 09:11:04 +0900, Hyunwoo Kim wrote:
> 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.

Not really. Every user of strp that is open to the strp_recv vs
cancel_* race is also open to the strp_recv vs free race, so switching
from cancel_* to disable_* is only a partial fix.

But if we took and released the socket lock in strp_done, we would
solve the issue for all users even without resetting the callbacks?

@@ -503,6 +503,10 @@ void strp_done(struct strparser *strp)
 {
 	WARN_ON(!strp->stopped);
 
+	lock_sock(strp->sk);
+	/* sync with other code */
+	release_sock(strp->sk);
+
 	cancel_delayed_work_sync(&strp->msg_timer_work);
 	cancel_work_sync(&strp->work);
 


 - strp->stopped so any new call into strp_data_ready will not do anything

 - lock/release need to take bh_lock_sock so any existing call to
   strp_data_ready will have to complete before we move on to cancel*_work



Or maybe the requirement should be that strp_stop has to be called
under lock_sock() (or even just bh_lock_sock), but again I can't
figure out if that's ok for sockmap.


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

I'm not going to nack a patch doing s/cancel_/disable_/ in strp_done,
but it doesn't fully solve the race condition if the caller isn't
doing the right thing, and it doesn't do anything if the strp user is
handling the teardown correctly.

-- 
Sabrina