2020-04-17 15:36:53

by Madhuparna Bhowmik

Subject: [PATCH] drivers: char: tlclk.c: Avoid data race between init and interrupt handler

From: Madhuparna Bhowmik <[email protected]>

After registering character device the file operation callbacks can be
called. The open callback registers interrupt handler.
Therefore interrupt handler can execute in parallel with rest of the init
function. To avoid such data race initialize telclk_interrupt variable
and struct alarm_events before registering character device.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Madhuparna Bhowmik <[email protected]>
---
drivers/char/tlclk.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/char/tlclk.c b/drivers/char/tlclk.c
index 6d81bb3bb503..896a3550fba9 100644
--- a/drivers/char/tlclk.c
+++ b/drivers/char/tlclk.c
@@ -777,17 +777,21 @@ static int __init tlclk_init(void)
{
int ret;

+ telclk_interrupt = (inb(TLCLK_REG7) & 0x0f);
+
+ alarm_events = kzalloc( sizeof(struct tlclk_alarms), GFP_KERNEL);
+ if (!alarm_events) {
+ ret = -ENOMEM;
+ goto out1;
+ }
+
ret = register_chrdev(tlclk_major, "telco_clock", &tlclk_fops);
if (ret < 0) {
printk(KERN_ERR "tlclk: can't get major %d.\n", tlclk_major);
+ kfree(alarm_events);
return ret;
}
tlclk_major = ret;
- alarm_events = kzalloc( sizeof(struct tlclk_alarms), GFP_KERNEL);
- if (!alarm_events) {
- ret = -ENOMEM;
- goto out1;
- }

/* Read telecom clock IRQ number (Set by BIOS) */
if (!request_region(TLCLK_BASE, 8, "telco_clock")) {
@@ -796,7 +800,6 @@ static int __init tlclk_init(void)
ret = -EBUSY;
goto out2;
}
- telclk_interrupt = (inb(TLCLK_REG7) & 0x0f);

if (0x0F == telclk_interrupt ) { /* not MCPBL0010 ? */
printk(KERN_ERR "telclk_interrupt = 0x%x non-mcpbl0010 hw.\n",
@@ -837,8 +840,8 @@ static int __init tlclk_init(void)
release_region(TLCLK_BASE, 8);
out2:
kfree(alarm_events);
-out1:
unregister_chrdev(tlclk_major, "telco_clock");
+out1:
return ret;
}

--
2.17.1

2020-04-17 15:54:59

by Gross, Mark

Subject: RE: [PATCH] drivers: char: tlclk.c: Avoid data race between init and interrupt handler

Looks reasonable to me.
+1
Ack
--mark

> -----Original Message-----
> From: [email protected] <[email protected]>
> Sent: Friday, April 17, 2020 8:35 AM
> To: Gross, Mark <[email protected]>; [email protected];
> [email protected]
> Cc: [email protected]; [email protected]; ldv-
> [email protected]; Madhuparna Bhowmik
> <[email protected]>
> Subject: [PATCH] drivers: char: tlclk.c: Avoid data race between init and interrupt
> handler
>
> From: Madhuparna Bhowmik <[email protected]>
>
> After registering character device the file operation callbacks can be called. The
> open callback registers interrupt handler.
> Therefore interrupt handler can execute in parallel with rest of the init function. To
> avoid such data race initialize telclk_interrupt variable and struct alarm_events
> before registering character device.
>
> Found by Linux Driver Verification project (linuxtesting.org).
>
> Signed-off-by: Madhuparna Bhowmik <[email protected]>
> ---
> drivers/char/tlclk.c | 17 ++++++++++-------
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/char/tlclk.c b/drivers/char/tlclk.c index
> 6d81bb3bb503..896a3550fba9 100644
> --- a/drivers/char/tlclk.c
> +++ b/drivers/char/tlclk.c
> @@ -777,17 +777,21 @@ static int __init tlclk_init(void) {
> int ret;
>
> + telclk_interrupt = (inb(TLCLK_REG7) & 0x0f);
> +
> + alarm_events = kzalloc( sizeof(struct tlclk_alarms), GFP_KERNEL);
> + if (!alarm_events) {
> + ret = -ENOMEM;
> + goto out1;
> + }
> +
> ret = register_chrdev(tlclk_major, "telco_clock", &tlclk_fops);
> if (ret < 0) {
> printk(KERN_ERR "tlclk: can't get major %d.\n", tlclk_major);
> + kfree(alarm_events);
> return ret;
> }
> tlclk_major = ret;
> - alarm_events = kzalloc( sizeof(struct tlclk_alarms), GFP_KERNEL);
> - if (!alarm_events) {
> - ret = -ENOMEM;
> - goto out1;
> - }
>
> /* Read telecom clock IRQ number (Set by BIOS) */
> if (!request_region(TLCLK_BASE, 8, "telco_clock")) { @@ -796,7 +800,6
> @@ static int __init tlclk_init(void)
> ret = -EBUSY;
> goto out2;
> }
> - telclk_interrupt = (inb(TLCLK_REG7) & 0x0f);
>
> if (0x0F == telclk_interrupt ) { /* not MCPBL0010 ? */
> printk(KERN_ERR "telclk_interrupt = 0x%x non-mcpbl0010 hw.\n",
> @@ -837,8 +840,8 @@ static int __init tlclk_init(void)
> release_region(TLCLK_BASE, 8);
> out2:
> kfree(alarm_events);
> -out1:
> unregister_chrdev(tlclk_major, "telco_clock");
> +out1:
> return ret;
> }
>
> --
> 2.17.1