[PATCH] nfc: nci: drain rx_wq before cmd_wq on unregister

[PATCH] nfc: nci: drain rx_wq before cmd_wq on unregister

[Yogesh Gaur]

nci_unregister_device() destroys cmd_wq first and rx_wq second. Any
rx_work still running at that point can handle a response and queue
cmd_work from nci_rsp_packet(), which is refused because cmd_wq is
already draining:

  workqueue: cannot queue nci_cmd_work on wq nfc2_nci_cmd_wq
  WARNING: kernel/workqueue.c:2352 at __queue_work+0xdb7/0x1370 kernel/workqueue.c:2351, CPU#3: kworker/u32:0/12
  Workqueue: nfc2_nci_rx_wq nci_rx_work
  Call Trace:
   queue_work_on+0x180/0x1e0 kernel/workqueue.c:2501
   queue_work include/linux/workqueue.h:700 [inline]
   nci_rsp_packet+0x297/0x3420 net/nfc/nci/rsp.c:463
   nci_rx_work+0x29c/0x430 net/nfc/nci/core.c:1579
   process_one_work+0xac7/0x1b10 kernel/workqueue.c:3396

nci_close_device() does not stop rx_work from running again
afterwards: when the device was never brought up it does not flush
rx_wq at all, and the driver can still deliver frames through
nci_recv_frame() until it has stopped calling it.

Destroy the queues in dependency order instead. rx_work queues
cmd_work and tx_work, so rx_wq goes first. The cmd and data timers
queue cmd_work and rx_work, and cmd_work and tx_work re-arm them, so
shut both timers down before any of the queues go away; after
timer_shutdown_sync() the re-arming is a no-op.

Fixes: 6a2968aaf50c ("NFC: basic NCI protocol implementation")
Reported-by: syzbot+ebbbf06f152da8ea4716@syzkaller.appspotmail.com
Assisted-by: LLM
Signed-off-by: Yogesh Gaur <yogeshgaur.83@gmail.com>
---
Built with W=1 only. syzbot has no reproducer for this report, so the
fix has not been runtime-tested

 net/nfc/nci/core.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
index 73e3a96470ac..aa417f863f96 100644
--- a/net/nfc/nci/core.c
+++ b/net/nfc/nci/core.c
@@ -1329,8 +1329,13 @@ void nci_unregister_device(struct nci_dev *ndev)
 
 	nci_close_device(ndev);
 
-	destroy_workqueue(ndev->cmd_wq);
+	/* cmd_work and tx_work re-arm these, and they queue cmd/rx work */
+	timer_shutdown_sync(&ndev->cmd_timer);
+	timer_shutdown_sync(&ndev->data_timer);
+
+	/* rx_work queues cmd_work and tx_work, so drain rx_wq first */
 	destroy_workqueue(ndev->rx_wq);
+	destroy_workqueue(ndev->cmd_wq);
 	destroy_workqueue(ndev->tx_wq);
 
 	list_for_each_entry_safe(conn_info, n, &ndev->conn_info_list, list) {
-- 
2.55.0.windows.5

Re: [PATCH] nfc: nci: drain rx_wq before cmd_wq on unregister

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 (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 new teardown order in nci_unregister_device() does not cover
  a deferred producer of tx_work: the NCI-HCI msg_rx_work, which runs on…
- [High] nci_recv_frame() (net/nfc/nci/core.c:1360-1368) tests
  NCI_UP/NCI_INIT without a lock, then calls skb_queue_tail(&ndev->rx_q)…

Pre-existing issues:
- [High] This problem was not introduced by this patch, but
  nxp_nci_i2c_remove() calls nxp_nci_remove() before…

--

nfc: nci: drain rx_wq before cmd_wq on unregister

This patch changes the teardown order in nci_unregister_device(). It
first shuts down cmd_timer and data_timer with timer_shutdown_sync().
It then destroys rx_wq before cmd_wq and tx_wq, so rx_work can no
longer queue cmd_work on a cmd_wq that is already draining.

> diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
> index 73e3a96470ace..aa417f863f967 100644
> --- a/net/nfc/nci/core.c
> +++ b/net/nfc/nci/core.c
> @@ -1329,8 +1329,13 @@ void nci_unregister_device(struct nci_dev *ndev)
>  
>  	nci_close_device(ndev);
>  
> -	destroy_workqueue(ndev->cmd_wq);
> +	/* cmd_work and tx_work re-arm these, and they queue cmd/rx work */
> +	timer_shutdown_sync(&ndev->cmd_timer);
> +	timer_shutdown_sync(&ndev->data_timer);
> +
> +	/* rx_work queues cmd_work and tx_work, so drain rx_wq first */
>  	destroy_workqueue(ndev->rx_wq);

[Severity: High]
The commit message says "the driver can still deliver frames through
nci_recv_frame() until it has stopped calling it". Does anything order
that producer against destroy_workqueue(ndev->rx_wq) here?

nci_recv_frame() tests the flags without a lock, then queues rx_work:

net/nfc/nci/core.c:nci_recv_frame() {
	if (!ndev || (!test_bit(NCI_UP, &ndev->flags) &&
	    !test_bit(NCI_INIT, &ndev->flags))) {
		kfree_skb(skb);
		return -ENXIO;
	}

	/* Queue frame for rx worker thread */
	skb_queue_tail(&ndev->rx_q, skb);
	queue_work(ndev->rx_wq, &ndev->rx_work);
	...
}

Suppose a driver thread passes the flag test before nci_close_device()
clears the flags with ndev->flags &= BIT(NCI_UNREG), and is then delayed
before queue_work(). Two outcomes look possible:

- It calls queue_work() while destroy_workqueue(rx_wq) is draining. That
  gives the same "cannot queue" warning.
- It calls queue_work() after destroy_workqueue(rx_wq) has returned.
  That uses the freed workqueue_struct.

This looks reachable with nfcmrvl i2c. The remove path:

nfcmrvl_i2c_remove()
  nfcmrvl_nci_unregister_dev()
    nci_unregister_device()

runs while the devm-managed threaded IRQ is still registered. The IRQ
thread, nfcmrvl_i2c_int_irq_thread_fn()->nfcmrvl_nci_recv_frame()->
nci_recv_frame(), shares no lock with the remove path.

nxp-nci does not seem to be affected by this race.
nxp_nci_remove() holds info->info_lock across nci_unregister_device(),
and the IRQ thread takes the same lock.

Would this need a core-side gate around the flag test and queue_work()?
Or should drivers be required to stop RX before they call
nci_unregister_device()?

> +	destroy_workqueue(ndev->cmd_wq);
>  	destroy_workqueue(ndev->tx_wq);
>  
>  	list_for_each_entry_safe(conn_info, n, &ndev->conn_info_list, list) {

[Severity: High]
Does this ordering cover the NCI-HCI msg_rx_work? It runs on system_wq
and can also queue tx_work.

When rx_work handles a data packet on the HCI connection, the callback
installed by nci_hci_dev_session_init() defers non-response HCP
messages:

nci_rx_work()
  nci_rx_data_packet()
    nci_add_rx_data_frag()
      nci_data_exchange_complete()
        nci_hci_data_received_cb()
          schedule_work(&ndev->hci_dev->msg_rx_work);

msg_rx_work then always sends a reply:

nci_hci_msg_rx_work()
  nci_hci_hcp_message_rx()
    nci_hci_cmd_received()
      nci_hci_send_data()
        nci_send_data()
          queue_work(ndev->tx_wq, &ndev->tx_work);

Neither nci_close_device() nor nci_unregister_device() flushes or
cancels msg_rx_work. conn_info_list is only emptied after the
destroy_workqueue() calls, so nci_send_data() still finds the HCI
connection.

The window is narrower than it first looks.
nci_data_exchange_complete() only runs the callback if
test_and_clear_bit(NCI_DATA_EXCHANGE) succeeds. nci_close_device()
clears that bit, and nfc_unregister_rfkill() sets shutting_down, which
stops the bit from being set again. So the work can't be newly
scheduled during the rx_wq drain.

But msg_rx_work might have been scheduled before nci_close_device() and
still be pending or running here. Could it then queue tx_work while
tx_wq is draining, or after destroy_workqueue(ndev->tx_wq) has freed it?

Later, nci_free_device()->nci_hci_deallocate() calls
kfree(ndev->hci_dev). That happens while msg_rx_work and msg_rx_queue
may still be in use.

Should hci_dev->msg_rx_work be cancelled or flushed after rx_wq is
drained and before cmd_wq and tx_wq are destroyed?

[Severity: High]
This isn't a bug introduced by this patch. While looking at callers of
nci_unregister_device(), I noticed that the nxp-nci i2c remove path
frees the nci_dev before it frees the IRQ:

drivers/nfc/nxp-nci/i2c.c:nxp_nci_i2c_remove() {
	...
	nxp_nci_remove(phy->ndev);
	free_irq(client->irq, phy);
}

nxp_nci_remove() calls nci_unregister_device() and nci_free_device(),
and phy->ndev is never cleared. If the IRQ fires before free_irq(),
nxp_nci_i2c_irq_thread_fn() does:

	info = nci_get_drvdata(phy->ndev);
	...
	mutex_lock(&info->info_lock);

Is this a use-after-free read of the freed nci_dev? The read happens
before the info->mode check. So switching the controller to
NXP_NCI_MODE_COLD beforehand doesn't prevent it.

Would freeing or disabling the IRQ before nxp_nci_remove() fix this?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261005102849.486-1-yogeshgaur.83%40gmail.com