Re: [PATCH net-next] s390/ctcm: Convert fsm.h to proper kernel-doc format

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

Pre-existing issues:
- [High] `fsm_deltimer()` uses non-sync `timer_delete()`, which can lead to a use-after-free when timers are freed during teardown.
- [High] `fsm_addtimer()` unconditionally calls `timer_setup()`, which corrupts the timer wheel if the timer is already active.
--

--- Patch [1]: [PATCH net-next] s390/ctcm: Convert fsm.h to proper kernel-doc format ---
commit 2780dcd525cc21b0863567563af1ccbda9bf1645
Author: Nagamani PV <[email protected]>

s390/ctcm: Convert fsm.h to proper kernel-doc format

This commit converts function declarations and comments in drivers/s390/net/fsm.h
to proper kernel-doc format. It changes debug macros and internal comments to
use standard block comments and removes redundant extern keywords.

> diff --git a/drivers/s390/net/fsm.h b/drivers/s390/net/fsm.h
> index 16dc071a2973c..6a0b47ca87f0c 100644
> --- a/drivers/s390/net/fsm.h
> +++ b/drivers/s390/net/fsm.h
[ ... ]
>  /**
> - * Clears a pending timer of an FSM instance.
> + * fsm_deltimer - Clears a pending timer of an FSM instance
> + * @timer: The timer to clear
>   *
> - * @param timer The timer to clear.
> + * Stops and removes the timer. Safe to call on an inactive timer.
>   */
> -extern void fsm_deltimer(fsm_timer *timer);
> +void fsm_deltimer(fsm_timer *timer);

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for this
non-synchronous timer deletion to cause a use-after-free during device
teardown?

Looking at fsm_deltimer() in drivers/s390/net/fsm.c, it uses timer_delete().
During teardown in ctcm_free_channel(), the driver calls:

ctcm_free_channel() {
        fsm_deltimer(&ch->timer);
        ...
        kfree_fsm(ch->fsm);
        kfree(ch);
}

Since timer_delete() does not wait for a concurrently executing timer
callback, could fsm_expire_timer() still be running on another CPU and
dereference ch->fsm after it has been freed by kfree_fsm()? Would it be
safer to use timer_shutdown_sync() or timer_delete_sync() here?

>  /**
> - * Adds and starts a timer to an FSM instance.
> + * fsm_addtimer - Adds and starts a timer for an FSM instance
> + * @timer: The timer to be added (timer->fi must point to the FSM instance)
> + * @millisec: Duration in milliseconds after which the timer expires
> + * @event: Event to trigger when timer expires
> + * @arg: Generic argument provided to the event handler
>   *
> - * @param timer    The timer to be added. The field fi of that timer
> - *                 must have been set to point to the instance.
> - * @param millisec Duration, after which the timer should expire.
> - * @param event    Event, to trigger if timer expires.
> - * @param arg      Generic argument, provided to expiry function.
> + * Starts a timer that will trigger the specified event after the given
> + * duration. The timer must have been initialized with fsm_settimer().
>   *
> - * @return         0 on success, -1 if timer is already active.
> + * Return: Always returns 0
>   */
> -extern int fsm_addtimer(fsm_timer *timer, int millisec, int event, void *arg);
> +int fsm_addtimer(fsm_timer *timer, int millisec, int event, void *arg);

[Severity: High]
This is a pre-existing issue, but does fsm_addtimer() risk corrupting the
timer wheel if called on an already active timer?

In drivers/s390/net/fsm.c, fsm_addtimer() unconditionally calls
timer_setup():

fsm_addtimer() {
        timer_setup(&this->tl, fsm_expire_timer, 0);
        ...
        add_timer(&this->tl);
}

Since timer_setup() zeros out the list entry fields, calling this on a
currently pending timer will corrupt the timer wheel list. While many
callers delete the timer first, there are paths like mpc_action_side_xid()
in drivers/s390/net/ctcm_mpc.c that invoke fsm_addtimer() without a
preceding fsm_deltimer(). Should this use mod_timer() instead to safely
handle already active timers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1