1. Patchew
  2. linux
  3. View series

[PATCH] tty: serial: uartlite: ensure uart driver is registered

Elodie Decerle posted 1 patch 9 months, 1 week ago 
drivers/tty/serial/uartlite.c | 6 ++++++
1 file changed, 6 insertions(+)
[PATCH] tty: serial: uartlite: ensure uart driver is registered
Posted by Elodie Decerle 9 months, 1 week ago 
When two instances of uart devices are probing, a concurrency race can
occur. If one thread calls uart_register_driver function, which first
allocates and assigns memory to 'uart_state' member of uart_driver
structure, the other instance can bypass uart driver registration and
call ulite_assign. This calls uart_add_one_port, which expects the uart
driver to be fully initialized. This leads to a kernel panic due to a
null pointer dereference:

[    8.143581] BUG: kernel NULL pointer dereference, address: 00000000000002b8
[    8.156982] #PF: supervisor write access in kernel mode
[    8.156984] #PF: error_code(0x0002) - not-present page
[    8.156986] PGD 0 P4D 0
...
[    8.180668] RIP: 0010:mutex_lock+0x19/0x30
[    8.188624] Call Trace:
[    8.188629]  ? __die_body.cold+0x1a/0x1f
[    8.195260]  ? page_fault_oops+0x15c/0x290
[    8.209183]  ? __irq_resolve_mapping+0x47/0x80
[    8.209187]  ? exc_page_fault+0x64/0x140
[    8.209190]  ? asm_exc_page_fault+0x22/0x30
[    8.209196]  ? mutex_lock+0x19/0x30
[    8.223116]  uart_add_one_port+0x60/0x440
[    8.223122]  ? proc_tty_register_driver+0x43/0x50
[    8.223126]  ? tty_register_driver+0x1ca/0x1e0
[    8.246250]  ulite_probe+0x357/0x4b0 [uartlite]

Adding a mutex lock around the uart_register_driver call in the probe
function prevents this race condition and ensures that the uart driver
structure is fully initialized and registered before it is used.

Signed-off-by: Elodie Decerle <elodie.decerle@nokia.com>
Reviewed-by: Jakub Lewalski <jakub.lewalski@nokia.com>
---
 drivers/tty/serial/uartlite.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
index a41e7fc373b7..460eb2032efa 100644
--- a/drivers/tty/serial/uartlite.c
+++ b/drivers/tty/serial/uartlite.c
@@ -23,6 +23,8 @@
 #include <linux/clk.h>
 #include <linux/pm_runtime.h>
 
+static DEFINE_MUTEX(uart_driver_register_lock);
+
 #define ULITE_NAME		"ttyUL"
 #if CONFIG_SERIAL_UARTLITE_NR_UARTS > 4
 #define ULITE_MAJOR             0       /* use dynamic node allocation */
@@ -880,6 +882,8 @@ static int ulite_probe(struct platform_device *pdev)
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
 
+	mutex_lock(&uart_driver_register_lock);
+
 	if (!ulite_uart_driver.state) {
 		dev_dbg(&pdev->dev, "uartlite: calling uart_register_driver()\n");
 		ret = uart_register_driver(&ulite_uart_driver);
@@ -890,6 +894,8 @@ static int ulite_probe(struct platform_device *pdev)
 		}
 	}
 
+	mutex_unlock(&uart_driver_register_lock);
+
 	ret = ulite_assign(&pdev->dev, id, res->start, irq, pdata);
 
 	pm_runtime_mark_last_busy(&pdev->dev);
-- 
2.43.0
Re: [PATCH] tty: serial: uartlite: ensure uart driver is registered
Posted by Greg KH 9 months, 1 week ago 
On Thu, Mar 13, 2025 at 09:58:50PM +0100, Elodie Decerle wrote:
> When two instances of uart devices are probing, a concurrency race can
> occur.
> 
> If one thread calls uart_register_driver function, which first
> allocates and assigns memory to 'uart_state' member of uart_driver
> structure, the other instance can bypass uart driver registration and
> call ulite_assign. This calls uart_add_one_port, which expects the uart
> driver to be fully initialized. This leads to a kernel panic due to a
> null pointer dereference:
> 
> [    8.143581] BUG: kernel NULL pointer dereference, address: 00000000000002b8
> [    8.156982] #PF: supervisor write access in kernel mode
> [    8.156984] #PF: error_code(0x0002) - not-present page
> [    8.156986] PGD 0 P4D 0
> ...
> [    8.180668] RIP: 0010:mutex_lock+0x19/0x30
> [    8.188624] Call Trace:
> [    8.188629]  ? __die_body.cold+0x1a/0x1f
> [    8.195260]  ? page_fault_oops+0x15c/0x290
> [    8.209183]  ? __irq_resolve_mapping+0x47/0x80
> [    8.209187]  ? exc_page_fault+0x64/0x140
> [    8.209190]  ? asm_exc_page_fault+0x22/0x30
> [    8.209196]  ? mutex_lock+0x19/0x30
> [    8.223116]  uart_add_one_port+0x60/0x440
> [    8.223122]  ? proc_tty_register_driver+0x43/0x50
> [    8.223126]  ? tty_register_driver+0x1ca/0x1e0
> [    8.246250]  ulite_probe+0x357/0x4b0 [uartlite]
> 
> Adding a mutex lock around the uart_register_driver call in the probe
> function prevents this race condition and ensures that the uart driver
> structure is fully initialized and registered before it is used.
> 
> Signed-off-by: Elodie Decerle <elodie.decerle@nokia.com>
> Reviewed-by: Jakub Lewalski <jakub.lewalski@nokia.com>
> ---
>  drivers/tty/serial/uartlite.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
> index a41e7fc373b7..460eb2032efa 100644
> --- a/drivers/tty/serial/uartlite.c
> +++ b/drivers/tty/serial/uartlite.c
> @@ -23,6 +23,8 @@
>  #include <linux/clk.h>
>  #include <linux/pm_runtime.h>
>  
> +static DEFINE_MUTEX(uart_driver_register_lock);
> +
>  #define ULITE_NAME		"ttyUL"
>  #if CONFIG_SERIAL_UARTLITE_NR_UARTS > 4
>  #define ULITE_MAJOR             0       /* use dynamic node allocation */
> @@ -880,6 +882,8 @@ static int ulite_probe(struct platform_device *pdev)
>  	pm_runtime_set_active(&pdev->dev);
>  	pm_runtime_enable(&pdev->dev);
>  
> +	mutex_lock(&uart_driver_register_lock);
> +
>  	if (!ulite_uart_driver.state) {

So the problem that there is a single "state" for the driver as a whole.
That should be fixed up to be local to each individual device that is
added to the system.  Don't add a lock to paper over this as odds are
this is not the only place that will have problems.

Are you just now having 2 of these devices in your system at the same
time?

thanks,

greg k-h
Re: [PATCH] tty: serial: uartlite: ensure uart driver is registered
Posted by Maarten Brock 9 months ago 
Hi,

> > When two instances of uart devices are probing, a concurrency race can
> > occur.
> >
> > Adding a mutex lock around the uart_register_driver call in the probe
> > function prevents this race condition and ensures that the uart driver
> > structure is fully initialized and registered before it is used.
> 
> So the problem that there is a single "state" for the driver as a whole.
> That should be fixed up to be local to each individual device that is
> added to the system.  Don't add a lock to paper over this as odds are
> this is not the only place that will have problems.
> 
> Are you just now having 2 of these devices in your system at the same
> time?

In the past I have created a Zynq 7000 based system (dual core ARM) with
11 uartlites and I have not seen this problem. This was with a 5.10 kernel.

Maarten
Re: [PATCH] tty: serial: uartlite: ensure uart driver is registered
Posted by Elodie Decerle 8 months, 4 weeks ago 
Hi,

On 18/03/2025 10:46, Maarten Brock wrote:
> [Some people who received this message don't often get email from maarten.brock@sttls.nl. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> CAUTION: This is an external email. Please be very careful when clicking links or opening attachments. See the URL nok.it/ext for additional information.
> 
> 
> 
> Hi,
> 
>>> When two instances of uart devices are probing, a concurrency race can
>>> occur.
>>>
>>> Adding a mutex lock around the uart_register_driver call in the probe
>>> function prevents this race condition and ensures that the uart driver
>>> structure is fully initialized and registered before it is used.
>>
>> So the problem that there is a single "state" for the driver as a whole.
>> That should be fixed up to be local to each individual device that is
>> added to the system.  Don't add a lock to paper over this as odds are
>> this is not the only place that will have problems.
>>
>> Are you just now having 2 of these devices in your system at the same
>> time?
> 
> In the past I have created a Zynq 7000 based system (dual core ARM) with
> 11 uartlites and I have not seen this problem. This was with a 5.10 kernel.
> 
> Maarten

Thanks for your fast reviewing and suggestion. In fact, we are using
multiple UART devices in our multi-cores system. The problem is
extremely rare and we could not even reproduce it so far. We'll provide
another version for this patch based on your comments, Greg.

Best regards,

Elodie
[PATCH v3] tty: serial: uartlite: register uart driver in init
Posted by Elodie Decerle 8 months, 3 weeks ago 
From: Jakub Lewalski <jakub.lewalski@nokia.com>

When two instances of uart devices are probing, a concurrency race can
occur. If one thread calls uart_register_driver function, which first
allocates and assigns memory to 'uart_state' member of uart_driver
structure, the other instance can bypass uart driver registration and
call ulite_assign. This calls uart_add_one_port, which expects the uart
driver to be fully initialized. This leads to a kernel panic due to a
null pointer dereference:

[    8.143581] BUG: kernel NULL pointer dereference, address: 00000000000002b8
[    8.156982] #PF: supervisor write access in kernel mode
[    8.156984] #PF: error_code(0x0002) - not-present page
[    8.156986] PGD 0 P4D 0
...
[    8.180668] RIP: 0010:mutex_lock+0x19/0x30
[    8.188624] Call Trace:
[    8.188629]  ? __die_body.cold+0x1a/0x1f
[    8.195260]  ? page_fault_oops+0x15c/0x290
[    8.209183]  ? __irq_resolve_mapping+0x47/0x80
[    8.209187]  ? exc_page_fault+0x64/0x140
[    8.209190]  ? asm_exc_page_fault+0x22/0x30
[    8.209196]  ? mutex_lock+0x19/0x30
[    8.223116]  uart_add_one_port+0x60/0x440
[    8.223122]  ? proc_tty_register_driver+0x43/0x50
[    8.223126]  ? tty_register_driver+0x1ca/0x1e0
[    8.246250]  ulite_probe+0x357/0x4b0 [uartlite]

To prevent it, move uart driver registration in to init function. This
will ensure that uart_driver is always registered when probe function
is called.

Signed-off-by: Jakub Lewalski <jakub.lewalski@nokia.com>
Signed-off-by: Elodie Decerle <elodie.decerle@nokia.com>
---
V2 -> V3: Cleaned up patch format
V1 -> V2:
- Removed mutex lock in uart_register_driver
- Moved uart driver registration to init function (Greg's feedback)

 drivers/tty/serial/uartlite.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
index a41e7fc373b7..39c1fd1ff9ce 100644
--- a/drivers/tty/serial/uartlite.c
+++ b/drivers/tty/serial/uartlite.c
@@ -880,16 +880,6 @@ static int ulite_probe(struct platform_device *pdev)
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
 
-	if (!ulite_uart_driver.state) {
-		dev_dbg(&pdev->dev, "uartlite: calling uart_register_driver()\n");
-		ret = uart_register_driver(&ulite_uart_driver);
-		if (ret < 0) {
-			dev_err(&pdev->dev, "Failed to register driver\n");
-			clk_disable_unprepare(pdata->clk);
-			return ret;
-		}
-	}
-
 	ret = ulite_assign(&pdev->dev, id, res->start, irq, pdata);
 
 	pm_runtime_mark_last_busy(&pdev->dev);
@@ -929,16 +919,25 @@ static struct platform_driver ulite_platform_driver = {
 
 static int __init ulite_init(void)
 {
+	int ret;
+
+	pr_debug("uartlite: calling uart_register_driver()\n");
+	ret = uart_register_driver(&ulite_uart_driver);
+	if (ret)
+		return ret;
 
 	pr_debug("uartlite: calling platform_driver_register()\n");
-	return platform_driver_register(&ulite_platform_driver);
+	ret = platform_driver_register(&ulite_platform_driver);
+	if (ret)
+		uart_unregister_driver(&ulite_uart_driver);
+
+	return ret;
 }
 
 static void __exit ulite_exit(void)
 {
 	platform_driver_unregister(&ulite_platform_driver);
-	if (ulite_uart_driver.state)
-		uart_unregister_driver(&ulite_uart_driver);
+	uart_unregister_driver(&ulite_uart_driver);
 }
 
 module_init(ulite_init);
-- 
2.43.0
[PATCH v2] tty: serial: uartlite: register uart driver in init
Posted by Elodie Decerle 8 months, 3 weeks ago 
From: Jakub Lewalski <jakub.lewalski@nokia.com>

When two instances of uart devices are probing, a concurrency race can
occur. If one thread calls uart_register_driver function, which first
allocates and assigns memory to 'uart_state' member of uart_driver
structure, the other instance can bypass uart driver registration and
call ulite_assign. This calls uart_add_one_port, which expects the uart
driver to be fully initialized. This leads to a kernel panic due to a
null pointer dereference:

[    8.143581] BUG: kernel NULL pointer dereference, address: 00000000000002b8
[    8.156982] #PF: supervisor write access in kernel mode
[    8.156984] #PF: error_code(0x0002) - not-present page
[    8.156986] PGD 0 P4D 0
...
[    8.180668] RIP: 0010:mutex_lock+0x19/0x30
[    8.188624] Call Trace:
[    8.188629]  ? __die_body.cold+0x1a/0x1f
[    8.195260]  ? page_fault_oops+0x15c/0x290
[    8.209183]  ? __irq_resolve_mapping+0x47/0x80
[    8.209187]  ? exc_page_fault+0x64/0x140
[    8.209190]  ? asm_exc_page_fault+0x22/0x30
[    8.209196]  ? mutex_lock+0x19/0x30
[    8.223116]  uart_add_one_port+0x60/0x440
[    8.223122]  ? proc_tty_register_driver+0x43/0x50
[    8.223126]  ? tty_register_driver+0x1ca/0x1e0
[    8.246250]  ulite_probe+0x357/0x4b0 [uartlite]

To prevent it, move uart driver registration in to init function. This
will ensure that uart_driver is always registered when probe function
is called.

Signed-off-by: Jakub Lewalski <jakub.lewalski@nokia.com>
Tested-by: Elodie Decerle <elodie.decerle@nokia.com>

Changes since v1:
- Remove mutex lock in uart_register_driver
- Move uart driver registration to init function (Greg's feedback)
---
 drivers/tty/serial/uartlite.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
index a41e7fc373b7..39c1fd1ff9ce 100644
--- a/drivers/tty/serial/uartlite.c
+++ b/drivers/tty/serial/uartlite.c
@@ -880,16 +880,6 @@ static int ulite_probe(struct platform_device *pdev)
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
 
-	if (!ulite_uart_driver.state) {
-		dev_dbg(&pdev->dev, "uartlite: calling uart_register_driver()\n");
-		ret = uart_register_driver(&ulite_uart_driver);
-		if (ret < 0) {
-			dev_err(&pdev->dev, "Failed to register driver\n");
-			clk_disable_unprepare(pdata->clk);
-			return ret;
-		}
-	}
-
 	ret = ulite_assign(&pdev->dev, id, res->start, irq, pdata);
 
 	pm_runtime_mark_last_busy(&pdev->dev);
@@ -929,16 +919,25 @@ static struct platform_driver ulite_platform_driver = {
 
 static int __init ulite_init(void)
 {
+	int ret;
+
+	pr_debug("uartlite: calling uart_register_driver()\n");
+	ret = uart_register_driver(&ulite_uart_driver);
+	if (ret)
+		return ret;
 
 	pr_debug("uartlite: calling platform_driver_register()\n");
-	return platform_driver_register(&ulite_platform_driver);
+	ret = platform_driver_register(&ulite_platform_driver);
+	if (ret)
+		uart_unregister_driver(&ulite_uart_driver);
+
+	return ret;
 }
 
 static void __exit ulite_exit(void)
 {
 	platform_driver_unregister(&ulite_platform_driver);
-	if (ulite_uart_driver.state)
-		uart_unregister_driver(&ulite_uart_driver);
+	uart_unregister_driver(&ulite_uart_driver);
 }
 
 module_init(ulite_init);
-- 
2.43.0
Re: [PATCH v2] tty: serial: uartlite: register uart driver in init
Posted by Greg KH 8 months, 3 weeks ago 
On Wed, Mar 26, 2025 at 11:04:57AM +0100, Elodie Decerle wrote:
> From: Jakub Lewalski <jakub.lewalski@nokia.com>
> 
> When two instances of uart devices are probing, a concurrency race can
> occur. If one thread calls uart_register_driver function, which first
> allocates and assigns memory to 'uart_state' member of uart_driver
> structure, the other instance can bypass uart driver registration and
> call ulite_assign. This calls uart_add_one_port, which expects the uart
> driver to be fully initialized. This leads to a kernel panic due to a
> null pointer dereference:
> 
> [    8.143581] BUG: kernel NULL pointer dereference, address: 00000000000002b8
> [    8.156982] #PF: supervisor write access in kernel mode
> [    8.156984] #PF: error_code(0x0002) - not-present page
> [    8.156986] PGD 0 P4D 0
> ...
> [    8.180668] RIP: 0010:mutex_lock+0x19/0x30
> [    8.188624] Call Trace:
> [    8.188629]  ? __die_body.cold+0x1a/0x1f
> [    8.195260]  ? page_fault_oops+0x15c/0x290
> [    8.209183]  ? __irq_resolve_mapping+0x47/0x80
> [    8.209187]  ? exc_page_fault+0x64/0x140
> [    8.209190]  ? asm_exc_page_fault+0x22/0x30
> [    8.209196]  ? mutex_lock+0x19/0x30
> [    8.223116]  uart_add_one_port+0x60/0x440
> [    8.223122]  ? proc_tty_register_driver+0x43/0x50
> [    8.223126]  ? tty_register_driver+0x1ca/0x1e0
> [    8.246250]  ulite_probe+0x357/0x4b0 [uartlite]
> 
> To prevent it, move uart driver registration in to init function. This
> will ensure that uart_driver is always registered when probe function
> is called.
> 
> Signed-off-by: Jakub Lewalski <jakub.lewalski@nokia.com>
> Tested-by: Elodie Decerle <elodie.decerle@nokia.com>

If you forward on a patch from someone else, you also have to sign off
on it.  Please read our documentation for what this means.

> 
> Changes since v1:
> - Remove mutex lock in uart_register_driver
> - Move uart driver registration to init function (Greg's feedback)

The changes stuff goes below the:

> ---

line.

Again, our documentation should explain this.

thanks,

greg k-h

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.