[PATCH net 0/2] s390/ctcm: Fix timer bugs in fsm.c

[PATCH net 0/2] s390/ctcm: Fix timer bugs in fsm.c

[Nagamani PV]

Fix two bugs in drivers/s390/net/fsm.c found by Sashiko AI code review.

Patch 1 fixes timer list corruption when fsm_addtimer() is called on
an already-pending timer - timer_setup() re-initializes the timer
list_head while it is still enqueued in the wheel.

Patch 2 fixes a use-after-free in channel_remove() - timer_delete()
returns before any running callback finishes, leaving a window where
the timer callback can access freed memory. Fix by waiting for any
running callback to complete before freeing the channel.

Nagamani PV (2):
  s390/ctcm: fix timer corruption in fsm_addtimer()
  s390/ctcm: fix use-after-free in channel_remove()

 drivers/s390/net/ctcm_main.c |  4 ++--
 drivers/s390/net/fsm.c       | 10 ++--------
 2 files changed, 4 insertions(+), 10 deletions(-)

-- 
2.47.1

[PATCH net 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
ctcm_send_sweep() 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>
---
 drivers/s390/net/fsm.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/s390/net/fsm.c b/drivers/s390/net/fsm.c
index 9b8622aef807..8d2e9bbb74a2 100644
--- a/drivers/s390/net/fsm.c
+++ b/drivers/s390/net/fsm.c
@@ -171,12 +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);
-	return 0;
+	return mod_timer(&this->tl, jiffies + msecs_to_jiffies(millisec));
 }
 
 /* FIXME: this function is never used, why */
@@ -189,12 +186,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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Use-After-Free during teardown due to non-synchronous timer cancellation of grp->timer and priv->restart_timer
--

commit 6ecc23adfe371b690518fad3a360058f37350370
Author: Nagamani PV <nagamani@linux.ibm.com>
Subject: s390/ctcm: Fix timer corruption in fsm_addtimer()

fsm_addtimer() calls timer_setup() unconditionally before add_timer(), which
corrupts the timer list if the timer is already pending. This commit removes
the redundant timer_setup() calls from fsm_addtimer() and fsm_modtimer(),
and replaces add_timer() with mod_timer().

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

> diff --git a/drivers/s390/net/fsm.c b/drivers/s390/net/fsm.c
> index 9b8622aef807b..8d2e9bbb74a23 100644
> --- a/drivers/s390/net/fsm.c
> +++ b/drivers/s390/net/fsm.c

[ ... ]

> @@ -189,12 +186,9 @@ fsm_modtimer(fsm_timer *this, int millisec, int event, void *arg)
>  		this->fi->name, this, millisec);
>  #endif
>  
> -	timer_delete(&this->tl);

[Severity: High]
This is a pre-existing issue, but while reviewing the timer handling changes
here, I noticed a potential Use-After-Free during device teardown due to
fsm_deltimer() using the non-synchronous timer_delete().

While a later patch in this series ("s390/ctcm: Fix use-after-free in
channel_remove()") correctly avoids fsm_deltimer() in favor of
timer_delete_sync() for ch->timer, it appears the same vulnerability remains
for grp->timer and priv->restart_timer.

During device unbind or offline actions, the driver calls fsm_deltimer()
to cancel these timers and then immediately frees the enclosing structures.

For instance, in drivers/s390/net/ctcm_main.c:ctcm_free_netdevice():

	tasklet_kill(&grp->mpc_tasklet2);
	kfree(grp);

And similarly in ctcm_remove_device():

	if (cgdev->state == CCWGROUP_ONLINE)
		ctcm_shutdown_device(cgdev);
	dev_set_drvdata(&cgdev->dev, NULL);
	kfree(priv);

If the fsm_expire_timer() callback is executing concurrently on another CPU
when kfree() is called, could it dereference the freed grp or priv
structures?

Would it make sense to update fsm_deltimer() to use timer_delete_sync() to
safely wait for concurrent callbacks, or otherwise ensure synchronization
during teardown?

> -	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));
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907144511.3810194-1-nagamani@linux.ibm.com?part=1

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 (1) · High: 1 (1) · Medium: 0 (2) · Low: 2

- [High] Incomplete fix: the commit removes timer_setup() from
  fsm_addtimer()/fsm_modtimer() on the premise that "[t]he timer is…
- [Low] Documented return contract of the exported helper fsm_addtimer()
  is now false.
- [Low] The commit message justifies reachability with "including
  ctcm_main.c ctcm_send_sweep() and ctcm_mpc.c mpc_action_side_xid()",…

Pre-existing issues:
- [Critical] Invalid/double free during MPC device teardown:
  ctcm_free_netdevice() calls kfree(priv->xid) on a pointer that is not…
- [High] FSM timer teardown in ctcm_free_netdevice() still frees objects
  that an armed/executing fsm timer dereferences.
- [Medium] ctcm_init_netdevice() leaks priv->fsm when MPC group
  initialization fails.
- [Medium] fsm_addtimer()/fsm_modtimer() publish this->expire_event and
  this->event_arg with plain, unlocked stores immediately before…

--

s390/ctcm: Fix timer corruption in fsm_addtimer()

This removes the redundant timer_setup() from fsm_addtimer() and
fsm_modtimer() and replaces the open-coded expires assignment plus
add_timer() with mod_timer(), so that re-arming an already-pending fsm
timer no longer re-initializes it while it is enqueued in the wheel.

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

A few questions below, one about the changelog and the rest about the
resulting timer lifetime in the ctcm driver.

The changelog says:

    Multiple callsites invoke fsm_addtimer() without a preceding
    fsm_deltimer(), including ctcm_main.c ctcm_send_sweep() and
    ctcm_mpc.c mpc_action_side_xid()

[Severity: Low]
There is no symbol named ctcm_send_sweep() in the tree. The ctcm_main.c
call site that arms a timer without a preceding fsm_deltimer() appears to
be ctcmpc_send_sweep_req():

	fsm_addtimer(&ch->sweep_timer, 100, CTC_EVENT_RSWEEP_TIMER, ch);

The other cited site, mpc_action_side_xid() in ctcm_mpc.c, does exist.
Could the function name be corrected, given this patch is tagged for
stable and the changelog is what backporters use to verify the premise?

> diff --git a/drivers/s390/net/fsm.c b/drivers/s390/net/fsm.c
> index 9b8622aef807b..8d2e9bbb74a23 100644
> --- a/drivers/s390/net/fsm.c
> +++ b/drivers/s390/net/fsm.c
> @@ -171,12 +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);

[Severity: High]
With this removed, fsm_settimer() becomes the only timer_setup() call site
for fsm timers, and it is also called unconditionally. Does that leave the
same corruption reachable for priv->restart_timer?

fsm_settimer(priv->fsm, &priv->restart_timer) runs from
ctcm_init_netdevice() on every ccwgroup set_online, while struct ctcm_priv
(which embeds restart_timer) is allocated once in ctcm_probe_device() and
freed only in ctcm_remove_device().

No offline path disarms restart_timer. dev_action_restart() arms it for
CTCM_TIME_1_SEC / CTCM_TIME_5_SEC:

	fsm_addtimer(&priv->restart_timer, restart_timer,
			DEV_EVENT_START, dev);

only dev_action_start() deletes it, and the offline sequence

	ctcm_shutdown_device() -> ctcm_close() -> dev_action_stop()
		-> ctcm_free_netdevice()

never calls fsm_deltimer(&priv->restart_timer). So if the device is set
online again inside that 1-5 s window, doesn't fsm_settimer() call
timer_setup() on a still-enqueued timer and clear entry.pprev and flags
underneath the wheel bucket?

And in the meantime ctcm_free_netdevice() does:

		if (priv->fsm) {
			kfree_fsm(priv->fsm);
			priv->fsm = NULL;
		}

so the still-armed timer's this->fi points at freed memory, which
fsm_expire_timer() then dereferences via fsm_event(). Should the teardown
side also be fixed here, either with an fsm_deltimer(&priv->restart_timer)
in the stop path or a timer_delete_sync()/timer_shutdown_sync() before
kfree_fsm(priv->fsm)?

>  	this->expire_event = event;
>  	this->event_arg = arg;
> -	this->tl.expires = jiffies + (millisec * HZ) / 1000;
> -	add_timer(&this->tl);
> -	return 0;
> +	return mod_timer(&this->tl, jiffies + msecs_to_jiffies(millisec));

[Severity: Low]
This now propagates the mod_timer() result, but the kernel-doc for the
exported fsm_addtimer() in fsm.h still says:

 * Return: Always returns 0
 */
int fsm_addtimer(fsm_timer *timer, int millisec, int event, void *arg);

mod_timer() returns 1 when the timer was already pending and got requeued,
which is exactly the case this patch targets (ctcmpc_send_sweep_req(),
mpc_action_side_xid()). None of the current call sites in
drivers/s390/net/ look at the return value, so there is no behaviour change
today, but elsewhere in this file non-zero conventionally means an error
(see fsm_event()).

Would it be better to keep mod_timer(...) followed by return 0, or update
the fsm.h kernel-doc to describe the 0/1 "was pending" semantics?

>  }
>  
>  /* FIXME: this function is never used, why */
> @@ -189,12 +186,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));

[Severity: Medium]
This isn't a bug introduced by this patch, but with the timer_delete()
gone, the two payload stores are now published with plain stores while the
timer may be pending or its callback already running:

	this->expire_event = event;
	this->event_arg = arg;
	mod_timer(&this->tl, jiffies + msecs_to_jiffies(millisec));

fsm_expire_timer() reads both fields from timer softirq context on any CPU:

	fsm_event(this->fi, this->expire_event, this->event_arg);

with no lock or barrier shared with the writer. If a writer such as
ctcmpc_send_sweep_req() or mpc_action_side_xid() re-arms a pending timer
while it expires on another CPU, can the callback deliver the new
expire_event with the old event_arg, at the old expiry rather than the newly
requested one? And since mod_timer() does not wait for an in-flight
callback, can the FSM see the event twice? The same publish pattern applies
to fsm_addtimer() above.

[Severity: High]
This is a pre-existing issue, but it interacts with the switch to
mod_timer(). fsm_deltimer() only does:

	timer_delete(&this->tl);

which does not wait for a callback running on another CPU, and
ctcm_free_netdevice() performs no cancellation at all for grp->timer or
priv->restart_timer before freeing their containers:

			if (grp->fsm)
				kfree_fsm(grp->fsm);
			dev_kfree_skb(grp->xid_skb);
			dev_kfree_skb(grp->rcvd_xid_skb);
			tasklet_kill(&grp->mpc_tasklet2);
			kfree(grp);

grp->timer is armed for MPC_XID_TIMEOUT_VALUE from mpc_channel_action()
and the XID paths in ctcm_fsms.c:

		fsm_addtimer(&grp->timer, MPC_XID_TIMEOUT_VALUE,
						MPCG_EVENT_TIMER, dev);

so a set_offline during an XID exchange leaves a struct timer_list linked
in the wheel inside memory freed by kfree(grp), and fsm_expire_timer() then
reads this->fi (the fsm_instance freed by kfree_fsm(grp->fsm)) and
indirect-calls through fi->f->jumpmatrix in fsm_event(). Can that be hit
today?

The sibling instance of this in channel_remove() is addressed later in this
series by "s390/ctcm: Fix use-after-free in channel_remove()", which
replaces fsm_deltimer(&ch->timer) / fsm_deltimer(&ch->sweep_timer) with
timer_delete_sync(&ch->timer.tl) / timer_delete_sync(&ch->sweep_timer.tl).
At the end of the series ctcm_free_netdevice() has no equivalent change, so
the grp->timer and priv->restart_timer cases look untreated.

Also, now that fsm_addtimer() uses mod_timer(), a concurrent FSM action can
silently re-arm a timer after a plain fsm_deltimer(). Should the teardown
paths use timer_delete_sync() or timer_shutdown_sync() semantics rather
than timer_delete()?

Two further observations in the same ctcm_free_netdevice() teardown path
that this series touches, neither of them introduced here:

[Severity: Critical]
This isn't a bug introduced by this patch, but ctcm_free_netdevice() frees
priv->xid with kfree() even though it is an interior pointer into an SKB
data buffer that was released a few statements earlier:

			dev_kfree_skb(grp->xid_skb);
...
		kfree(priv->xid);
		priv->xid = NULL;

ctcmpc_init_mpc_group() sets it up as:

	grp->xid = (struct xid2 *)skb_tail_pointer(grp->xid_skb);
...
	priv->xid = grp->xid;

and that is the only assignment to priv->xid in the driver. Since priv is
kzalloc'ed, non-MPC devices hit kfree(NULL), but for an MPC group does this
kfree() hand the allocator an address TH_HEADER_LENGTH bytes into an
already-freed buffer on every offline or teardown?

[Severity: Medium]
This is a pre-existing issue as well, but does ctcm_init_netdevice() leak
priv->fsm when the MPC group setup fails?

		grp = ctcmpc_init_mpc_group(priv);
		if (grp == NULL) {
			MPC_DBF_DEV(SETUP, dev, "init_mpc_group error");
			free_netdev(dev);
			return NULL;
		}

priv->fsm was allocated by init_fsm("ctcmdev", ...) just above, and
ctcmpc_init_mpc_group() returns NULL on its GFP_ATOMIC|GFP_DMA xid_skb /
rcvd_xid_skb allocation failures. free_netdev() does not release the
fsm_instance, and the caller ctcm_new_device() goes to out_ccw2 without
calling ctcm_free_netdevice(), so the instance and its jumpmatrix stay
allocated until kfree(priv) in ctcm_remove_device() drops the last
reference to them, or a retried set_online overwrites priv->fsm.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907144511.3810194-1-nagamani%40linux.ibm.com

[PATCH net 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(). timer_delete_sync() is used rather than
timer_shutdown_sync() because the channel is fully torn down by
channel_remove() and the timer is never re-armed after this point,
making the re-arm prevention of timer_shutdown_sync() unnecessary.
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().

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>
---
 drivers/s390/net/ctcm_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/net/ctcm_main.c b/drivers/s390/net/ctcm_main.c
index 8b0d76a47d9f..63f8832f9b14 100644
--- a/drivers/s390/net/ctcm_main.c
+++ b/drivers/s390/net/ctcm_main.c
@@ -211,9 +211,9 @@ static void channel_remove(struct channel *ch)
 	while (*c) {
 		if (*c == ch) {
 			*c = ch->next;
-			fsm_deltimer(&ch->timer);
+			timer_delete_sync(&ch->timer.tl);
 			if (IS_MPC(ch))
-				fsm_deltimer(&ch->sweep_timer);
+				timer_delete_sync(&ch->sweep_timer.tl);
 
 			kfree_fsm(ch->fsm);
 			clear_normalized_cda(&ch->ccw[4]);
-- 
2.53.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The patch fixes a use-after-free for `ch->timer` by switching to `timer_delete_sync()`, but fails to apply the same fix for `priv->restart_timer` and `grp->timer`, leaving identical use-after-free vulnerabilities in the teardown paths.
- [High] Lockless traversal and modification of the global `channels` list can lead to list corruption and crashes.
--

commit 0701d8bf2eb4b9bc69bd86286f3b23ff8226be83
Author: Nagamani PV <nagamani@linux.ibm.com>

s390/ctcm: Fix use-after-free in channel_remove()

The commit fixes a use-after-free vulnerability by transitioning from
fsm_deltimer() to timer_delete_sync() in channel_remove(). This prevents
timer callbacks from executing after the channel structure has been freed.

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

> diff --git a/drivers/s390/net/ctcm_main.c b/drivers/s390/net/ctcm_main.c
> index 8b0d76a47d9f0..63f8832f9b149 100644
> --- a/drivers/s390/net/ctcm_main.c
> +++ b/drivers/s390/net/ctcm_main.c
> @@ -211,9 +211,9 @@ static void channel_remove(struct channel *ch)
>  	while (*c) {
>  		if (*c == ch) {
>  			*c = ch->next;

[Severity: High]
This is a pre-existing issue, but does modifying the global channels list
locklessly here race with other list modifications?

For example, in drivers/s390/net/ctcm_main.c:add_channel():

	ch->next = *c;
	*c = ch;

Since device teardown and probe can be triggered concurrently by sysfs
writes to the online attribute, could this lead to list corruption or
crashes?

> -			fsm_deltimer(&ch->timer);
> +			timer_delete_sync(&ch->timer.tl);

[Severity: High]
This isn't a bug introduced by this patch, but should a similar fix be
applied to grp->timer and priv->restart_timer?

In drivers/s390/net/ctcm_main.c:ctcm_free_netdevice(), grp->timer is not
synchronously canceled before the FSM and mpc_group are freed:

	if (grp->fsm)
		kfree_fsm(grp->fsm);
	...
	kfree(grp);

Similarly, in drivers/s390/net/ctcm_main.c:ctcm_remove_device(),
priv->restart_timer is not synchronously canceled before priv is freed:

	kfree(priv);

Since unprivileged userspace can trigger device teardown via the sysfs
online attribute, could a concurrent timer callback access the freed FSM
and driver state, leading to a use-after-free?

>  			if (IS_MPC(ch))
> -				fsm_deltimer(&ch->sweep_timer);
> +				timer_delete_sync(&ch->sweep_timer.tl);
>  
>  			kfree_fsm(ch->fsm);
>  			clear_normalized_cda(&ch->ccw[4]);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907144511.3810194-1-nagamani@linux.ibm.com?part=2

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 1 (3) · Medium: 0 · Low: 1

- [High] channel_remove() (drivers/s390/net/ctcm_main.c:214/216) uses
  timer_delete_sync() instead of timer_shutdown_sync().
- [Low] The patch fixes a real use-after-free reachable during device
  teardown but carries no `Fixes:` tag, so stable maintainers cannot…

Pre-existing issues:
- [High] The same fsm_timer/kfree_fsm() UAF pattern that this patch fixes
  for ch->timer/ch->sweep_timer remains completely unfixed for the…
- [High] The synchronous cancel was added in channel_remove(), but
  ctcm_shutdown_device() frees the objects that timer-driven FSM actions…
- [High] The MPC channel tasklets are killed only after the resources
  they dereference have been freed.

--

s390/ctcm: Fix use-after-free in channel_remove()

This replaces the two fsm_deltimer() calls in channel_remove() with
timer_delete_sync() on the embedded timer_list, so the channel timer
callback cannot still be running when kfree_fsm(ch->fsm) and kfree(ch)
free the memory it uses.

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

[Severity: Low]
There is no Fixes: tag on this patch. git blame on the two replaced
fsm_deltimer() calls and the following kfree_fsm(ch->fsm) points at:

  293d984f0e3604 ("ctcm: infrastructure for replaced ctc driver")

Could that be added as a Fixes: tag so stable can work out the affected
range?

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

[Severity: High]
The commit message states:

  timer_delete_sync() is used rather than timer_shutdown_sync() because
  the channel is fully torn down by channel_remove() and the timer is
  never re-armed after this point, making the re-arm prevention of
  timer_shutdown_sync() unnecessary.

Does that hold for the MPC case?  timer_delete_sync() leaves
timer->function intact, so a later fsm_addtimer() simply re-queues the
timer:

drivers/s390/net/fsm.c:fsm_addtimer() {
	...
	return mod_timer(&this->tl, jiffies + msecs_to_jiffies(millisec));
}

Here ch->timer is cancelled before ch->sweep_timer, and the sweep timer
action re-arms both of them:

drivers/s390/net/ctcm_fsms.c:ctcmpc_chx_send_sweep() {
	...
		fsm_addtimer(&wch->sweep_timer,
			200, CTC_EVENT_RSWEEP_TIMER, wch);
	...
	fsm_addtimer(&wch->timer, CTCM_TIME_5_SEC, CTC_EVENT_TIMER, wch);
}

For ch == priv->channel[CTCM_WRITE], can a sweep callback that has already
been dispatched on another CPU execute the fsm_addtimer(&wch->timer, ...)
above after timer_delete_sync(&ch->timer.tl) has returned?  The following
timer_delete_sync(&ch->sweep_timer.tl) waits for the sweep callback, but
nothing then re-checks ch->timer.

The MPC tasklets look like a second source of re-arms, since they are only
killed further down in channel_remove(), after both cancels:

  ctcmpc_bh() -> ctcmpc_unpack_skb() -> mpc_rcvd_sweep_req() ->
  ctcmpc_send_sweep_resp()

drivers/s390/net/ctcm_mpc.c:ctcmpc_send_sweep_resp() {
	...
	fsm_addtimer(&ch->sweep_timer, 100, CTC_EVENT_RSWEEP_TIMER, ch);
}

drivers/s390/net/ctcm_mpc.c:mpc_action_side_xid() {
	...
	fsm_addtimer(&ch->timer, 5000 , CTC_EVENT_TIMER, ch);
}

tasklet_kill() only waits for a running tasklet to finish, so a re-arm done
by that tasklet survives it.

If either re-arm wins, kfree(ch) below returns a timer_list that is still
linked into a timer base to the slab allocator, and on expiry
fsm_expire_timer() dereferences this->fi (the fsm_instance freed by
kfree_fsm()) and calls through fi->f->jumpmatrix.

Would timer_shutdown_sync(), which makes the later mod_timer() a no-op,
and/or moving the tasklet_kill() calls ahead of the timer teardown, close
this window?

>  
>  			kfree_fsm(ch->fsm);

[Severity: High]
This is a pre-existing ordering problem and not something the patch
introduces, but kfree_fsm(ch->fsm) here runs before the MPC tasklets are
stopped later in the same function:

			if (IS_MPC(ch)) {
				tasklet_kill(&ch->ch_tasklet);
				tasklet_kill(&ch->ch_disc_tasklet);

ctcmpc_bh(), which is ch->ch_tasklet, starts with:

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) &&
	...
}

By the time channel_remove() is reached from ctcm_shutdown_device(),
ctcm_free_netdevice() has already done:

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

Can a tasklet that was pending before teardown therefore read a freed
net_device, a NULL priv->mpcg, and a freed grp->fsm, and then call through
its jumpmatrix?  Should the tasklet_kill() calls happen before the
netdevice, group and FSM frees rather than after them?

[Severity: High]
This is a pre-existing gap rather than a regression from this patch, but
the driver has two more fsm_timer instances with the same kfree_fsm()
pattern, and neither is cancelled at all in the teardown path.

grp->timer is bound to grp->fsm by fsm_settimer(grp->fsm, &grp->timer) in
ctcmpc_init_mpc_group(), and it is armed during XID negotiation:

drivers/s390/net/ctcm_mpc.c:mpc_channel_action() {
	...
			fsm_addtimer(&grp->timer, MPC_XID_TIMEOUT_VALUE,
						MPCG_EVENT_TIMER, dev);
}

ctcm_free_netdevice() then frees grp->fsm and grp itself, and the
timer_list lives inside struct mpc_group, so a pending grp->timer is handed
back to the slab allocator with no timer_delete()/timer_delete_sync()
anywhere in that path.

priv->restart_timer has the same shape: fsm_settimer(priv->fsm,
&priv->restart_timer) in ctcm_init_netdevice(), armed by
dev_action_restart() in ctcm_fsms.c, and priv->fsm is freed in
ctcm_free_netdevice() without a synchronous cancel.

For MPC devices ctcm_close() is a no-op:

drivers/s390/net/ctcm_main.c:ctcm_close() {
	...
	if (!IS_MPC(priv))
		fsm_event(priv->fsm, DEV_EVENT_STOP, dev);
}

so neither ctcm_close() nor unregister_netdev() quiesces these two timers
first.  Since fsm_deltimer() stays non-synchronous and no synchronous
counterpart is being added, would it make sense to add one (for example an
fsm_deltimer_sync() helper) and use it at these sites too?

[Severity: High]
Also pre-existing, but related to where the new synchronous cancel sits in
the overall shutdown ordering.  In ctcm_shutdown_device() the objects the
channel timer callbacks dereference are freed before channel_remove() runs:

drivers/s390/net/ctcm_main.c:ctcm_shutdown_device() {
	...
		ctcm_close(dev);
	...
	if (dev) {
		unregister_netdev(dev);
		ctcm_free_netdevice(dev);
	}

	if (priv->fsm)
		kfree_fsm(priv->fsm);

	ccw_device_set_offline(cgdev->cdev[1]);
	ccw_device_set_offline(cgdev->cdev[0]);
	channel_remove(priv->channel[CTCM_READ]);
	channel_remove(priv->channel[CTCM_WRITE]);
}

A channel timer can still be armed at that point: ctcm_close() is a no-op
for MPC, and for non-MPC the stop handler ctcm_chx_haltio() itself arms
ch->timer for CTCM_TIME_5_SEC without waiting for the halt.

If that timer expires in the window before channel_remove() is entered,
fsm_expire_timer() -> fsm_event(ch->fsm, CTC_EVENT_TIMER, ch) can dispatch:

drivers/s390/net/ctcm_fsms.c:ctcm_chx_txretry() {
	struct net_device *dev = ch->netdev;
	struct ctcm_priv *priv = dev->ml_priv;
	...
		fsm_event(priv->fsm, DEV_EVENT_TXDOWN, dev);
}

dev has already been freed by free_netdev() in MODULE builds, and
priv->fsm was set to NULL by ctcm_free_netdevice().  Should the
synchronous cancel happen before unregister_netdev()/ctcm_free_netdevice()
/kfree_fsm(priv->fsm), or should those frees move after channel_remove()?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907144511.3810194-1-nagamani%40linux.ibm.com