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

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

[Hyunwoo Kim]

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.

Fixes: bbb03029a899 ("strparser: Generalize strparser")
Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
---
Changes in v2:
- Update Fixes tag
- Incorporate Simon’s review: https://lore.kernel.org/all/aZSLHaFj7k8DPmLG@horms.kernel.org/
  - Shorten the patch subject
  - Target the net tree
  - Add the bug discovery background and the race scenario to the commit message
- v1: https://lore.kernel.org/all/aZLn2Faeg1FB7XOf@v4bel/
---
 net/strparser/strparser.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/strparser/strparser.c b/net/strparser/strparser.c
index fe0e76fdd1f1..15cd9cadbd1a 100644
--- a/net/strparser/strparser.c
+++ b/net/strparser/strparser.c
@@ -503,8 +503,8 @@ void strp_done(struct strparser *strp)
 {
 	WARN_ON(!strp->stopped);
 
-	cancel_delayed_work_sync(&strp->msg_timer_work);
-	cancel_work_sync(&strp->work);
+	disable_delayed_work_sync(&strp->msg_timer_work);
+	disable_work_sync(&strp->work);
 
 	if (strp->skb_head) {
 		kfree_skb(strp->skb_head);
-- 
2.43.0

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

[Sabrina Dubroca]

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

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

Without that, if strp_recv runs in parallel (not from strp->work) with
strp_done, cleaning up skb_head in strp_done seems problematic.

-- 
Sabrina

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

[Hyunwoo Kim]

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

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

[Sabrina Dubroca]

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

Yes.

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

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.

Thanks.

-- 
Sabrina

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

[Hyunwoo Kim]

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

It seems that the two changes address slightly different kinds of issues.

If you agree, I can prepare and submit the espintcp callback restoration patch 
separately shortly.


Best regards,
Hyunwoo Kim

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

[Sabrina Dubroca]

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

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

[Hyunwoo Kim]

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

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

[Sabrina Dubroca]

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

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

[Hyunwoo Kim]

On Fri, Mar 06, 2026 at 11:13:19AM +0100, Sabrina Dubroca wrote:
> 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?

Looks good to me. With this change, it seems the issue can be resolved
not only for espintcp but for all strp users.

When strp_stop() runs first:
```
                 cpu0                                cpu1

espintcp_close()
  strp_stop()
    strp->stopped = 1;
                                         espintcp_data_ready()
                                           strp_data_ready()
                                             if (unlikely(strp->stopped)) return;
  strp_done()
    lock_sock();
    release_sock();
    cancel_delayed_work_sync(&strp->msg_timer_work);
    kfree_skb(strp->skb_head);
```

When strp_data_ready() runs first:
```
                 cpu0                                cpu1

                                         espintcp_data_ready()
                                           strp_data_ready()
                                             if (unlikely(strp->stopped)) return;
espintcp_close()
  strp_stop()
    strp->stopped = 1;
  strp_done()
    lock_sock();
                                             strp_read_sock()
                                               tcp_read_sock()
                                                 __tcp_read_sock()
                                                   strp_recv()
                                                     __strp_recv()
                                                       head = strp->skb_head;
                                                       strp_start_timer()
                                                         mod_delayed_work(&strp->msg_timer_work);
                                                       ...
    release_sock();
    cancel_delayed_work_sync(&strp->msg_timer_work);
    kfree_skb(strp->skb_head);
```
In both cases, the race does not appear to cause any problem.

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

From my perspective, adding lock_sock() inside strp_done(), as in the
patch above, looks cleaner.

> under lock_sock() (or even just bh_lock_sock), but again I can't
> figure out if that's ok for sockmap.

sockmap/psock has a more complex call stack compared to other strp
users, so I'm also not entirely certain about that part.

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

I agree with your point there. Still, after the core patches addressing
this race are applied, I plan to resubmit the _disable patch with an
updated commit message. I think applying that change is still beneficial.


Best regards,
Hyunwoo Kim

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

[Hyunwoo Kim]

On Fri, Mar 06, 2026 at 08:41:02PM +0900, Hyunwoo Kim wrote:
> On Fri, Mar 06, 2026 at 11:13:19AM +0100, Sabrina Dubroca wrote:
> > 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?
> 
> Looks good to me. With this change, it seems the issue can be resolved
> not only for espintcp but for all strp users.
> 
> When strp_stop() runs first:
> ```
>                  cpu0                                cpu1
> 
> espintcp_close()
>   strp_stop()
>     strp->stopped = 1;
>                                          espintcp_data_ready()
>                                            strp_data_ready()
>                                              if (unlikely(strp->stopped)) return;
>   strp_done()
>     lock_sock();
>     release_sock();
>     cancel_delayed_work_sync(&strp->msg_timer_work);
>     kfree_skb(strp->skb_head);
> ```
> 
> When strp_data_ready() runs first:
> ```
>                  cpu0                                cpu1
> 
>                                          espintcp_data_ready()
>                                            strp_data_ready()
>                                              if (unlikely(strp->stopped)) return;
> espintcp_close()
>   strp_stop()
>     strp->stopped = 1;
>   strp_done()
>     lock_sock();
>                                              strp_read_sock()
>                                                tcp_read_sock()
>                                                  __tcp_read_sock()
>                                                    strp_recv()
>                                                      __strp_recv()
>                                                        head = strp->skb_head;
>                                                        strp_start_timer()
>                                                          mod_delayed_work(&strp->msg_timer_work);
>                                                        ...
>     release_sock();
>     cancel_delayed_work_sync(&strp->msg_timer_work);
>     kfree_skb(strp->skb_head);
> ```
> In both cases, the race does not appear to cause any problem.
> 
> > 
> > @@ -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
> 
> From my perspective, adding lock_sock() inside strp_done(), as in the
> patch above, looks cleaner.
> 
> > under lock_sock() (or even just bh_lock_sock), but again I can't
> > figure out if that's ok for sockmap.
> 
> sockmap/psock has a more complex call stack compared to other strp
> users, so I'm also not entirely certain about that part.

I looked into the sockmap/psock side. sk_psock_strp_data_ready() is protected 
by read_lock_bh(&sk->sk_callback_lock), and during teardown sk_psock_drop() 
performs callback restoration and strp_stop() under 
write_lock_bh(&sk->sk_callback_lock), so sockmap/psock doesn't have this race 
to begin with.

As for introducing this patch, in sockmap/psock strp_done() is only called 
from sk_psock_destroy(), which is scheduled via queue_rcu_work() and runs 
on system_percpu_wq after an RCU GP, so no locks including lock_sock are held 
at that point. And since lock_sock is released before cancel_work_sync, 
there's no circular dependency with do_strp_work/strp_msg_timeout either. 
So this patch shouldn't introduce any new issues for sockmap/psock.

> 
> > 
> > 
> > > 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.
> 
> I agree with your point there. Still, after the core patches addressing
> this race are applied, I plan to resubmit the _disable patch with an
> updated commit message. I think applying that change is still beneficial.
> 
> 
> Best regards,
> Hyunwoo Kim

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

[Jiayuan Chen]

On Fri, Mar 06, 2026 at 08:41:02PM +0800, Hyunwoo Kim wrote:
> On Fri, Mar 06, 2026 at 11:13:19AM +0100, Sabrina Dubroca wrote:
> > 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?
> 
> Looks good to me. With this change, it seems the issue can be resolved
> not only for espintcp but for all strp users.
> 
> When strp_stop() runs first:
> ```
>                  cpu0                                cpu1
> 
> espintcp_close()
>   strp_stop()
>     strp->stopped = 1;
>                                          espintcp_data_ready()
>                                            strp_data_ready()
>                                              if (unlikely(strp->stopped)) return;
>   strp_done()
>     lock_sock();
>     release_sock();
>     cancel_delayed_work_sync(&strp->msg_timer_work);
>     kfree_skb(strp->skb_head);
> ```
> 
> When strp_data_ready() runs first:
> ```
>                  cpu0                                cpu1
> 
>                                          espintcp_data_ready()
>                                            strp_data_ready()
>                                              if (unlikely(strp->stopped)) return;
> espintcp_close()
>   strp_stop()
>     strp->stopped = 1;
>   strp_done()
>     lock_sock();
>                                              strp_read_sock()
>                                                tcp_read_sock()
>                                                  __tcp_read_sock()
>                                                    strp_recv()
>                                                      __strp_recv()
>                                                        head = strp->skb_head;
>                                                        strp_start_timer()
>                                                          mod_delayed_work(&strp->msg_timer_work);
>                                                        ...
>     release_sock();
>     cancel_delayed_work_sync(&strp->msg_timer_work);
>     kfree_skb(strp->skb_head);
> ```
> In both cases, the race does not appear to cause any problem.
> 
> > 
> > @@ -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
> 
> >From my perspective, adding lock_sock() inside strp_done(), as in the
> patch above, looks cleaner.
> 
> > under lock_sock() (or even just bh_lock_sock), but again I can't
> > figure out if that's ok for sockmap.
> 
> sockmap/psock has a more complex call stack compared to other strp
> users, so I'm also not entirely certain about that part.


For sockmap, I think it's not affected by this race.

The sockmap path has additional synchronization:

1. sk_psock_strp_data_ready() holds read_lock_bh(&sk->sk_callback_lock) when
calling strp_data_ready(), while sk_psock_drop() holds write_lock_bh(&sk->sk_callback_lock)
when calling strp_stop(). This rwlock serializes the stopped flag check with the stopped
flag set.

2. strp_done() is not called directly from sk_psock_drop(), but deferred via
queue_rcu_work() → sk_psock_destroy() → sk_psock_done_strp().
The RCU grace period guarantees that all in-flight sk_psock_strp_data_ready()
calls (which are within rcu_read_lock() sections) have completed before strp_done()
runs. So cancel_delayed_work_sync() / cancel_work_sync() in
strp_done() won't be defeated by a subsequent re-schedule.

I think it common practice to use strparser just like the doc says:

	void strp_done(struct strparser *strp);

     strp_done is called to release any resources held by the stream
     parser instance. This must be called after the stream processor
     has been stopped.

> 
> > 
> > 
> > > 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.
> 
> I agree with your point there. Still, after the core patches addressing
> this race are applied, I plan to resubmit the _disable patch with an
> updated commit message. I think applying that change is still beneficial.
> 
> 
> Best regards,
> Hyunwoo Kim

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

[Hyunwoo Kim]

On Wed, Mar 11, 2026 at 01:13:45PM +0900, Hyunwoo Kim wrote:
> On Fri, Mar 06, 2026 at 08:41:02PM +0900, Hyunwoo Kim wrote:
> > On Fri, Mar 06, 2026 at 11:13:19AM +0100, Sabrina Dubroca wrote:
> > > 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?
> > 
> > Looks good to me. With this change, it seems the issue can be resolved
> > not only for espintcp but for all strp users.
> > 
> > When strp_stop() runs first:
> > ```
> >                  cpu0                                cpu1
> > 
> > espintcp_close()
> >   strp_stop()
> >     strp->stopped = 1;
> >                                          espintcp_data_ready()
> >                                            strp_data_ready()
> >                                              if (unlikely(strp->stopped)) return;
> >   strp_done()
> >     lock_sock();
> >     release_sock();
> >     cancel_delayed_work_sync(&strp->msg_timer_work);
> >     kfree_skb(strp->skb_head);
> > ```
> > 
> > When strp_data_ready() runs first:
> > ```
> >                  cpu0                                cpu1
> > 
> >                                          espintcp_data_ready()
> >                                            strp_data_ready()
> >                                              if (unlikely(strp->stopped)) return;
> > espintcp_close()
> >   strp_stop()
> >     strp->stopped = 1;
> >   strp_done()
> >     lock_sock();
> >                                              strp_read_sock()
> >                                                tcp_read_sock()
> >                                                  __tcp_read_sock()
> >                                                    strp_recv()
> >                                                      __strp_recv()
> >                                                        head = strp->skb_head;
> >                                                        strp_start_timer()
> >                                                          mod_delayed_work(&strp->msg_timer_work);
> >                                                        ...
> >     release_sock();
> >     cancel_delayed_work_sync(&strp->msg_timer_work);
> >     kfree_skb(strp->skb_head);
> > ```
> > In both cases, the race does not appear to cause any problem.
> > 
> > > 
> > > @@ -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
> > 
> > From my perspective, adding lock_sock() inside strp_done(), as in the
> > patch above, looks cleaner.
> > 
> > > under lock_sock() (or even just bh_lock_sock), but again I can't
> > > figure out if that's ok for sockmap.
> > 
> > sockmap/psock has a more complex call stack compared to other strp
> > users, so I'm also not entirely certain about that part.
> 
> I looked into the sockmap/psock side. sk_psock_strp_data_ready() is protected 
> by read_lock_bh(&sk->sk_callback_lock), and during teardown sk_psock_drop() 
> performs callback restoration and strp_stop() under 
> write_lock_bh(&sk->sk_callback_lock), so sockmap/psock doesn't have this race 
> to begin with.
> 
> As for introducing this patch, in sockmap/psock strp_done() is only called 
> from sk_psock_destroy(), which is scheduled via queue_rcu_work() and runs 
> on system_percpu_wq after an RCU GP, so no locks including lock_sock are held 
> at that point. And since lock_sock is released before cancel_work_sync, 
> there's no circular dependency with do_strp_work/strp_msg_timeout either. 
> So this patch shouldn't introduce any new issues for sockmap/psock.

Hi Sabrina,

Could you please provide an update on the status of this patch?


Best regards,
Hyunwoo Kim

> 
> > 
> > > 
> > > 
> > > > 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.
> > 
> > I agree with your point there. Still, after the core patches addressing
> > this race are applied, I plan to resubmit the _disable patch with an
> > updated commit message. I think applying that change is still beneficial.
> > 
> > 
> > Best regards,
> > Hyunwoo Kim