[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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (3) · Medium: 1 · 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).

- [Medium] The patch sets the TX-side UART state (nu->drv_data, nu->ndev)
  before nci_register_device(), but the RX side is still attached late.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  leaves it in place: the SPI transport has the same publish-after-…
- [High] Concern: on the normal teardown paths, nu->drv_data and nu->ndev
  are never withdrawn.
- [High] This problem wasn't introduced by this patch, but the USB
  transport has the same publish-after-registration bug, and it crashes…

--

nfc: nfcmrvl: publish UART state before registration

This commit adds nci_ready() and nci_unready() callbacks to
nfcmrvl_if_ops. nfcmrvl_nci_register_dev() calls nci_ready() before
nci_register_device(), so the UART transport now sets nu->drv_data,
nu->ndev and priv->support_fw_dnld before the NCI device is exposed. The
same function calls nci_unready() if registration fails, and it now calls
nfcmrvl_chip_halt() before registration.

> diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
> index 6efa832191172..3d26a54ef51b4 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);

[Severity: High]
This is a pre-existing issue and this patch didn't introduce it. The SPI
transport seems to have the same publish-after-registration problem. spi_ops
in drivers/nfc/nfcmrvl/spi.c has no .nci_ready, and nfcmrvl_spi_probe()
still sets its state only after registration returns:

drivers/nfc/nfcmrvl/spi.c:nfcmrvl_spi_probe() {
    ...
	drv_data->priv = nfcmrvl_nci_register_dev(NFCMRVL_PHY_SPI,
						  drv_data, &spi_ops,
						  &drv_data->spi->dev,
						  pdata);
    ...
	drv_data->priv->support_fw_dnld = true;

	drv_data->nci_spi = nci_spi_allocate_spi(drv_data->spi, 0, 10,
						 drv_data->priv->ndev);
    ...
	init_completion(&drv_data->handshake_completion);
}

nfc_register_device() calls device_add() before it takes device_lock(). If
an NFC_CMD_DEV_UP arrives in that window, can this path reach
nci_spi_send() with drv_data->nci_spi still NULL?

nfc_dev_up()->nci_dev_up()->nci_open_device()->CORE_RESET->nci_cmd_work()
  ->nfcmrvl_nci_send()->nfcmrvl_spi_nci_send()
    ->nci_spi_send(drv_data->nci_spi == NULL, ...)
      hdr[1] = nspi->acknowledge_mode;

The SPI IRQ is also requested before registration. If SPI_INT falls before
probe finishes, nfcmrvl_spi_int_irq_thread_fn() would call
nci_spi_read(drv_data->nci_spi) and nfcmrvl_nci_recv_frame(drv_data->priv,
skb) while both pointers are still NULL.

The I2C transport also sets support_fw_dnld only after registration, in
nfcmrvl_i2c_probe().

Should SPI implement the new nci_ready hook as well?

[Severity: High]
This is also a pre-existing issue, not one introduced by this patch. The
USB transport looks like it has the same window, and there the crash would
happen in URB completion context. usb_ops has no .nci_ready, and
nfcmrvl_probe() in drivers/nfc/nfcmrvl/usb.c sets drv_data->priv only after
registration:

	priv = nfcmrvl_nci_register_dev(NFCMRVL_PHY_USB, drv_data, &usb_ops,
					&intf->dev, &config);
	if (IS_ERR(priv))
		return PTR_ERR(priv);

	drv_data->priv = priv;

An early DEV_UP goes through nfcmrvl_usb_nci_open(), which submits the
bulk-IN URBs and sets NFCMRVL_USB_BULK_RUNNING. nfcmrvl_bulk_complete()
guards only on this check:

	if (!test_bit(NFCMRVL_NCI_RUNNING, &drv_data->flags))
		return;

NFCMRVL_NCI_RUNNING and NFCMRVL_USB_BULK_RUNNING are both bit 1, so the
guard passes. The handler then does:

		skb = nci_skb_alloc(drv_data->priv->ndev, urb->actual_length,
				    GFP_ATOMIC);

When the controller answers the CORE_RESET in that window, does this
dereference a NULL drv_data->priv?

>  	if (rc) {
>  		nfc_err(dev, "nci_register_device failed %d\n", rc);
>  		goto error_fw_dnld_deinit;
>  	}

[ ... ]

> diff --git a/drivers/nfc/nfcmrvl/uart.c b/drivers/nfc/nfcmrvl/uart.c
> index 9aedd168759c7..d688071f4b26d 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;
> +}

[Severity: Medium]
This sets the TX-side state before registration. Is the RX side still
attached too late?

nci_uart_set_driver() in net/nfc/nci/uart.c sets tty->disc_data only after
nu->ops.open() returns, and open() is the call that exposes the NCI device:

	ret = nu->ops.open(nu);
	...
	tty->disc_data = nu;

Until then, nci_uart_tty_receive() drops all incoming data:

	if (!nu || tty != nu->tty)
		return;

nci_uart_tty_wakeup() also returns early in that window.

nfc_dev_up() holds device_lock() across the whole nci_dev_up() reset
request. Meanwhile, the registering task waits on device_lock() in
nfc_register_device(), after device_add(). In the early DEV_UP case this
patch targets, the CORE_RESET goes out, but the response seems to be lost.
open() cannot return and set disc_data until the reset request times out.

Would the early DEV_UP then fail with a timeout instead of crashing, with
the NCIUARTSETDRIVER ioctl stalled for that time? Does tty->disc_data also
need to be set before the device is exposed?

> +
> +static void nfcmrvl_uart_nci_unready(struct nfcmrvl_private *priv)
> +{
> +	struct nci_uart *nu = priv->drv_data;
> +
> +	nu->drv_data = NULL;
> +	nu->ndev = NULL;
> +}

[Severity: High]
This is a pre-existing issue, not one this patch introduced, and the next
patch in this series ("nfc: nci: uart: quiesce write_work during teardown")
appears to fix it. The issue: nu->drv_data and nu->ndev are cleared only on
the registration failure path, never on normal teardown.

In nci_uart_tty_close(), nu->ops.close() runs before write_work is
cancelled:

	nu->ops.close(nu);
	nu->tty = NULL;
	module_put(nu->owner);

	cancel_work_sync(&nu->write_work);

nu->ops.close() reaches nfcmrvl_nci_unregister_dev(), which frees the
nci_dev and priv without quiescing UART TX. The try_module_get() failure
path in nci_uart_set_driver() calls nu->ops.close() and kfree(nu) without
cancelling write_work at all.

Say write_work is pending or running, for example because
nci_unregister_device()->nci_close_device() sent a CORE_RESET. Could
nfcmrvl_nci_uart_tx_start() or nfcmrvl_nci_uart_tx_done() then use the
freed priv, ndev or nu?

The later patch addresses this. It adds an nci_quiesce hook that calls
nci_uart_tx_stop() and nci_unready() in nfcmrvl_nci_unregister_dev() before
kfree(priv). It also adds a CLOSING check in nci_uart_tx_wakeup(). Finally,
it moves nci_uart_tx_stop() earlier in nci_uart_tty_close() and adds it to
the open error path in nci_uart_set_driver().

> +
>  static void nfcmrvl_uart_nci_update_config(struct nfcmrvl_private *priv,
>  					   const void *param)
>  {

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013025746.41292-1-shubham%40octane.security

[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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (2) · Medium: 1 (1) · 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).

- [Medium] nci_uart_write_work() (net/nfc/nci/uart.c) drops the old 'if
  (test_bit(NCI_UART_TX_WAKEUP)) goto restart;' check that ran before…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is a high-
  severity issue in the code this patch reworks.
- [High] This problem wasn't introduced by this patch, but it is a high-
  severity issue in the teardown sequence this patch edits.
- [Medium] nci_uart_set_driver() reads 'template =
  nci_uart_drivers[driver]' and then evaluates…

--

nfc: nci: uart: quiesce write_work during teardown

This commit adds a tx_lock and an NCI_UART_CLOSING state, so scheduling of
the nci_uart write_work is serialized against teardown. A new
nci_uart_tx_stop() blocks further scheduling and cancels the worker before
the transmit skbs are freed. The Marvell UART transport gets a new
nci_quiesce callback, and the transport module reference is now taken
before the open callback runs.

> diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
> index 3d26a54ef51b4..45c7d3546b0e9 100644
> --- a/drivers/nfc/nfcmrvl/main.c
> +++ b/drivers/nfc/nfcmrvl/main.c

[ ... ]

> @@ -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);

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. Since this
teardown sequence is being changed anyway: can the firmware download
rx_work still be running at this point?

nfcmrvl_nci_unregister_dev() runs these steps in order:

nci_unregister_device()
    destroy_workqueue(ndev->cmd_wq), rx_wq, tx_wq
nfcmrvl_fw_dnld_abort()
    fw_dnld_over()
        release_firmware(priv->fw_dnld.fw);
        priv->fw_dnld.fw = NULL;
        priv->fw_dnld.header = NULL;
        ...
nfcmrvl_fw_dnld_deinit()
    destroy_workqueue(priv->fw_dnld.rx_wq);

Suppose nfcmrvl_fw_dnld_recv_frame() queued fw_dnld_rx_work() just before
the tty close. That worker is only drained in the last step.

Until then, its state handlers can touch firmware state that has been
released or set to NULL. One example is process_state_init():

	memcpy(cmd.param.val, &priv->fw_dnld.header->ref_clock, 4);

Another is process_state_fw_dnld():

			skb_put_data(out_skb,
				     ((uint8_t *)priv->fw_dnld.fw->data) + priv->fw_dnld.offset,
				     priv->fw_dnld.chunk_len);

Handlers that call nci_send_cmd() would also queue_work() on
ndev->cmd_wq, which nci_unregister_device() has already destroyed. The
rx_work error path can also call fw_dnld_over() at the same time as the
abort.

The new nci_quiesce callback only stops the UART write_work. Should
fw_dnld.rx_wq be drained before nci_unregister_device() runs and before
fw_dnld_over() releases the firmware?

[ ... ]

> diff --git a/net/nfc/nci/uart.c b/net/nfc/nci/uart.c
> index aa20e8603f327..8e54eb1c8712e 100644
> --- a/net/nfc/nci/uart.c
> +++ b/net/nfc/nci/uart.c

[ ... ]

> @@ -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);
>  }

[Severity: Medium]
Was it intended to drop the TX_WAKEUP check that ran before tx_done()?

Before this patch, a wakeup that arrived during the write loop sent the
worker back to restart without calling tx_done(). Now tx_done() runs as
soon as nci_uart_queue_empty() is true, right after tty->ops->write()
returns.

On the Marvell UART transport with break_control set,
nfcmrvl_nci_uart_tx_done() does:

	if (priv->config.break_control && nu->tty->ops->break_ctl) {
		nu->tty->ops->break_ctl(nu->tty, -1);
		usleep_range(1000, 3000);
	}

At that point the tty driver has only buffered the bytes.
uart_break_ctl()->serial8250_do_break_ctl() sets UART_LCR_SBC and does not
wait for the transmitter to drain. Could the BREAK corrupt the tail of the
NCI frame that was just written?

For short frames, the serial write_wakeup usually fires from the TX
interrupt almost at once. That means TX_WAKEUP is likely to be set by the
time of the final check, after the sleep in tx_done(). The worker then
restarts:

nci_uart_write_work()
  nfcmrvl_nci_uart_tx_start()
    break_ctl(nu->tty, 0); usleep_range(3000, 5000);
  (nothing left to send)
  nfcmrvl_nci_uart_tx_done()
    break_ctl(nu->tty, -1); usleep_range(1000, 3000);

Does this add an extra BREAK/release pulse and several milliseconds of
latency to every frame?

Would a locked TX_WAKEUP check before tx_done(), kept alongside the new
final check, close the lost-wakeup race and still preserve the old BREAK
timing? The commit message doesn't mention this behavior change.

>  
>  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;

[Severity: Medium]
This isn't a bug introduced by this patch, but what keeps template alive
between the load from nci_uart_drivers[] and the try_module_get()?

nci_uart_unregister() clears the slot with a plain store, without taking
any lock:

	nci_uart_drivers[nu->driver] = NULL;

The template, for example the static nfcmrvl_nci_uart, lives in the
transport module's data. Suppose this ioctl is preempted right after the
load. A concurrent rmmod could then run the module exit and free the
module, and template->owner would be read from freed memory. The
try_module_get() comment in include/linux/module.h says callers must make
sure the module exists before calling it.

This patch does pin the module across ops.open(), as the commit message
says. The remaining window predates the patch, and the old code had a
wider one. Would putting a lock or RCU read section around the registry
lookup and the try_module_get() close it?

[ ... ]

>  	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;

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. Can two
concurrent NCIUARTSETDRIVER ioctls on the same tty both reach this store?

nci_uart_tty_ioctl() checks disc_data without holding any lock:

	case NCIUARTSETDRIVER:
		if (!nu)
			return nci_uart_set_driver(tty, (unsigned int)arg);

tty_ioctl() holds only a shared tty_ldisc_ref_wait() reference around
ld->ops->ioctl(). So two threads can both see disc_data == NULL. Each one
allocates an nci_uart, takes a module reference and calls ops.open(),
which registers its own NCI device through nfcmrvl_nci_register_dev().
Whichever thread stores to tty->disc_data last wins.

The other nu is left orphaned. Its NCI device stays registered, its
nu->tty still points at the tty, and it keeps holding the transport
module. nci_uart_tty_close() only tears down the nu in disc_data. The
orphan never gets ops.close() or nci_uart_tx_stop(), so the new
NCI_UART_CLOSING barrier never applies to it.

After the tty is freed, bringing up the orphaned device would go:

NFC_CMD_DEV_UP
  nci_open_device()
    ... CORE_RESET ...
      nfcmrvl_nci_send()
        nci_uart_send()
          nci_uart_tx_wakeup()
            nci_uart_write_work()
              tty->ops->write()

and that write would run on the freed tty_struct. Even without that
path, the nci_uart, nfcmrvl_private and nci_dev are leaked. Should the
disc_data check and the store to it be serialized?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013025746.41292-1-shubham%40octane.security