Re: [PATCH] HSI: ssi_protocol: don't cancel_work_sync() under ssi->lock in ssip_reset

[khj]

 > But is it okay to run before timers are gone

 Good question. The three timers never queue ssi->work, so cancelling the
 work before deleting them cannot let a timer re-arm it: keep_alive only
 re-arms itself, and rx_wd/tx_wd just call ssip_error(). ssi->work is
 queued only by the xmit path (ssip_pn_xmit(), SEND_READY). So the v1
 ordering is safe in that respect.

 But your question made me look harder, and the real problem is bigger
 than the lock: ssip_reset() is itself called from softirq context on
 several paths -- the rx_wd/tx_wd watchdog timers reach it via
 ssip_error(), and so do the ssip_*_complete() HSI message callbacks.
 cancel_work_sync() has an unconditional might_sleep(), so it is invalid
 in ssip_reset() on all of those, not just under ssi->lock. Moving it out
 of the lock (v1) does not fix that.

 The cancel guards teardown/free, so its right home is the process-context
 stop path, not ssip_reset(). I'll send a v2 that drops cancel_work_sync()
 from ssip_reset() and does it in ssip_pn_stop() instead, which is still on
 the remove path (unregister_netdev() -> ndo_stop), so the UAF fix is
 preserved. The watchdog/error reset paths do not free anything and
 ssip_xmit_work() bails once main_state != ACTIVE, so they do not need a
 synchronous cancel.

 Thanks,
 Hyunjung

2026년 11월 11일 (수) 오후 11:46, Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx>님이 작성:
>
> On Wed, Nov 11, 2026 at 06:04:47PM +0900, Hyunjung Ko wrote:
> > ssip_reset() calls cancel_work_sync(&ssi->work) inside the
> > spin_lock_bh(&ssi->lock) section. cancel_work_sync() calls might_sleep(),
> > so running it under the bh spinlock triggers "BUG: sleeping function called
> > from invalid context". In addition, the cancelled work ssip_xmit_work() ->
> > ssip_xmit() takes ssi->lock itself, so waiting for it while holding the
> > lock can deadlock.
> >
> > Move cancel_work_sync() out of the locked section. It still runs in
> > ssip_reset() before teardown, so the use-after-free fix is preserved.
>
> But is it okay to run before timers are gone?
>
> ...
>
> >       spin_unlock_bh(&ssi->lock);
> >       if (test_and_clear_bit(SSIP_WAKETEST_FLAG, &ssi->flags))
> >               ssi_waketest(cl, 0); /* FIXME: To be removed */
> > +     cancel_work_sync(&ssi->work);
> >       spin_lock_bh(&ssi->lock);
> >       timer_delete(&ssi->rx_wd);
> >       timer_delete(&ssi->tx_wd);
> >       timer_delete(&ssi->keep_alive);
> > -     cancel_work_sync(&ssi->work);
>
> --
> With Best Regards,
> Andy Shevchenko
>
>