[PATCH net] strparser: make sure __strp_recv isn't running before tearing down the parser

[PATCH net] strparser: make sure __strp_recv isn't running before tearing down the parser

[Sabrina Dubroca]

The comment above strp_done() claims that if strp is stopped,
strp_recv will no longer be called. That's only true if the caller has
a mechanism (eg locking) to guarantee that strp_recv() calls that
started before strp_stop() have completed by the time we call
strp_done().

This adds a lock_sock/release_sock pair to guarantee that any
in-flight strp_recv() (which runs under either bh_lock_sock or
lock_sock, depending if it's called from ->sk_data_ready or strp_work)
has completed.

Only espintcp can be affected by this race condition, but this patch
makes sure no future user of strp can have the bug.

Reported-by: Hyunwoo Kim <imv4bel@gmail.com>
Link: https://lore.kernel.org/all/aZgpkyTDU3aXe_V0@v4bel/
Fixes: e27cca96cd68 ("xfrm: add espintcp (RFC 8229)")
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
This could crash on strp->sk ("general mode" of strp), but this mode
has been here for 9 years and has never been used. I'll send a
net-next patch to get rid of it.

 net/strparser/strparser.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/strparser/strparser.c b/net/strparser/strparser.c
index a23f4b4dfc67..401e67533005 100644
--- a/net/strparser/strparser.c
+++ b/net/strparser/strparser.c
@@ -511,6 +511,10 @@ void strp_done(struct strparser *strp)
 {
 	WARN_ON(!strp->stopped);
 
+	lock_sock(strp->sk);
+	/* sync with pending strp_recv */
+	release_sock(strp->sk);
+
 	cancel_delayed_work_sync(&strp->msg_timer_work);
 	cancel_work_sync(&strp->work);
 
-- 
2.55.0

Re: [PATCH net] strparser: make sure __strp_recv isn't running before tearing down the parser

[Sabrina Dubroca]

2026-09-25, 15:51:58 +0200, Sabrina Dubroca wrote:
> The comment above strp_done() claims that if strp is stopped,
> strp_recv will no longer be called. That's only true if the caller has
> a mechanism (eg locking) to guarantee that strp_recv() calls that
> started before strp_stop() have completed by the time we call
> strp_done().
> 
> This adds a lock_sock/release_sock pair to guarantee that any
> in-flight strp_recv() (which runs under either bh_lock_sock or
> lock_sock, depending if it's called from ->sk_data_ready or strp_work)
> has completed.
> 
> Only espintcp can be affected by this race condition, but this patch
> makes sure no future user of strp can have the bug.
> 
> Reported-by: Hyunwoo Kim <imv4bel@gmail.com>
> Link: https://lore.kernel.org/all/aZgpkyTDU3aXe_V0@v4bel/
> Fixes: e27cca96cd68 ("xfrm: add espintcp (RFC 8229)")
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>

And obviously I forgot the Cc: stable tag once more [*]. I guess I'll
post a v2 tomorrow if I don't get any comments on this today.

[*] but hopefully not again:
https://lore.kernel.org/all/1e5824f7bb8f5d32783c5b47ef4c06b1fc59969a.1790437098.git.sd@queasysnail.net/

-- 
Sabrina

Re: [PATCH net] strparser: make sure __strp_recv isn't running before tearing down the parser

[Hyunwoo Kim]

On Fri, Sep 25, 2026 at 03:51:58PM +0200, Sabrina Dubroca wrote:
> The comment above strp_done() claims that if strp is stopped,
> strp_recv will no longer be called. That's only true if the caller has
> a mechanism (eg locking) to guarantee that strp_recv() calls that
> started before strp_stop() have completed by the time we call
> strp_done().
> 
> This adds a lock_sock/release_sock pair to guarantee that any
> in-flight strp_recv() (which runs under either bh_lock_sock or
> lock_sock, depending if it's called from ->sk_data_ready or strp_work)
> has completed.
> 
> Only espintcp can be affected by this race condition, but this patch
> makes sure no future user of strp can have the bug.
> 
> Reported-by: Hyunwoo Kim <imv4bel@gmail.com>
> Link: https://lore.kernel.org/all/aZgpkyTDU3aXe_V0@v4bel/
> Fixes: e27cca96cd68 ("xfrm: add espintcp (RFC 8229)")
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>

Reviewed-by: Hyunwoo Kim <imv4bel@gmail.com>

LGTM. Thanks.


Best regards,
Hyunwoo Kim

Re: [PATCH net] strparser: make sure __strp_recv isn't running before tearing down the parser

[Jiayuan Chen]

On 9/25/26 9:51 PM, Sabrina Dubroca wrote:
> The comment above strp_done() claims that if strp is stopped,
> strp_recv will no longer be called. That's only true if the caller has
> a mechanism (eg locking) to guarantee that strp_recv() calls that
> started before strp_stop() have completed by the time we call
> strp_done().
>
> This adds a lock_sock/release_sock pair to guarantee that any
> in-flight strp_recv() (which runs under either bh_lock_sock or
> lock_sock, depending if it's called from ->sk_data_ready or strp_work)
> has completed.
>
> Only espintcp can be affected by this race condition, but this patch
> makes sure no future user of strp can have the bug.
>
> Reported-by: Hyunwoo Kim <imv4bel@gmail.com>
> Link: https://lore.kernel.org/all/aZgpkyTDU3aXe_V0@v4bel/
> Fixes: e27cca96cd68 ("xfrm: add espintcp (RFC 8229)")
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> ---
> This could crash on strp->sk ("general mode" of strp), but this mode
> has been here for 9 years and has never been used. I'll send a
> net-next patch to get rid of it.
>
>   net/strparser/strparser.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/net/strparser/strparser.c b/net/strparser/strparser.c
> index a23f4b4dfc67..401e67533005 100644
> --- a/net/strparser/strparser.c
> +++ b/net/strparser/strparser.c
> @@ -511,6 +511,10 @@ void strp_done(struct strparser *strp)
>   {
>   	WARN_ON(!strp->stopped);
>   
> +	lock_sock(strp->sk);
> +	/* sync with pending strp_recv */
> +	release_sock(strp->sk);
> +


Using sk directly inside strparser breaks the abstraction and that's why 
cb exists.

We can totally use it instead
     strp->cb.lock(strp);
     strp->cb.unlock(strp);


>   	cancel_delayed_work_sync(&strp->msg_timer_work);
>   	cancel_work_sync(&strp->work);
>   

Re: [PATCH net] strparser: make sure __strp_recv isn't running before tearing down the parser

[Sabrina Dubroca]

2026-09-28, 17:23:57 +0800, Jiayuan Chen wrote:
> 
> On 9/25/26 9:51 PM, Sabrina Dubroca wrote:
> > The comment above strp_done() claims that if strp is stopped,
> > strp_recv will no longer be called. That's only true if the caller has
> > a mechanism (eg locking) to guarantee that strp_recv() calls that
> > started before strp_stop() have completed by the time we call
> > strp_done().
> > 
> > This adds a lock_sock/release_sock pair to guarantee that any
> > in-flight strp_recv() (which runs under either bh_lock_sock or
> > lock_sock, depending if it's called from ->sk_data_ready or strp_work)
> > has completed.
> > 
> > Only espintcp can be affected by this race condition, but this patch
> > makes sure no future user of strp can have the bug.
> > 
> > Reported-by: Hyunwoo Kim <imv4bel@gmail.com>
> > Link: https://lore.kernel.org/all/aZgpkyTDU3aXe_V0@v4bel/
> > Fixes: e27cca96cd68 ("xfrm: add espintcp (RFC 8229)")
> > Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> > ---
> > This could crash on strp->sk ("general mode" of strp), but this mode
> > has been here for 9 years and has never been used. I'll send a
> > net-next patch to get rid of it.
> > 
> >   net/strparser/strparser.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> > 
> > diff --git a/net/strparser/strparser.c b/net/strparser/strparser.c
> > index a23f4b4dfc67..401e67533005 100644
> > --- a/net/strparser/strparser.c
> > +++ b/net/strparser/strparser.c
> > @@ -511,6 +511,10 @@ void strp_done(struct strparser *strp)
> >   {
> >   	WARN_ON(!strp->stopped);
> > +	lock_sock(strp->sk);
> > +	/* sync with pending strp_recv */
> > +	release_sock(strp->sk);
> > +
> 
> 
> Using sk directly inside strparser breaks the abstraction and that's why cb
> exists.
> 
> We can totally use it instead
>     strp->cb.lock(strp);
>     strp->cb.unlock(strp);

That's what I'm saying above the diffstat. This cb abstraction hasn't
been used in the 9 years it's existed, we should get rid of those
useless indirections (I'll have the patch out on netdev this week).

-- 
Sabrina