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
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 <redacted>
---
drivers/s390/net/fsm.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
@@ -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);-return0;+returnmod_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);
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 <redacted>
---
drivers/s390/net/ctcm_main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
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?
@@ -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)?
[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?
[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
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?
[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
On 10/09/26 8:16 AM, netdev-bot+sashiko@kernel.org wrote:
- [High] Incomplete fix: the commit removes timer_setup() from
fsm_addtimer()/fsm_modtimer() on the premise that "[t]he timer is…
This refers to priv->restart_timer — addressed inline below.
[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():
Correct. Will fix the function name in v2.
[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?
Pre-existing issue not introduced by this patch.
[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
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?
Will fix in v2 by calling mod_timer() separately and returning 0
explicitly, preserving the documented return contract.
[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:
Pre-existing issue not introduced by this patch.
[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:
Pre-existing issue not introduced by this patch.
[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:
Pre-existing issue not introduced by this patch.
[Severity: Medium]
This is a pre-existing issue as well, but does ctcm_init_netdevice() leak
priv->fsm when the MPC group setup fails?
On 10/09/26 8:16 AM, netdev-bot+sashiko@kernel.org wrote:
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.
--
[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?
Will add in v2:
Fixes: 293d984f0e36 ("ctcm: infrastructure for replaced ctc driver")
Cc: stable@vger.kernel.org
> [Severity: High]
Does that hold for the MPC case? timer_delete_sync() leaves
timer->function intact, so a later fsm_addtimer() simply re-queues the
timer:
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?
Pre-existing issue not introduced by this patch.
[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.
Pre-existing issue not introduced by this patch.
[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.
Pre-existing issue not introduced by this patch.
[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:
Pre-existing issue not introduced by this patch.
Nagamani
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.
Changes in v2:
- Patch 1: fix function name ctcm_send_sweep() -> ctcmpc_send_sweep_req()
in the commit message (Sashiko netdev-bot)
- Patch 1: call mod_timer() then return 0 explicitly, preserving the
"Always returns 0" contract documented in fsm.h (Sashiko netdev-bot)
- Patch 2: add Fixes: and Cc: stable@vger.kernel.org tags
(Sashiko netdev-bot)
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 | 9 ++-------
2 files changed, 4 insertions(+), 9 deletions(-)
--
2.53.0
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 <redacted>
---
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(-)
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().
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
Reviewed-by: Aswin Karuvally <aswin@linux.ibm.com>
Tested-by: Aswin Karuvally <aswin@linux.ibm.com>
Signed-off-by: Nagamani PV <redacted>
---
Changes in v2:
- add Fixes: and Cc: stable@vger.kernel.org tags (Sashiko netdev-bot)
---
drivers/s390/net/ctcm_main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Jakub Kicinski <kuba@kernel.org> Date: 2026-09-21 23:15:20
On Wed, 16 Sep 2026 15:21:47 +0200 Nagamani PV wrote:
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.
Changes in v2:
- Patch 1: fix function name ctcm_send_sweep() -> ctcmpc_send_sweep_req()
in the commit message (Sashiko netdev-bot)
- Patch 1: call mod_timer() then return 0 explicitly, preserving the
"Always returns 0" contract documented in fsm.h (Sashiko netdev-bot)
- Patch 2: add Fixes: and Cc: stable@vger.kernel.org tags
(Sashiko netdev-bot)
On Wed, 16 Sep 2026 15:21:47 +0200 Nagamani PV wrote:
quoted
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.
Changes in v2:
- Patch 1: fix function name ctcm_send_sweep() -> ctcmpc_send_sweep_req()
in the commit message (Sashiko netdev-bot)
- Patch 1: call mod_timer() then return 0 explicitly, preserving the
"Always returns 0" contract documented in fsm.h (Sashiko netdev-bot)
- Patch 2: add Fixes: and Cc: stable@vger.kernel.org tags
(Sashiko netdev-bot)
Thank you for the pointer. I have reviewed all four High findings.
Two share a root cause and are addressed in v3, sent Tue 22 Sep 2026
as "[PATCH net v3 0/2] s390/ctcm: Fix timer corruption and use-after-free"
[off-list ref] (raw). The remaining two
are pre-existing issues outside the scope of this series and will be
fixed in a follow-up patch.
--- Findings addressed in v3 ---
1. [High] ch_tasklet re-arms sweep_timer after timer_shutdown_sync()
returns (Sashiko patch 2/2, finding 2):
ch_tasklet (ctcmpc_bh -> ctcmpc_send_sweep_resp) can re-arm
sweep_timer after timer_shutdown_sync() returns because the v2 patch
killed tasklets after stopping the timers.
2. [High] kfree_fsm(ch->fsm) before tasklet_kill() races with a running
tasklet accessing ch->fsm (Sashiko patch 2/2, finding 5).
v3 fix for both: kill ch_tasklet and ch_disc_tasklet first, then
timer_shutdown_sync(&ch->sweep_timer.tl), then
timer_delete_sync(&ch->timer.tl), before freeing ch->fsm.
kfree(ch->discontact_th) is also moved into the MPC teardown block
after tasklet_kill(ch_disc_tasklet).
--- Pre-existing findings tracked for follow-up ---
3. [High] grp->timer / priv->restart_timer not cancelled before free in
ctcm_free_netdevice() (Sashiko patch 1/2 finding 2, patch 2/2
finding 1): predates this series (commit 293d984f0e36), out of scope
here. Will be fixed in a follow-up patch.
4. [High] NULL deref on peekskb->len in ctcmpc_chx_txdone()
ctcm_fsms.c:1299 (Sashiko patch 1/2, finding 1): skb_peek() returns
NULL when collect_queue empties. Predates this series. Same
follow-up patch.
--- v3 summary ---
Patch 1 (fsm.c): unchanged from v2.
Patch 2 (ctcm_main.c): tasklet_kill() moved before timer stops;
kfree(discontact_th) moved into the MPC teardown block.
Aswin Karuvally will review and test the new teardown order.
Nagamani