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

On Mon, Feb 23, 2026 at 06:20:58PM +0100, Sabrina Dubroca wrote:

2026-02-20, 18:29:55 +0900, Hyunwoo Kim wrote:

quoted

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