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

[Nagamani PV <nagamani@linux.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?
> 

Pre-existing issue not introduced by this patch.