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

On Wed, Sep 2, 2026 at 11:23 PM xietangxin [off-list ref] wrote:

quoted

On 9/2/2026 5:46 AM, David Laight wrote:

quoted

On Tue, 1 Sep 2026 11:06:31 -0400
Xin Long [off-list ref] wrote:

quoted

On Tue, Sep 1, 2026 at 10:35 AM David Laight
[off-list ref] wrote:

quoted

On Tue, 1 Sep 2026 09:45:42 -0400
Xin Long [off-list ref] wrote:

quoted

On Mon, Aug 31, 2026 at 11:47 PM xietangxin [off-list ref] wrote:

quoted

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,

quoted

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] */
  }

quoted

quoted

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.

quoted

quoted

quoted

quoted

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.

quoted

quoted

David

quoted

Thanks.

quoted

The RHS (task 2) would need to hold bh_lock_sock().

Try giving sctp_generate_heartbeat_event() two holds.

David

quoted

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.

quoted

  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.