Re: [PATCH net] nfc: llcp: use timer_shutdown_sync to prevent timer rearm

[Simon Horman <horms@kernel.org>]

On Tue, Oct 27, 2026 at 07:56:11AM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 0 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [High] The fix is incomplete.
> 
> --
> 
> nfc: llcp: use timer_shutdown_sync to prevent timer rearm
> 
> This commit switches link_timer and sdreq_timer in local_cleanup() from
> timer_delete_sync() to timer_shutdown_sync(). The goal is to stop the
> associated work items from re-arming the timers while cancel_work_sync()
> runs, so that no deferred work runs after kfree(local).
> 
> > diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
> > index 74bf817007cf2..a098d72a15ba8 100644
> > --- a/net/nfc/llcp_core.c
> > +++ b/net/nfc/llcp_core.c
> > @@ -173,14 +173,14 @@ static struct nfc_llcp_local *nfc_llcp_local_get(struct nfc_llcp_local *local)
> >  static void local_cleanup(struct nfc_llcp_local *local)
> >  {
> >  	nfc_llcp_socket_release(local, false, ENXIO);
> > -	timer_delete_sync(&local->link_timer);
> > +	timer_shutdown_sync(&local->link_timer);
> >  	skb_queue_purge(&local->tx_queue);
> >  	cancel_work_sync(&local->tx_work);
> >  	cancel_work_sync(&local->rx_work);
> 
> [Severity: High]
> Does this fully close the use-after-free described in the commit message?
> The timers can no longer be re-armed. But tx_work, rx_work, timeout_work
> and sdreq_timeout_work still use cancel_work_sync(), and that does not
> stop them from being queued again afterwards.
> 
> tx_work is cancelled before rx_work, and nfc_llcp_rx_work() always queues
> tx_work:
> 
> net/nfc/llcp_core.c:nfc_llcp_rx_work() {
>     ...
> 	nfc_llcp_rx_skb(local, skb);
> 
> 	schedule_work(&local->tx_work);
>     ...
> }
> 
> Suppose rx_work is running when local_cleanup() reaches
> cancel_work_sync(&local->rx_work). That cancel waits for it, and in the
> meantime rx_work queues tx_work again. local_cleanup() then returns and
> local_release_work() does:
> 
> 	local_cleanup(local);
> 	kfree(local);
> 
> Can nfc_llcp_tx_work() then run on the freed local? It would touch
> local->tx_queue, call nfc_data_exchange(local->dev, ...) and call
> mod_timer(&local->link_timer). The pending tx_work work_struct inside the
> freed object would also still be linked on system_wq.
> 
> Here is one way rx_work can be running during the final cleanup:
> 
> nfc_llcp_data_received()
>     nfc_llcp_find_local()             /* takes a reference */
>                                       nfc_llcp_unregister_device()
>                                           nfc_llcp_remove_local()
>                                           local_cleanup()
>                                           nfc_llcp_local_put()
>     __nfc_llcp_recv()
>         schedule_work(&local->rx_work)    /* system_wq */
>     nfc_llcp_local_put()  /* last ref, queues release_work on llcp_wq */
> 
> After this, rx_work and release_work run at the same time on different
> workqueues.
> 
> Digital drivers can reach this. nfc_digital_unregister_device() calls
> nfc_unregister_device() before cancel_work_sync(&ddev->cmd_complete_work).
> That lets
> digital_tg_recv_dep_req()->nfc_tm_data_received()->nfc_llcp_data_received()
> overlap with LLCP teardown. nfc_tm_data_received() checks dep_link_up but
> not shutting_down.
> 
> There is a related path. nfc_llcp_tx_work() passes the raw local pointer
> to nfc_data_exchange() as the context for nfc_llcp_recv(), and it takes no
> reference:
> 
> 	ret = nfc_data_exchange(local->dev, local->target_idx,
> 				skb, nfc_llcp_recv, local);
> 
> For digital devices, this completion runs from cmd_complete_work, which is
> only cancelled after nfc_unregister_device(). A late completion goes
> through nfc_llcp_recv()->__nfc_llcp_recv(), which writes local->rx_pending
> and calls schedule_work(&local->rx_work). The local it touches may already
> have been through local_cleanup(), or may already be freed.
> 
> Would disable_work_sync() work for the LLCP work items, or at least
> disabling rx_work before tx_work? That would match what
> timer_shutdown_sync() does for the timers.

This does seem to be a legitimate concern.

-- 
pw-bot: changes-requested