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

[Sabrina Dubroca <sd@queasysnail.net>]

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