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

[netdev-bot+sashiko@kernel.org]

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