Re: [syzbot] [sctp?] WARNING: refcount bug in sctp_transport_put (6)

[xietangxin <xietangxin@h-partners.com>]

On 9/3/2026 10:00 PM, Xin Long wrote:
> On Wed, Sep 2, 2026 at 11:23 PM xietangxin <xietangxin@h-partners.com> wrote:
>>
>>
>>
>> On 9/2/2026 5:46 AM, David Laight wrote:
>>> On Tue, 1 Sep 2026 11:06:31 -0400
>>> Xin Long <lucien.xin@gmail.com> wrote:
>>>
>>>> On Tue, Sep 1, 2026 at 10:35 AM David Laight
>>>> <david.laight.linux@gmail.com> wrote:
>>>>>
>>>>> On Tue, 1 Sep 2026 09:45:42 -0400
>>>>> Xin Long <lucien.xin@gmail.com> wrote:
>>>>>
>>>>>> On Mon, Aug 31, 2026 at 11:47 PM xietangxin <xietangxin@h-partners.com> wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> I have analyzed this issue and successfully reproduced locally.
>>>>>>> The race occurs between the timer callback (`sctp_generate_heartbeat_event`) and
>>>>>>> the transport cleanup path (`sctp_transport_free`):
>>>>>>>
>>>>>>> Task 1(Timer Softirq)               Task 2(sctp_transport_free)
>>>>>>> ==========================              ===============================
>>>>>>> sctp_generate_heartbeat_event()
>>>>>>>   refcnt = 2
>>>>>>>
>>>>>>>   bh_lock_sock(sk)
>>>>>>>   sock_owned_by_user(sk)
>>>>>>>   mod_timer(&hb_timer) -> returns 0
>>>>>>>                                           sctp_transport_free()
>>>>>>>                                             transport->dead = 1
>>>>>>>                                             del_timer(&hb_timer) -> returns 1!
>>>>>>>                                               sctp_transport_put() (2 -> 1)
>>>>>>>                                             sctp_transport_put() (1 -> 0)
>>>>>>>                                               sctp_transport_destroy()
>>>>>>>
>>>>>>>   sctp_transport_hold()
>>>>>>>     -> refcnt is 0, increment fails
>>>>>>
>>>>>> This should not be 0, as the transport must hold a refcnt to start the
>>>>>> hb_timer.
>>>>>
>>>>> Isn't there one hold sctp_generate_heartbeat_event() and a second for
>>>>> whatever 'task 2' is doing.
>>>> Right,
>>>>
>>>>> When hb_timer is started it is given another hold (does it actually need one??).
>>>> You mean mod_timer() in sctp_generate_heartbeat_event()?  Yes, as it will
>>>> release the last one in out_unlock, it must hold a new one.
>>>
>>> But can that ever be the last 'hold' that actually calls sctp_transport_destroy().
> Sorry, by “release the last one” above, I meant releasing the previous
> “hold.”
> 
> If the hold succeeds after mod_timer(), it can never be the last/final hold
> that calls sctp_transport_destroy().
> 
> However, in the current code, there's a window between mod_timer() and
> sctp_transport_hold() at [1], sctp_transport_destroy() can be called
> to  delete the newly enqueued timer and release the previous hold.
> The hold at [2] for the newly enqueuing has not been taken yet.
> 
>   if (!mod_timer(&transport->hb_timer, jiffies + (HZ/20))) {
>           mdelay(1);  /* [1] */
>           sctp_transport_hold(transport); /* [2] */
>   }
> 
>>> It the timer is always deleted (as task 2 above) it doesn't need one itself.
>>> Might need to be del_timer_sync() so that it waits for the completion function
>>> to terminate.
>>>
> Unfortunately, del_timer_sync() can only be used in sleepable contexts.
> 
>>>>
>>>>> So when hb_timer is deleted it's hold is removed.
>>>>> But the del_timer() is happening before the the extra hold is obtained.
>>>>>
>>>> Ahh, I can see the race now.
>>>>
>>>> I remember you mentioned holding it before mod_reduce() in a previous
>>>> patch, maybe it will work here, like:
>>>>
>>>>                 sctp_transport_hold(transport);
>>>>                 if (mod_timer(&transport->hb_timer, jiffies + (HZ/20)))
>>>>                         sctp_transport_put(transport);
>>>>
>>>> Does it make sense?
>>>
>>> That should close the timing window, but is probably inefficient.
>>> Rather depends on how often the 'put' ends up being done.
>>>
> Each timer enqueue must have a corresponding hold. As long as we take the
> hold before enqueueing the timer, there is no race.
> 
>>> David
>>>
>>>>
>>>> Thanks.
>>>>
>>>>> The RHS (task 2) would need to hold bh_lock_sock().
>>>>>
>>>>> Try giving sctp_generate_heartbeat_event() two holds.
>>>>>
>>>>> David
>>>>>
>>>>>
>>>>>>
>>>>>> Also, the delay below is under bh_lock_sock(), so it should not be the
>>>>>> real cause of the issue.
>>>>>>
>>>>>> Could you share the PoC for this issue?
>>>>>>
>>>>>> Thanks.
>>>>>>
>>>>>>>   out_unlock:
>>>>>>>     sctp_transport_put() (0 -> -1)
>>>>>>>     -> refcount underflow warning!
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Adding a small delay  after `mod_timer()` increases the reproduction rate:
>>>>>>>
>>>>>>> --- a/net/sctp/sm_sideeffect.c
>>>>>>> +++ b/net/sctp/sm_sideeffect.c
>>>>>>> @@ -373,8 +373,10 @@ void sctp_generate_heartbeat_event(struct timer_list *t)
>>>>>>>                 pr_debug("%s: sock is busy\n", __func__);
>>>>>>>
>>>>>>>                 /* Try again later.  */
>>>>>>> -               if (!mod_timer(&transport->hb_timer, jiffies + (HZ/20)))
>>>>>>> +               if (!mod_timer(&transport->hb_timer, jiffies + (HZ/20))) {
>>>>>>> +                       mdelay(1);
>>>>>>>                         sctp_transport_hold(transport);
>>>>>>> +               }
>>>>>>>                 goto out_unlock;
>>>>>>>         }
>>>>>>>
>>>>>>> Any feedback or guidance would be greatly appreciated.
>>>>>>>
>>>>>>> --
>>>>>>> Best regards,
>>>>>>> Tangxin Xie
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>> Hi , David,
>>
>> I have a syzkaller execution log that can trigger this issue.
>> Please let me know if there is anything else I can do.
> 
> Hi, Tangxin,
> 
> Can you try if the change below will fix the race?
> (You may add mdelay() back for reproducing)
> 
> diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> index 94716406d602..c0fddd5e8097 100644
> --- a/net/sctp/sm_sideeffect.c
> +++ b/net/sctp/sm_sideeffect.c
> @@ -378,8 +378,9 @@ void sctp_generate_heartbeat_event(struct timer_list *t)
>                 pr_debug("%s: sock is busy\n", __func__);
> 
>                 /* Try again later.  */
> -               if (!mod_timer(&transport->hb_timer, jiffies + (HZ/20)))
> -                       sctp_transport_hold(transport);
> +               sctp_transport_hold(transport);
> +               if (mod_timer(&transport->hb_timer, jiffies + (HZ/20)))
> +                       sctp_transport_put(transport);
>                 goto out_unlock;
>         }
> 
> Thanks.
Hi Xin,

Thanks for the patch.
I have tested this fix in my local environment.

Testing results:
- Without patch: The refcount underflow issue could be reproduced
within 10 minutes using the reproducer.
- With patch applied: Ran the same reproducer continuously for
over 16 hours without encountering any issues.

-- 
Best regards,
Tangxin Xie