[PATCH net v5 0/2] nfc: nci: uart: quiesce write_work during teardown

[PATCH net v5 0/2] nfc: nci: uart: quiesce write_work during teardown

[Shubham Antil]

This series fixes the NCI UART write-worker teardown ordering without
blocking the controller reset which nci_unregister_device() may transmit
during teardown.  It also fixes the Marvell UART state-publication race
reproduced while exercising that path.

Patch 1 publishes the Marvell UART back-pointers before NCI device
registration can expose the device, rolls them back on registration
failure, and establishes the controller's initial halt state before
registration.

Patch 2 serializes producers with the write worker's final state handoff.
The generic close invokes the transport close first, allowing the
unregister CORE_RESET transmit attempt, then blocks new work and cancels
the worker before freeing the skbs.  Marvell performs that barrier after
NCI unregister but before it releases private state.  The failed-open path
uses the same barrier, and the transport module is held while its open
callback runs.

The tty write-wakeup description has been corrected: a wakeup may queue
or run immediately before close; it does not run inside the close callback.
The same spinlock now protects producer scheduling and the worker's final
SENDING/WAKEUP transition, closing the reported lost-wakeup window.

Both ordering issues were found during an LLM-assisted source audit and
reproduced using a pty-backed NCI UART under KASAN.  With these patches, a
61-cycle regression run and a 56,690-cycle registration/hangup stress run
completed without sanitizer reports.

Other concurrency concerns raised during review are outside the scope of
this teardown series and will be handled separately.

---

Changes in v5:
- Preserve the unregister-time CORE_RESET transmit attempt by closing the
  transport before starting the generic worker barrier.
- Quiesce the Marvell worker after NCI unregister and before private-state
  release.
- Serialize the worker's final state handoff to prevent a lost wakeup.
- Correct the tty write-wakeup race description and close-contract comment.
- Take the module reference before ops.open() and drain work on open
  failure.
- Publish and roll back Marvell UART state through transport callbacks.
- Add the LLM-assisted audit and KASAN discovery details.
- Keep unrelated concurrency follow-ups outside this series.

v4: https://lore.kernel.org/netdev/20261006115532.72100-1-shubham@octane.security/
v3: https://lore.kernel.org/netdev/20261002055620.18339-1-shubham@octane.security/

Shubham Antil (2):
  nfc: nfcmrvl: publish UART state before registration
  nfc: nci: uart: quiesce write_work during teardown

 drivers/nfc/nfcmrvl/main.c    | 18 +++++--
 drivers/nfc/nfcmrvl/nfcmrvl.h |  3 ++
 drivers/nfc/nfcmrvl/uart.c    | 32 +++++++++++--
 include/net/nfc/nci_core.h    |  3 ++
 net/nfc/nci/uart.c            | 90 ++++++++++++++++++++++++++++-------
 5 files changed, 120 insertions(+), 26 deletions(-)


base-commit: 72d3fcf802c45d00b300f25b848a93c3a2bd7c7e
-- 
2.43.0

[PATCH net v5 1/2] nfc: nfcmrvl: publish UART state before registration

[Shubham Antil]

nfcmrvl_nci_uart_open() does not publish nu->drv_data and nu->ndev until
nfcmrvl_nci_register_dev() returns.  The registration call exposes the NCI
device first, so an early transmit can reach the UART callbacks while those
back-pointers are not initialized.

Add callbacks which publish the UART back-pointers before
nci_register_device() and withdraw them if registration fails.  Halt the
controller before exposing the device as well.

The ordering issue was found during an LLM-assisted source audit and
reproduced using a pty-backed NCI UART under KASAN.

Fixes: e097dc624f78 ("NFC: nfcmrvl: add UART driver")
Cc: stable@vger.kernel.org
Assisted-by: LLM (OpenAI Codex, gpt-5.6-sol)
Signed-off-by: Shubham Antil <shubham@octane.security>
---
 drivers/nfc/nfcmrvl/main.c    | 11 ++++++++---
 drivers/nfc/nfcmrvl/nfcmrvl.h |  2 ++
 drivers/nfc/nfcmrvl/uart.c    | 24 +++++++++++++++++++-----
 3 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
index 6efa83219117..3d26a54ef51b 100644
--- a/drivers/nfc/nfcmrvl/main.c
+++ b/drivers/nfc/nfcmrvl/main.c
@@ -154,20 +154,25 @@ struct nfcmrvl_private *nfcmrvl_nci_register_dev(enum nfcmrvl_phy phy,
 
 	nci_set_drvdata(priv->ndev, priv);
 
+	/* Ensure that controller is powered off before exposing the device. */
+	nfcmrvl_chip_halt(priv);
+
+	if (priv->if_ops->nci_ready)
+		priv->if_ops->nci_ready(priv);
+
 	rc = nci_register_device(priv->ndev);
 	if (rc) {
 		nfc_err(dev, "nci_register_device failed %d\n", rc);
 		goto error_fw_dnld_deinit;
 	}
 
-	/* Ensure that controller is powered off */
-	nfcmrvl_chip_halt(priv);
-
 	nfc_info(dev, "registered with nci successfully\n");
 	return priv;
 
 error_fw_dnld_deinit:
 	nfcmrvl_fw_dnld_deinit(priv);
+	if (priv->if_ops->nci_unready)
+		priv->if_ops->nci_unready(priv);
 error_free_dev:
 	nci_free_device(priv->ndev);
 error_free:
diff --git a/drivers/nfc/nfcmrvl/nfcmrvl.h b/drivers/nfc/nfcmrvl/nfcmrvl.h
index 62fa77d587b2..66f8cc06e2d5 100644
--- a/drivers/nfc/nfcmrvl/nfcmrvl.h
+++ b/drivers/nfc/nfcmrvl/nfcmrvl.h
@@ -112,6 +112,8 @@ struct nfcmrvl_if_ops {
 	int (*nci_open) (struct nfcmrvl_private *priv);
 	int (*nci_close) (struct nfcmrvl_private *priv);
 	int (*nci_send) (struct nfcmrvl_private *priv, struct sk_buff *skb);
+	void (*nci_ready)(struct nfcmrvl_private *priv);
+	void (*nci_unready)(struct nfcmrvl_private *priv);
 	void (*nci_update_config)(struct nfcmrvl_private *priv,
 				  const void *param);
 };
diff --git a/drivers/nfc/nfcmrvl/uart.c b/drivers/nfc/nfcmrvl/uart.c
index 9aedd168759c..d688071f4b26 100644
--- a/drivers/nfc/nfcmrvl/uart.c
+++ b/drivers/nfc/nfcmrvl/uart.c
@@ -44,6 +44,23 @@ static int nfcmrvl_uart_nci_send(struct nfcmrvl_private *priv,
 	return nu->ops.send(nu, skb);
 }
 
+static void nfcmrvl_uart_nci_ready(struct nfcmrvl_private *priv)
+{
+	struct nci_uart *nu = priv->drv_data;
+
+	priv->support_fw_dnld = true;
+	nu->drv_data = priv;
+	nu->ndev = priv->ndev;
+}
+
+static void nfcmrvl_uart_nci_unready(struct nfcmrvl_private *priv)
+{
+	struct nci_uart *nu = priv->drv_data;
+
+	nu->drv_data = NULL;
+	nu->ndev = NULL;
+}
+
 static void nfcmrvl_uart_nci_update_config(struct nfcmrvl_private *priv,
 					   const void *param)
 {
@@ -58,6 +75,8 @@ static const struct nfcmrvl_if_ops uart_ops = {
 	.nci_open = nfcmrvl_uart_nci_open,
 	.nci_close = nfcmrvl_uart_nci_close,
 	.nci_send = nfcmrvl_uart_nci_send,
+	.nci_ready = nfcmrvl_uart_nci_ready,
+	.nci_unready = nfcmrvl_uart_nci_unready,
 	.nci_update_config = nfcmrvl_uart_nci_update_config
 };
 
@@ -136,11 +155,6 @@ static int nfcmrvl_nci_uart_open(struct nci_uart *nu)
 	if (IS_ERR(priv))
 		return PTR_ERR(priv);
 
-	priv->support_fw_dnld = true;
-
-	nu->drv_data = priv;
-	nu->ndev = priv->ndev;
-
 	return 0;
 }
 
-- 
2.43.0

[PATCH net v5 2/2] nfc: nci: uart: quiesce write_work during teardown

[Shubham Antil]

nci_uart_tty_close() releases the active transmit state before ensuring
write_work has stopped.  A TTY write-wakeup can queue or run that worker
immediately before close, leaving the worker active while teardown
proceeds.

Serialize work scheduling and the worker's final state handoff with a
transmit lock.  Once closing starts, block new scheduling and synchronously
cancel the worker before freeing its skbs and queue.  Test and clear the
wakeup bit under the producers' lock.  This prevents a lost wakeup between
the worker's final test and clearing the sending bit.

Keep the transport close callback before the generic stop barrier because
NCI unregister may transmit a CORE_RESET command.  Require the transport to
quiesce UART callbacks before releasing its private state; the Marvell UART
transport does so after NCI unregister and before freeing that state.  Also
hold the transport module while its open callback runs and apply the same
worker barrier to the open-error path.

The teardown ordering issue was found during an LLM-assisted source audit
and reproduced using a pty-backed NCI UART under KASAN.

Fixes: 9961127d4bce ("NFC: nci: add generic uart support")
Cc: stable@vger.kernel.org
Assisted-by: LLM (OpenAI Codex, gpt-5.6-sol)
Signed-off-by: Shubham Antil <shubham@octane.security>
---
 drivers/nfc/nfcmrvl/main.c    |  7 +++
 drivers/nfc/nfcmrvl/nfcmrvl.h |  1 +
 drivers/nfc/nfcmrvl/uart.c    |  8 ++++
 include/net/nfc/nci_core.h    |  3 ++
 net/nfc/nci/uart.c            | 90 ++++++++++++++++++++++++++++-------
 5 files changed, 91 insertions(+), 18 deletions(-)

diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
index 3d26a54ef51b..45c7d3546b0e 100644
--- a/drivers/nfc/nfcmrvl/main.c
+++ b/drivers/nfc/nfcmrvl/main.c
@@ -170,6 +170,8 @@ struct nfcmrvl_private *nfcmrvl_nci_register_dev(enum nfcmrvl_phy phy,
 	return priv;
 
 error_fw_dnld_deinit:
+	if (priv->if_ops->nci_quiesce)
+		priv->if_ops->nci_quiesce(priv);
 	nfcmrvl_fw_dnld_deinit(priv);
 	if (priv->if_ops->nci_unready)
 		priv->if_ops->nci_unready(priv);
@@ -186,10 +188,15 @@ void nfcmrvl_nci_unregister_dev(struct nfcmrvl_private *priv)
 	struct nci_dev *ndev = priv->ndev;
 
 	nci_unregister_device(ndev);
+	if (priv->if_ops->nci_quiesce)
+		priv->if_ops->nci_quiesce(priv);
+
 	if (priv->ndev->nfc_dev->fw_download_in_progress)
 		nfcmrvl_fw_dnld_abort(priv);
 
 	nfcmrvl_fw_dnld_deinit(priv);
+	if (priv->if_ops->nci_unready)
+		priv->if_ops->nci_unready(priv);
 
 	nci_free_device(ndev);
 	kfree(priv);
diff --git a/drivers/nfc/nfcmrvl/nfcmrvl.h b/drivers/nfc/nfcmrvl/nfcmrvl.h
index 66f8cc06e2d5..3b7483e5aff9 100644
--- a/drivers/nfc/nfcmrvl/nfcmrvl.h
+++ b/drivers/nfc/nfcmrvl/nfcmrvl.h
@@ -114,6 +114,7 @@ struct nfcmrvl_if_ops {
 	int (*nci_send) (struct nfcmrvl_private *priv, struct sk_buff *skb);
 	void (*nci_ready)(struct nfcmrvl_private *priv);
 	void (*nci_unready)(struct nfcmrvl_private *priv);
+	void (*nci_quiesce)(struct nfcmrvl_private *priv);
 	void (*nci_update_config)(struct nfcmrvl_private *priv,
 				  const void *param);
 };
diff --git a/drivers/nfc/nfcmrvl/uart.c b/drivers/nfc/nfcmrvl/uart.c
index d688071f4b26..164896070dfc 100644
--- a/drivers/nfc/nfcmrvl/uart.c
+++ b/drivers/nfc/nfcmrvl/uart.c
@@ -61,6 +61,13 @@ static void nfcmrvl_uart_nci_unready(struct nfcmrvl_private *priv)
 	nu->ndev = NULL;
 }
 
+static void nfcmrvl_uart_nci_quiesce(struct nfcmrvl_private *priv)
+{
+	struct nci_uart *nu = priv->drv_data;
+
+	nci_uart_tx_stop(nu);
+}
+
 static void nfcmrvl_uart_nci_update_config(struct nfcmrvl_private *priv,
 					   const void *param)
 {
@@ -77,6 +84,7 @@ static const struct nfcmrvl_if_ops uart_ops = {
 	.nci_send = nfcmrvl_uart_nci_send,
 	.nci_ready = nfcmrvl_uart_nci_ready,
 	.nci_unready = nfcmrvl_uart_nci_unready,
+	.nci_quiesce = nfcmrvl_uart_nci_quiesce,
 	.nci_update_config = nfcmrvl_uart_nci_update_config
 };
 
diff --git a/include/net/nfc/nci_core.h b/include/net/nfc/nci_core.h
index 664d5058e66e..ce0ffe56593a 100644
--- a/include/net/nfc/nci_core.h
+++ b/include/net/nfc/nci_core.h
@@ -436,6 +436,7 @@ struct nci_uart;
 
 struct nci_uart_ops {
 	int (*open)(struct nci_uart *nci_uart);
+	/* Quiesce callbacks before releasing transport-private state. */
 	void (*close)(struct nci_uart *nci_uart);
 	int (*recv)(struct nci_uart *nci_uart, struct sk_buff *skb);
 	int (*send)(struct nci_uart *nci_uart, struct sk_buff *skb);
@@ -452,6 +453,7 @@ struct nci_uart {
 	/* Dynamic data */
 	struct nci_dev		*ndev;
 	spinlock_t		rx_lock;
+	spinlock_t		tx_lock; /* protects tx_state */
 	struct work_struct	write_work;
 	struct tty_struct	*tty;
 	unsigned long		tx_state;
@@ -465,5 +467,6 @@ struct nci_uart {
 int nci_uart_register(struct nci_uart *nu);
 void nci_uart_unregister(struct nci_uart *nu);
 void nci_uart_set_config(struct nci_uart *nu, int baudrate, int flow_ctrl);
+void nci_uart_tx_stop(struct nci_uart *nu);
 
 #endif /* __NCI_CORE_H */
diff --git a/net/nfc/nci/uart.c b/net/nfc/nci/uart.c
index aa20e8603f32..8e54eb1c8712 100644
--- a/net/nfc/nci/uart.c
+++ b/net/nfc/nci/uart.c
@@ -33,6 +33,7 @@
 /* TX states  */
 #define NCI_UART_SENDING	1
 #define NCI_UART_TX_WAKEUP	2
+#define NCI_UART_CLOSING	3
 
 static struct nci_uart *nci_uart_drivers[NCI_UART_DRIVER_MAX];
 
@@ -58,24 +59,59 @@ static inline int nci_uart_queue_empty(struct nci_uart *nu)
 
 static int nci_uart_tx_wakeup(struct nci_uart *nu)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&nu->tx_lock, flags);
+	if (test_bit(NCI_UART_CLOSING, &nu->tx_state))
+		goto unlock;
+
 	if (test_and_set_bit(NCI_UART_SENDING, &nu->tx_state)) {
 		set_bit(NCI_UART_TX_WAKEUP, &nu->tx_state);
-		return 0;
+		goto unlock;
 	}
 
 	schedule_work(&nu->write_work);
 
+unlock:
+	spin_unlock_irqrestore(&nu->tx_lock, flags);
+
 	return 0;
 }
 
+void nci_uart_tx_stop(struct nci_uart *nu)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&nu->tx_lock, flags);
+	set_bit(NCI_UART_CLOSING, &nu->tx_state);
+	spin_unlock_irqrestore(&nu->tx_lock, flags);
+
+	cancel_work_sync(&nu->write_work);
+
+	spin_lock_irqsave(&nu->tx_lock, flags);
+	clear_bit(NCI_UART_SENDING, &nu->tx_state);
+	clear_bit(NCI_UART_TX_WAKEUP, &nu->tx_state);
+	spin_unlock_irqrestore(&nu->tx_lock, flags);
+}
+EXPORT_SYMBOL_GPL(nci_uart_tx_stop);
+
 static void nci_uart_write_work(struct work_struct *work)
 {
 	struct nci_uart *nu = container_of(work, struct nci_uart, write_work);
 	struct tty_struct *tty = nu->tty;
 	struct sk_buff *skb;
+	unsigned long flags;
 
 restart:
+	spin_lock_irqsave(&nu->tx_lock, flags);
+	if (test_bit(NCI_UART_CLOSING, &nu->tx_state)) {
+		clear_bit(NCI_UART_SENDING, &nu->tx_state);
+		clear_bit(NCI_UART_TX_WAKEUP, &nu->tx_state);
+		spin_unlock_irqrestore(&nu->tx_lock, flags);
+		return;
+	}
 	clear_bit(NCI_UART_TX_WAKEUP, &nu->tx_state);
+	spin_unlock_irqrestore(&nu->tx_lock, flags);
 
 	if (nu->ops.tx_start)
 		nu->ops.tx_start(nu);
@@ -93,48 +129,65 @@ static void nci_uart_write_work(struct work_struct *work)
 		kfree_skb(skb);
 	}
 
-	if (test_bit(NCI_UART_TX_WAKEUP, &nu->tx_state))
-		goto restart;
-
 	if (nu->ops.tx_done && nci_uart_queue_empty(nu))
 		nu->ops.tx_done(nu);
 
+	spin_lock_irqsave(&nu->tx_lock, flags);
+	if (!test_bit(NCI_UART_CLOSING, &nu->tx_state) &&
+	    test_and_clear_bit(NCI_UART_TX_WAKEUP, &nu->tx_state)) {
+		spin_unlock_irqrestore(&nu->tx_lock, flags);
+		goto restart;
+	}
 	clear_bit(NCI_UART_SENDING, &nu->tx_state);
+	clear_bit(NCI_UART_TX_WAKEUP, &nu->tx_state);
+	spin_unlock_irqrestore(&nu->tx_lock, flags);
 }
 
 static int nci_uart_set_driver(struct tty_struct *tty, unsigned int driver)
 {
+	struct nci_uart *template;
 	struct nci_uart *nu = NULL;
 	int ret;
 
 	if (driver >= NCI_UART_DRIVER_MAX)
 		return -EINVAL;
 
-	if (!nci_uart_drivers[driver])
+	template = nci_uart_drivers[driver];
+	if (!template)
+		return -ENOENT;
+
+	if (!try_module_get(template->owner))
 		return -ENOENT;
 
 	nu = kzalloc_obj(*nu);
-	if (!nu)
+	if (!nu) {
+		module_put(template->owner);
 		return -ENOMEM;
+	}
 
-	memcpy(nu, nci_uart_drivers[driver], sizeof(struct nci_uart));
+	memcpy(nu, template, sizeof(struct nci_uart));
 	nu->tty = tty;
 	skb_queue_head_init(&nu->tx_q);
 	INIT_WORK(&nu->write_work, nci_uart_write_work);
+	spin_lock_init(&nu->tx_lock);
 	spin_lock_init(&nu->rx_lock);
 
 	ret = nu->ops.open(nu);
-	if (ret) {
-		kfree(nu);
-		return ret;
-	} else if (!try_module_get(nu->owner)) {
-		nu->ops.close(nu);
-		kfree(nu);
-		return -ENOENT;
-	}
+	if (ret)
+		goto err_stop;
+
 	tty->disc_data = nu;
 
 	return 0;
+
+err_stop:
+	nci_uart_tx_stop(nu);
+	kfree_skb(nu->tx_skb);
+	kfree_skb(nu->rx_skb);
+	skb_queue_purge(&nu->tx_q);
+	module_put(nu->owner);
+	kfree(nu);
+	return ret;
 }
 
 /* ------ LDISC part ------ */
@@ -180,17 +233,18 @@ static void nci_uart_tty_close(struct tty_struct *tty)
 	if (!nu)
 		return;
 
+	/* The transport quiesces callbacks before releasing private state. */
+	nu->ops.close(nu);
+	nci_uart_tx_stop(nu);
+
 	kfree_skb(nu->tx_skb);
 	kfree_skb(nu->rx_skb);
 
 	skb_queue_purge(&nu->tx_q);
 
-	nu->ops.close(nu);
 	nu->tty = NULL;
 	module_put(nu->owner);
 
-	cancel_work_sync(&nu->write_work);
-
 	kfree(nu);
 }
 
-- 
2.43.0