[PATCH] NFC: nci: uart: Set tty->disc_data only in success path

[PATCH] NFC: nci: uart: Set tty->disc_data only in success path

[Krzysztof Kozlowski]

Setting tty->disc_data before opening the NCI device means we need to
clean it up on error paths.  This also opens some short window if device
starts sending data, even before NCIUARTSETDRIVER IOCTL succeeded
(broken hardware?).  Close the window by exposing tty->disc_data only on
the success path, when opening of the NCI device and try_module_get()
succeeds.

The code differs in error path in one aspect: tty->disc_data won't be
ever assigned thus NULL-ified.  This however should not be relevant
difference, because of "tty->disc_data=NULL" in nci_uart_tty_open().

Cc: Greg KH <gregkh@linuxfoundation.org>
Cc: Linus Torvalds <torvalds@linuxfoundation.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Fixes: 9961127d4bce ("NFC: nci: add generic uart support")
Cc: <stable@vger.kernel.org>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 net/nfc/nci/uart.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/nfc/nci/uart.c b/net/nfc/nci/uart.c
index ed1508a9e093..aab107727f18 100644
--- a/net/nfc/nci/uart.c
+++ b/net/nfc/nci/uart.c
@@ -119,22 +119,22 @@ static int nci_uart_set_driver(struct tty_struct *tty, unsigned int driver)
 
 	memcpy(nu, nci_uart_drivers[driver], sizeof(struct nci_uart));
 	nu->tty = tty;
-	tty->disc_data = nu;
 	skb_queue_head_init(&nu->tx_q);
 	INIT_WORK(&nu->write_work, nci_uart_write_work);
 	spin_lock_init(&nu->rx_lock);
 
 	ret = nu->ops.open(nu);
 	if (ret) {
-		tty->disc_data = NULL;
 		kfree(nu);
+		return ret;
 	} else if (!try_module_get(nu->owner)) {
 		nu->ops.close(nu);
-		tty->disc_data = NULL;
 		kfree(nu);
 		return -ENOENT;
 	}
-	return ret;
+	tty->disc_data = nu;
+
+	return 0;
 }
 
 /* ------ LDISC part ------ */
-- 
2.45.2

Re: [PATCH] NFC: nci: uart: Set tty->disc_data only in success path

[Greg KH]

On Wed, Jun 18, 2025 at 09:36:50AM +0200, Krzysztof Kozlowski wrote:
> Setting tty->disc_data before opening the NCI device means we need to
> clean it up on error paths.  This also opens some short window if device
> starts sending data, even before NCIUARTSETDRIVER IOCTL succeeded
> (broken hardware?).  Close the window by exposing tty->disc_data only on
> the success path, when opening of the NCI device and try_module_get()
> succeeds.
> 
> The code differs in error path in one aspect: tty->disc_data won't be
> ever assigned thus NULL-ified.  This however should not be relevant
> difference, because of "tty->disc_data=NULL" in nci_uart_tty_open().
> 
> Cc: Greg KH <gregkh@linuxfoundation.org>
> Cc: Linus Torvalds <torvalds@linuxfoundation.org>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Fixes: 9961127d4bce ("NFC: nci: add generic uart support")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>  net/nfc/nci/uart.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/net/nfc/nci/uart.c b/net/nfc/nci/uart.c
> index ed1508a9e093..aab107727f18 100644
> --- a/net/nfc/nci/uart.c
> +++ b/net/nfc/nci/uart.c
> @@ -119,22 +119,22 @@ static int nci_uart_set_driver(struct tty_struct *tty, unsigned int driver)
>  
>  	memcpy(nu, nci_uart_drivers[driver], sizeof(struct nci_uart));
>  	nu->tty = tty;
> -	tty->disc_data = nu;
>  	skb_queue_head_init(&nu->tx_q);
>  	INIT_WORK(&nu->write_work, nci_uart_write_work);
>  	spin_lock_init(&nu->rx_lock);
>  
>  	ret = nu->ops.open(nu);
>  	if (ret) {
> -		tty->disc_data = NULL;
>  		kfree(nu);
> +		return ret;
>  	} else if (!try_module_get(nu->owner)) {
>  		nu->ops.close(nu);
> -		tty->disc_data = NULL;
>  		kfree(nu);
>  		return -ENOENT;
>  	}
> -	return ret;
> +	tty->disc_data = nu;
> +
> +	return 0;
>  }

Looks good, thanks for cleaning this up:

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH] NFC: nci: uart: Set tty->disc_data only in success path

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 18 Jun 2025 09:36:50 +0200 you wrote:
> Setting tty->disc_data before opening the NCI device means we need to
> clean it up on error paths.  This also opens some short window if device
> starts sending data, even before NCIUARTSETDRIVER IOCTL succeeded
> (broken hardware?).  Close the window by exposing tty->disc_data only on
> the success path, when opening of the NCI device and try_module_get()
> succeeds.
> 
> [...]

Here is the summary with links:
  - NFC: nci: uart: Set tty->disc_data only in success path
    https://git.kernel.org/netdev/net/c/fc27ab48904c

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html