[PATCH net v3 1/2] s390/ctcm: Fix timer corruption in fsm_addtimer()

[PATCH net v3 1/2] s390/ctcm: Fix timer corruption in fsm_addtimer()

[Nagamani PV]

fsm_addtimer() calls timer_setup() unconditionally before add_timer().
If called on an already-pending timer, timer_setup() re-initializes
the timer's list_head fields while the timer is still enqueued in the
wheel, corrupting the timer list.

The timer is already initialized once by fsm_settimer() which calls
timer_setup() correctly. Multiple callsites invoke fsm_addtimer()
without a preceding fsm_deltimer(), including ctcm_main.c
ctcmpc_send_sweep_req() and ctcm_mpc.c mpc_action_side_xid(), making the
redundant timer_setup() in fsm_addtimer() a real corruption risk.

Remove the redundant timer_setup() calls from fsm_addtimer() and
fsm_modtimer(), and replace add_timer() with mod_timer() which safely
handles both pending and non-pending timers atomically without
corrupting the timer wheel.

Fixes: e99e88a9d2b0 ("treewide: setup_timer() -> timer_setup()")
Cc: stable@vger.kernel.org
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://sashiko.dev/#/patchset/20260803182736.2356374-1-nagamani@linux.ibm.com?part=1
Reviewed-by: Aswin Karuvally <aswin@linux.ibm.com>
Tested-by: Aswin Karuvally <aswin@linux.ibm.com>
Signed-off-by: Nagamani PV <nagamani@linux.ibm.com>
---
Changes in v3:
- No changes to this patch.

Changes in v2:
- fix function name ctcm_send_sweep() -> ctcmpc_send_sweep_req()
  in the commit message (Sashiko netdev-bot)
- call mod_timer() then return 0 explicitly, preserving the
  "Always returns 0" contract documented in fsm.h (Sashiko netdev-bot)
---
 drivers/s390/net/fsm.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/s390/net/fsm.c b/drivers/s390/net/fsm.c
index 9b8622aef807..936ed0bc8401 100644
--- a/drivers/s390/net/fsm.c
+++ b/drivers/s390/net/fsm.c
@@ -171,11 +171,9 @@ fsm_addtimer(fsm_timer *this, int millisec, int event, void *arg)
 	       this->fi->name, this, millisec);
 #endif
 
-	timer_setup(&this->tl, fsm_expire_timer, 0);
 	this->expire_event = event;
 	this->event_arg = arg;
-	this->tl.expires = jiffies + (millisec * HZ) / 1000;
-	add_timer(&this->tl);
+	mod_timer(&this->tl, jiffies + msecs_to_jiffies(millisec));
 	return 0;
 }
 
@@ -189,12 +187,9 @@ fsm_modtimer(fsm_timer *this, int millisec, int event, void *arg)
 		this->fi->name, this, millisec);
 #endif
 
-	timer_delete(&this->tl);
-	timer_setup(&this->tl, fsm_expire_timer, 0);
 	this->expire_event = event;
 	this->event_arg = arg;
-	this->tl.expires = jiffies + (millisec * HZ) / 1000;
-	add_timer(&this->tl);
+	mod_timer(&this->tl, jiffies + msecs_to_jiffies(millisec));
 }
 
 EXPORT_SYMBOL(init_fsm);
-- 
2.53.0

[PATCH net v3 2/2] s390/ctcm: Fix use-after-free in channel_remove()

[Nagamani PV]

channel_remove() calls fsm_deltimer(), which internally uses
timer_delete(), then immediately frees the channel structure:

  fsm_deltimer(&ch->timer);
  kfree_fsm(ch->fsm);       /* freed while callback may still run */
  kfree(ch);

timer_delete() returns immediately even if the timer callback is
currently executing on another CPU, creating a window where
fsm_expire_timer() accesses this->fi (which points to ch->fsm)
after it has been freed by kfree_fsm().

Fix this by calling timer_delete_sync() directly on the underlying
timer_list fields before freeing, instead of going through
fsm_deltimer(). This cannot be fixed in fsm_deltimer() itself because
FSM action functions triggered by CTC_EVENT_TIMER call fsm_deltimer()
from within the timer callback chain, which would cause a self-deadlock in
timer_delete_sync().

For MPC channels, two additional problems exist. First, freeing
ch->fsm before tasklet_kill() leaves a window where ch_tasklet
(ctcmpc_bh) or ch_disc_tasklet could access the freed ch->fsm. Second,
ch_tasklet can re-arm sweep_timer via ctcmpc_send_sweep_resp(), and
sweep_timer's callback re-arms ch->timer via ctcmpc_chx_send_sweep().

Kill both tasklets before stopping the timers: this closes the freed
ch->fsm access window and eliminates the tasklet-driven sweep_timer
re-arm source. Then use timer_shutdown_sync() for sweep_timer to wait
for any running callback and prevent further re-arms of ch->timer from
sweep_timer. kfree(ch->discontact_th) is moved into this MPC block
after tasklet_kill(ch_disc_tasklet), since mpc_action_send_discontact()
accesses discontact_th.

timer_delete_sync() is retained for ch->timer because normal FSM timer
callbacks can delete and re-arm ch->timer; shutting it down would break
normal operation.

Patch 1 changes fsm_addtimer() to use mod_timer() without reinitializing
the timer, which is required for timer_shutdown_sync() to reliably
prevent subsequent re-arms.

Backporting patch 2 without patch 1 is not safe because the old
fsm_addtimer() reinitializes the timer with timer_setup() before
add_timer().

Fixes: 293d984f0e36 ("ctcm: infrastructure for replaced ctc driver")
Cc: stable@vger.kernel.org
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://sashiko.dev/#/patchset/20260803182736.2356374-1-nagamani@linux.ibm.com?part=1
Signed-off-by: Nagamani PV <nagamani@linux.ibm.com>
---
Changes in v3:
- Kill ch_tasklet and ch_disc_tasklet before stopping the timers and
  freeing ch->fsm: closes freed ch->fsm access from tasklet context and
  eliminates tasklet-driven sweep_timer re-arm.
- Shut down sweep_timer with timer_shutdown_sync() before deleting
  ch->timer to prevent sweep_timer callback from re-arming ch->timer.
- Move kfree(ch->discontact_th) after tasklet_kill(ch_disc_tasklet).
- Drop Reviewed-by and Tested-by; please re-review and re-test the
  new teardown order.

Changes in v2:
- add Fixes: and Cc: stable@vger.kernel.org tags (Sashiko netdev-bot)
---
 drivers/s390/net/ctcm_main.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/s390/net/ctcm_main.c b/drivers/s390/net/ctcm_main.c
index 8b0d76a47d9f..5053737f11fc 100644
--- a/drivers/s390/net/ctcm_main.c
+++ b/drivers/s390/net/ctcm_main.c
@@ -211,9 +211,13 @@ static void channel_remove(struct channel *ch)
 	while (*c) {
 		if (*c == ch) {
 			*c = ch->next;
-			fsm_deltimer(&ch->timer);
-			if (IS_MPC(ch))
-				fsm_deltimer(&ch->sweep_timer);
+			if (IS_MPC(ch)) {
+				tasklet_kill(&ch->ch_tasklet);
+				tasklet_kill(&ch->ch_disc_tasklet);
+				timer_shutdown_sync(&ch->sweep_timer.tl);
+				kfree(ch->discontact_th);
+			}
+			timer_delete_sync(&ch->timer.tl);
 
 			kfree_fsm(ch->fsm);
 			clear_normalized_cda(&ch->ccw[4]);
@@ -221,11 +225,6 @@ static void channel_remove(struct channel *ch)
 				clear_normalized_cda(&ch->ccw[1]);
 				dev_kfree_skb_any(ch->trans_skb);
 			}
-			if (IS_MPC(ch)) {
-				tasklet_kill(&ch->ch_tasklet);
-				tasklet_kill(&ch->ch_disc_tasklet);
-				kfree(ch->discontact_th);
-			}
 			kfree(ch->ccw);
 			kfree(ch->irb);
 			kfree(ch);
-- 
2.53.0

Re: [PATCH net v3 2/2] s390/ctcm: Fix use-after-free in channel_remove()

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
s390/ctcm: Fix use-after-free in channel_remove()

The patch changes channel_remove() so that it no longer calls
fsm_deltimer(). It calls timer_delete_sync() and timer_shutdown_sync() on
the underlying timer_list instead. It also moves the MPC tasklet_kill()
calls and kfree(ch->discontact_th) ahead of kfree_fsm(ch->fsm). The aim is
to stop timer and tasklet callbacks from running against freed channel
memory.

Link: https://sashiko.dev/#/patchset/20260803182736.2356374-1-nagamani@linux.ibm.com?part=1

> Patch 1 changes fsm_addtimer() to use mod_timer() without reinitializing
> the timer, which is required for timer_shutdown_sync() to reliably
> prevent subsequent re-arms.
>
> Backporting patch 2 without patch 1 is not safe because the old
> fsm_addtimer() reinitializes the timer with timer_setup() before
> add_timer().

[Severity: Low]
This isn't a bug, but "Patch 1" and "patch 2" won't mean anything once
these commits are in git history.

The prerequisite is "s390/ctcm: Fix timer corruption in fsm_addtimer()".
Its Fixes: tag (e99e88a9d2b0) is different from the one on this commit
(293d984f0e36). Without it, fsm_addtimer() calls timer_setup() again, which
restores ->function and undoes timer_shutdown_sync(&ch->sweep_timer.tl).

Could the commit message name the prerequisite by title or SHA? Could the
Cc: stable line also use the "Cc: stable@vger.kernel.org # <ver>: <sha>:
<title>" form, so the dependency is recorded in a way tools can pick up?

> diff --git a/drivers/s390/net/ctcm_main.c b/drivers/s390/net/ctcm_main.c
> index 8b0d76a47d9f0..5053737f11fc7 100644
> --- a/drivers/s390/net/ctcm_main.c
> +++ b/drivers/s390/net/ctcm_main.c
> @@ -211,9 +211,13 @@ static void channel_remove(struct channel *ch)
>  	while (*c) {
>  		if (*c == ch) {
>  			*c = ch->next;
> -			fsm_deltimer(&ch->timer);
> -			if (IS_MPC(ch))
> -				fsm_deltimer(&ch->sweep_timer);
> +			if (IS_MPC(ch)) {
> +				tasklet_kill(&ch->ch_tasklet);
> +				tasklet_kill(&ch->ch_disc_tasklet);

[Severity: High]
Is this quiesce early enough? On the offline path, channel_remove() only
runs after ctcm_free_netdevice() has already freed the shared MPC state:

ctcm_shutdown_device()
    channel_free(priv->channel[CTCM_READ])
    channel_free(priv->channel[CTCM_WRITE])
    unregister_netdev(dev)
    ctcm_free_netdevice(dev)
        kfree_fsm(grp->fsm)
        kfree(grp)
        priv->mpcg = NULL
        kfree_fsm(priv->fsm)
        free_netdev(dev)        /* MODULE builds only */
    ...
    channel_remove(priv->channel[CTCM_READ])
    channel_remove(priv->channel[CTCM_WRITE])

ctcmpc_bh(), the ch_tasklet handler, dereferences these objects without
checking the channel FSM state. Setting CTC_STATE_IDLE in channel_free()
therefore doesn't stop a tasklet that is already pending:

drivers/s390/net/ctcm_mpc.c:ctcmpc_bh() {
	struct net_device *dev	= ch->netdev;
	struct ctcm_priv  *priv	= dev->ml_priv;
	struct mpc_group  *grp	= priv->mpcg;
    ...
	while ((fsm_getstate(grp->fsm) != MPCG_STATE_FLOWC) &&
    ...
}

Suppose the RX path scheduled rch->ch_tasklet just before the device was
taken offline, and the tasklet is still pending (for example deferred to
ksoftirqd) when ctcm_free_netdevice() runs. Can it then dereference a NULL
priv->mpcg, or a freed dev in MODULE builds, before channel_remove() gets
to tasklet_kill()?

Something similar can happen with sweep_timer. An in-flight action in
ctcmpc_chx_send_sweep() can overwrite the CTC_STATE_IDLE set by
channel_free() and re-arm wch->timer:

	fsm_addtimer(&wch->timer, CTCM_TIME_5_SEC, CTC_EVENT_TIMER, wch);
	fsm_newstate(wch->fsm, CTC_STATE_TX);

If that timer fires before channel_remove(), the TX timer action would use
ch->netdev and priv->mpcg, which by then are freed or NULL.

The error path in ctcm_new_device() has the same order: it calls
ctcm_free_netdevice() at out_dev and channel_remove() on both channels
afterwards.

Would it make sense to quiesce the tasklets and timers of both channels
before ctcm_free_netdevice() is called?

> +				timer_shutdown_sync(&ch->sweep_timer.tl);
> +				kfree(ch->discontact_th);
> +			}
> +			timer_delete_sync(&ch->timer.tl);

[Severity: Medium]
Can a timer callback schedule ch_disc_tasklet again after the
tasklet_kill() above?

tasklet_kill() waits for the current run and clears TASKLET_STATE_SCHED.
It doesn't stop a later tasklet_hi_schedule(), and the timer actions can
reach one. ctcmpc_chx_send_sweep() raises MPCG_EVENT_INOP when
set_normalized_cda() fails:

	if (set_normalized_cda(&wch->ccw[4], skb->data)) {
		...
		fsm_event(grp->fsm, MPCG_EVENT_INOP, dev);

CTC_EVENT_TIMER actions such as ctcm_chx_txretry() can also end up raising
MPCG_EVENT_INOP. mpc_action_go_inop() then does:

	case MPCG_STATE_FLOWC:
	case MPCG_STATE_READY:
	default:
		tasklet_hi_schedule(&wch->ch_disc_tasklet);

Consider a timer action that read a non-IDLE state before channel_free()
and is still running across the tasklet_kill() calls.
timer_shutdown_sync() and timer_delete_sync() wait for that callback, but
not for the tasklet it queued. mpc_action_send_discontact() would then run
on the freed ch and start I/O through ccw[15]. add_channel() pointed
ccw[15].cda at the discontact_th buffer, which is freed just above.

On the ctcm_shutdown_device() path, the same callback would also touch the
grp->fsm that ctcm_free_netdevice() has already freed. The window is
narrow, but nothing blocks the reschedule.

Would a second tasklet_kill() after the timers are synced, or a
tasklet_disable() before them, close this?

>  
>  			kfree_fsm(ch->fsm);

[Severity: High]
This isn't a bug introduced by this patch, but the same removal sequence
seems to leave two other FSM timers running on freed memory.

ctcmpc_init_mpc_group() binds grp->timer, which is embedded in grp, with
fsm_settimer(grp->fsm, &grp->timer). ctcm_init_netdevice() binds
priv->restart_timer to priv->fsm. ctcm_free_netdevice() frees all of these
without first cancelling either timer synchronously:

drivers/s390/net/ctcm_main.c:ctcm_free_netdevice() {
    ...
		if (grp) {
			if (grp->fsm)
				kfree_fsm(grp->fsm);
			...
			kfree(grp);
			priv->mpcg = NULL;
		}
		if (priv->fsm) {
			kfree_fsm(priv->fsm);
			priv->fsm = NULL;
		}
    ...
}

priv, which embeds restart_timer, is freed later in ctcm_remove_device().

For MPC devices ctcm_close() does nothing, so no FSM event stops these
timers during ctcm_shutdown_device(). grp->timer is armed for
MPC_XID_TIMEOUT_VALUE during XID negotiation. restart_timer is armed for
500 ms by mpc_action_go_inop() and for 1 s by dev_action_restart(). With
port_persist == 1 and an unreachable peer, restart_timer keeps cycling.

If the MPC device is taken offline while either timer is pending,
fsm_expire_timer() would call fsm_event(this->fi, ...) on a freed
fsm_instance. For grp->timer, the timer_list itself also lives in freed
memory, which can corrupt the timer wheel. In MODULE builds the dev event
argument has been freed by free_netdev() as well.

Should grp->timer and priv->restart_timer be stopped with
timer_delete_sync() or timer_shutdown_sync() before ctcm_free_netdevice()
frees them?

[ ... ]

Re: [PATCH net v3 2/2] s390/ctcm: Fix use-after-free in channel_remove()

[Nagamani PV]

On 29/09/26 5:04 AM, Jakub Kicinski wrote:

Thank you for the detailed review. Responses inline.

> [Severity: Low]
> This isn't a bug, but "Patch 1" and "patch 2" won't mean anything once
> these commits are in git history.
> 
> The prerequisite is "s390/ctcm: Fix timer corruption in fsm_addtimer()".
> Its Fixes: tag (e99e88a9d2b0) is different from the one on this commit
> (293d984f0e36). Without it, fsm_addtimer() calls timer_setup() again, which
> restores ->function and undoes timer_shutdown_sync(&ch->sweep_timer.tl).
> 
> Could the commit message name the prerequisite by title or SHA? Could the
> Cc: stable line also use the "Cc: stable@vger.kernel.org # <ver>: <sha>:
> <title>" form, so the dependency is recorded in a way tools can pick up?
> 

Fixed in v4:
- Named the prerequisite by commit title ("s390/ctcm: Fix timer
  corruption in fsm_addtimer()") throughout the commit message.
- Updated the tag to structured format:
  Cc: stable@vger.kernel.org # depends on: s390/ctcm: Fix timer corruption in fsm_addtimer()

> 
> [Severity: High]
> Is this quiesce early enough? On the offline path, channel_remove() only
> runs after ctcm_free_netdevice() has already freed the shared MPC state:
> 
> ctcm_shutdown_device()
>     channel_free(priv->channel[CTCM_READ])
>     channel_free(priv->channel[CTCM_WRITE])
>     unregister_netdev(dev)
>     ctcm_free_netdevice(dev)
>         kfree_fsm(grp->fsm)
>         kfree(grp)
>         priv->mpcg = NULL
>         kfree_fsm(priv->fsm)
>         free_netdev(dev)        /* MODULE builds only */
>     ...
>     channel_remove(priv->channel[CTCM_READ])
>     channel_remove(priv->channel[CTCM_WRITE])
> 
> ctcmpc_bh(), the ch_tasklet handler, dereferences these objects without
> checking the channel FSM state. Setting CTC_STATE_IDLE in channel_free()
> therefore doesn't stop a tasklet that is already pending:
> 
> drivers/s390/net/ctcm_mpc.c:ctcmpc_bh() {
> 	struct net_device *dev	= ch->netdev;
> 	struct ctcm_priv  *priv	= dev->ml_priv;
> 	struct mpc_group  *grp	= priv->mpcg;
>     ...
> 	while ((fsm_getstate(grp->fsm) != MPCG_STATE_FLOWC) &&
>     ...
> }
> 
> Suppose the RX path scheduled rch->ch_tasklet just before the device was
> taken offline, and the tasklet is still pending (for example deferred to
> ksoftirqd) when ctcm_free_netdevice() runs. Can it then dereference a NULL
> priv->mpcg, or a freed dev in MODULE builds, before channel_remove() gets
> to tasklet_kill()?
> 
> Something similar can happen with sweep_timer. An in-flight action in
> ctcmpc_chx_send_sweep() can overwrite the CTC_STATE_IDLE set by
> channel_free() and re-arm wch->timer:
> 
> 	fsm_addtimer(&wch->timer, CTCM_TIME_5_SEC, CTC_EVENT_TIMER, wch);
> 	fsm_newstate(wch->fsm, CTC_STATE_TX);
> 
> If that timer fires before channel_remove(), the TX timer action would use
> ch->netdev and priv->mpcg, which by then are freed or NULL.
> 
> The error path in ctcm_new_device() has the same order: it calls
> ctcm_free_netdevice() at out_dev and channel_remove() on both channels
> afterwards.
> 
> Would it make sense to quiesce the tasklets and timers of both channels
> before ctcm_free_netdevice() is called?
> 

Confirmed pre-existing bug in ctcm_shutdown_device() / ctcm_new_device()
error path, outside the scope of channel_remove(). Aswin Karuvally has
picked this up and is preparing a fix in a follow-up series.

> 
> [Severity: Medium]
> Can a timer callback schedule ch_disc_tasklet again after the
> tasklet_kill() above?
> 
> tasklet_kill() waits for the current run and clears TASKLET_STATE_SCHED.
> It doesn't stop a later tasklet_hi_schedule(), and the timer actions can
> reach one. 
> [ ... ]> Would a second tasklet_kill() after the timers are synced, or a
> tasklet_disable() before them, close this?
> 

Fixed in v4 by shutting down timers before tasklet_kill():

  if (IS_MPC(ch))
      timer_shutdown_sync(&ch->sweep_timer.tl);
  timer_shutdown_sync(&ch->timer.tl);
  if (IS_MPC(ch)) {
      tasklet_kill(&ch->ch_tasklet);
      tasklet_kill(&ch->ch_disc_tasklet);
      kfree(ch->discontact_th);
  }

timer_shutdown_sync() waits for any active callback and prevents further
rearming. Shutting down sweep_timer first breaks the sweep -> ch->timer
re-arm chain, and shutting down ch->timer prevents scheduling
ch_disc_tasklet via mpc_action_go_inop(). tasklet_kill() then cleans up
any in-flight instances.

timer_shutdown_sync() is also used for non-MPC ch->timer since
ctcm_chx_txretry() can re-arm ch->timer on that path as well.

> 
> [Severity: High]
> This isn't a bug introduced by this patch, but the same removal sequence
> seems to leave two other FSM timers running on freed memory.
> 
> ctcmpc_init_mpc_group() binds grp->timer, which is embedded in grp, with
> fsm_settimer(grp->fsm, &grp->timer). ctcm_init_netdevice() binds
> priv->restart_timer to priv->fsm. ctcm_free_netdevice() frees all of these
> without first cancelling either timer synchronously:
> 
> [ ... ]

> Should grp->timer and priv->restart_timer be stopped with
> timer_delete_sync() or timer_shutdown_sync() before ctcm_free_netdevice()
> frees them?
> 
Confirmed pre-existing bug in ctcm_free_netdevice(). Aswin Karuvally has
picked this up for the follow-up series as well.

These changes are included in the v4 series posted to the list.

Nagamani