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

[Xin Long]

On Wed, Sep 2, 2026 at 11:23 PM xietangxin <xietangxin@xxxxxxxxxxxxxx> wrote:
>
>
>
> On 9/2/2026 5:46 AM, David Laight wrote:
> > On Tue, 1 Sep 2026 11:06:31 -0400
> > Xin Long <lucien.xin@xxxxxxxxx> wrote:
> >
> >> On Tue, Sep 1, 2026 at 10:35 AM David Laight
> >> <david.laight.linux@xxxxxxxxx> wrote:
> >>>
> >>> On Tue, 1 Sep 2026 09:45:42 -0400
> >>> Xin Long <lucien.xin@xxxxxxxxx> wrote:
> >>>
> >>>> On Mon, Aug 31, 2026 at 11:47 PM xietangxin <xietangxin@xxxxxxxxxxxxxx> 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 Xin, 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.