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

[Hyunwoo Kim <imv4bel@gmail.com>]

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

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

> 
> 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?
```
diff --git a/net/xfrm/espintcp.c b/net/xfrm/espintcp.c
index e1b11ab59f6e..989638fdc111 100644
--- a/net/xfrm/espintcp.c
+++ b/net/xfrm/espintcp.c
@@ -526,12 +526,28 @@ static void espintcp_release(struct sock *sk)
 	tcp_release_cb(sk);
 }

+static void espintcp_detach_sock(struct sock *sk)
+{
+	struct espintcp_ctx *ctx = espintcp_getctx(sk);
+
+	lock_sock(sk);
+
+	write_lock_bh(&sk->sk_callback_lock);
+	sk->sk_data_ready = ctx->saved_data_ready;
+	sk->sk_write_space = ctx->saved_write_space;
+	write_unlock_bh(&sk->sk_callback_lock);
+
+	strp_stop(&ctx->strp);
+
+	release_sock(sk);
+}
+
 static void espintcp_close(struct sock *sk, long timeout)
 {
 	struct espintcp_ctx *ctx = espintcp_getctx(sk);
 	struct espintcp_msg *emsg = &ctx->partial;

-	strp_stop(&ctx->strp);
+	espintcp_detach_sock(sk);

 	sk->sk_prot = &tcp_prot;
 	barrier();
```

Best regards,
Hyunwoo Kim