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