rtc_cmos oops in cmos_rtc_ioctl

rtc_cmos oops in cmos_rtc_ioctl

[Herton Ronaldo Krzesinski]

Hi,

Currently there is a problem with rtc_cmos. On one machine, I can make it oops
if I induce it to boot slowly, for example if I enable serial console in it with
"console=ttyS0,9600 console=tty0 ignore_loglevel". Then I get following trace:

rtc_cmos 00:03: RTC can wake from S4
rtc_cmos 00:03: rtc core: registered rtc_cmos as rtc0
input: PC Speaker as /devices/platform/pcspkr/input/input3
BUG: unable to handle kernel NULL pointer dereference at 00000008
IP: [<f89f1b70>] cmos_rtc_ioctl+0x30/0xd0 [rtc_cmos]
*pde = 00000000 
Oops: 0000 [#1] SMP 
last sysfs file: /sys/devices/pnp0/00:03/rtc/rtc0/dev
Modules linked in: pcspkr sr_mod(+) rtc_cmos(+) r8169 thermal button soundcore snd_page_alloc mii processor usbcore ata_generic ide_pci_generic ide_gd_mod ide_core pata_acpi ahci ata_piix libata sd_mod scsi_mod crc_t10dif ext3 jbd i915 drm i2c_algo_bit i2c_core video output

Pid: 532, comm: hwclock Not tainted (2.6.31-desktop-2mnb #1) System Product Name
EIP: 0060:[<f89f1b70>] EFLAGS: 00010246 CPU: 0
EIP is at cmos_rtc_ioctl+0x30/0xd0 [rtc_cmos]
EAX: fffffdfd EBX: 00000000 ECX: 00000003 EDX: 00007004
ESI: f89f1b40 EDI: 00007004 EBP: f613beb4 ESP: f613bea4
 DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
Process hwclock (pid: 532, ti=f613a000 task=f66a0c70 task.ti=f613a000)
Stack:
 f613beb4 e6fff99b f6516c00 f89f1b40 f613bf2c c039c96b f89f2b20 00000000
<0> f6516cd8 f613bf20 00000000 00000000 ffffff9c f6fe4000 00008000 00000000
<0> 00000000 f609ac00 f666cc40 f6459780 f6a31d48 c2bec380 df61c025 00000000
Call Trace:
 [<f89f1b40>] ? cmos_rtc_ioctl+0x0/0xd0 [rtc_cmos]
 [<c039c96b>] ? rtc_dev_ioctl+0x18b/0x480
 [<c039cc88>] ? rtc_dev_release+0x28/0x70
 [<c0202e12>] ? __fput+0xf2/0x200
 [<c0202f47>] ? fput+0x27/0x40
 [<c01fea87>] ? filp_close+0x57/0xa0
 [<c01feb3e>] ? sys_close+0x6e/0xc0
 [<c010407b>] ? sysenter_do_call+0x12/0x28
Code: 10 89 5d f8 89 75 fc 0f 1f 44 00 00 65 8b 0d 14 00 00 00 89 4d f4 31 c9 8d 8a ff 8f ff ff 83 f9 03 8b 58 4c b8 fd fd ff ff 77 50 <8b> 4b 08 66 b8 ea ff 85 c9 7e 45 b8 2c e9 6c c0 89 55 f0 e8 68 
EIP: [<f89f1b70>] cmos_rtc_ioctl+0x30/0xd0 [rtc_cmos] SS:ESP 0068:f613bea4
CR2: 0000000000000008
---[ end trace add67abfa3790852 ]---
rtc0: alarms up to one month, y3k, 114 bytes nvram, hpet irqs

The problem here is the rtc char device being created early and acessible before
rtc_cmos does dev_set_drvdata(dev, &cmos_rtc), so dev_get_drvdata in
cmos_rtc_ioctl can return null, like in this example where hwclock is run right
after char device creation that triggers the udev rule:
ACTION=="add", SUBSYSTEM=="rtc", RUN+="/sbin/hwclock --hctosys --rtc=/dev/%k"
And makes the oops possible, in this case hwclock looks to open and close the
device fast enough.

Not all machines reproduce it consistently this when using serial console (I
tried other two just by curiosity), but in a bug report where I asked for a
serial console dump the trace I got had also the similar oops, but on a bit
older kernel (2.6.29).

The fix is to just call dev_set_drvdata before device creation (before
rtc_device_register), like following patch:

diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index f7a4701..071f9ed 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -723,6 +723,8 @@ cmos_do_probe(struct device *dev, struct resource *ports, int rtc_irq)
 		}
 	}
 
+	dev_set_drvdata(dev, &cmos_rtc);
+
 	cmos_rtc.rtc = rtc_device_register(driver_name, dev,
 				&cmos_rtc_ops, THIS_MODULE);
 	if (IS_ERR(cmos_rtc.rtc)) {
@@ -731,7 +733,6 @@ cmos_do_probe(struct device *dev, struct resource *ports, int rtc_irq)
 	}
 
 	cmos_rtc.dev = dev;
-	dev_set_drvdata(dev, &cmos_rtc);
 	rename_region(ports, dev_name(&cmos_rtc.rtc->dev));
 
 	spin_lock_irq(&rtc_lock);


But I saw another issue: looks it could be possible that as cmos_rtc_ioctl
(ioctl) can be run before rtc_device_register returns, the following call chain
could happen in current code:
cmos_rtc_ioctl->cmos_irq_{en,dis}able->cmos_checkintr->rtc_update_irq
rtc_update_irq uses cmos->rtc, which is set only at return of
rtc_device_register, and here we may have another problem... is it
possible?

--
[]'s
Herton

Re: rtc_cmos oops in cmos_rtc_ioctl

[Alessandro Zummo]

On Mon, 21 Sep 2009 15:53:38 -0300
Herton Ronaldo Krzesinski <herton@mandriva.com.br> wrote:

> The problem here is the rtc char device being created early and acessible before
> rtc_cmos does dev_set_drvdata(dev, &cmos_rtc), so dev_get_drvdata in
> cmos_rtc_ioctl can return null, like in this example where hwclock is run right
> after char device creation that triggers the udev rule:
> ACTION=="add", SUBSYSTEM=="rtc", RUN+="/sbin/hwclock --hctosys --rtc=/dev/%k"
> And makes the oops possible, in this case hwclock looks to open and close the
> device fast enough.

 right. the best option would be to use the new irq api that was
 introduced after the creation of rtc_cmos (and thus remove the whole
 ioctl routine).

 [...]

> But I saw another issue: looks it could be possible that as cmos_rtc_ioctl
> (ioctl) can be run before rtc_device_register returns, the following call chain
> could happen in current code:
> cmos_rtc_ioctl->cmos_irq_{en,dis}able->cmos_checkintr->rtc_update_irq
> rtc_update_irq uses cmos->rtc, which is set only at return of
> rtc_device_register, and here we may have another problem... is it
> possible?

 this shouldn't happen, irqs are enabled only after everything
 has been setup to handle them.

 

-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it

Re: rtc_cmos oops in cmos_rtc_ioctl

[Herton Ronaldo Krzesinski]

Em Ter 22 Set 2009, às 07:40:41, Alessandro Zummo escreveu:
> On Mon, 21 Sep 2009 15:53:38 -0300
> Herton Ronaldo Krzesinski <herton@mandriva.com.br> wrote:
> 
> > The problem here is the rtc char device being created early and acessible before
> > rtc_cmos does dev_set_drvdata(dev, &cmos_rtc), so dev_get_drvdata in
> > cmos_rtc_ioctl can return null, like in this example where hwclock is run right
> > after char device creation that triggers the udev rule:
> > ACTION=="add", SUBSYSTEM=="rtc", RUN+="/sbin/hwclock --hctosys --rtc=/dev/%k"
> > And makes the oops possible, in this case hwclock looks to open and close the
> > device fast enough.
> 
>  right. the best option would be to use the new irq api that was
>  introduced after the creation of rtc_cmos (and thus remove the whole
>  ioctl routine).
> 
>  [...]

Something like this?:

>From a94365843ab40a1904c4bc244af4e551f2f2aca9 Mon Sep 17 00:00:00 2001
From: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
Date: Tue, 22 Sep 2009 13:46:00 -0300
Subject: [PATCH] rtc-cmos: convert RTC_AIE/RTC_UIE to rtc irq API

Drop ioctl function that handles RTC_AIE/RTC_UIE, and use instead the
rtc subsystem API (alarm_irq_enable/update_irq_enable callbacks).

Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
---
 drivers/rtc/rtc-cmos.c |   75 ++++++++++++++++++++++-------------------------
 1 files changed, 35 insertions(+), 40 deletions(-)

diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index f7a4701..a472242 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -420,49 +420,43 @@ static int cmos_irq_set_state(struct device *dev, int enabled)
 	return 0;
 }
 
-#if defined(CONFIG_RTC_INTF_DEV) || defined(CONFIG_RTC_INTF_DEV_MODULE)
-
-static int
-cmos_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
+static int cmos_alarm_irq_enable(struct device *dev, unsigned int enabled)
 {
 	struct cmos_rtc	*cmos = dev_get_drvdata(dev);
 	unsigned long	flags;
 
-	switch (cmd) {
-	case RTC_AIE_OFF:
-	case RTC_AIE_ON:
-	case RTC_UIE_OFF:
-	case RTC_UIE_ON:
-		if (!is_valid_irq(cmos->irq))
-			return -EINVAL;
-		break;
-	/* PIE ON/OFF is handled by cmos_irq_set_state() */
-	default:
-		return -ENOIOCTLCMD;
-	}
+	if (!is_valid_irq(cmos->irq))
+		return -EINVAL;
 
 	spin_lock_irqsave(&rtc_lock, flags);
-	switch (cmd) {
-	case RTC_AIE_OFF:	/* alarm off */
-		cmos_irq_disable(cmos, RTC_AIE);
-		break;
-	case RTC_AIE_ON:	/* alarm on */
+
+	if (enabled)
 		cmos_irq_enable(cmos, RTC_AIE);
-		break;
-	case RTC_UIE_OFF:	/* update off */
-		cmos_irq_disable(cmos, RTC_UIE);
-		break;
-	case RTC_UIE_ON:	/* update on */
-		cmos_irq_enable(cmos, RTC_UIE);
-		break;
-	}
+	else
+		cmos_irq_disable(cmos, RTC_AIE);
+
 	spin_unlock_irqrestore(&rtc_lock, flags);
 	return 0;
 }
 
-#else
-#define	cmos_rtc_ioctl	NULL
-#endif
+static int cmos_update_irq_enable(struct device *dev, unsigned int enabled)
+{
+	struct cmos_rtc	*cmos = dev_get_drvdata(dev);
+	unsigned long	flags;
+
+	if (!is_valid_irq(cmos->irq))
+		return -EINVAL;
+
+	spin_lock_irqsave(&rtc_lock, flags);
+
+	if (enabled)
+		cmos_irq_enable(cmos, RTC_UIE);
+	else
+		cmos_irq_disable(cmos, RTC_UIE);
+
+	spin_unlock_irqrestore(&rtc_lock, flags);
+	return 0;
+}
 
 #if defined(CONFIG_RTC_INTF_PROC) || defined(CONFIG_RTC_INTF_PROC_MODULE)
 
@@ -503,14 +497,15 @@ static int cmos_procfs(struct device *dev, struct seq_file *seq)
 #endif
 
 static const struct rtc_class_ops cmos_rtc_ops = {
-	.ioctl		= cmos_rtc_ioctl,
-	.read_time	= cmos_read_time,
-	.set_time	= cmos_set_time,
-	.read_alarm	= cmos_read_alarm,
-	.set_alarm	= cmos_set_alarm,
-	.proc		= cmos_procfs,
-	.irq_set_freq	= cmos_irq_set_freq,
-	.irq_set_state	= cmos_irq_set_state,
+	.read_time		= cmos_read_time,
+	.set_time		= cmos_set_time,
+	.read_alarm		= cmos_read_alarm,
+	.set_alarm		= cmos_set_alarm,
+	.proc			= cmos_procfs,
+	.irq_set_freq		= cmos_irq_set_freq,
+	.irq_set_state		= cmos_irq_set_state,
+	.alarm_irq_enable	= cmos_alarm_irq_enable,
+	.update_irq_enable	= cmos_update_irq_enable,
 };
 
 /*----------------------------------------------------------------*/
-- 
1.6.4.4

Still, couldn't an oops still trigger with this with dev_set_drvdata after
rtc_device_register? Lets say, a small test program like this:
#include <sys/types.h>
#include <sys/stat.h>
#include <sys/ioctl.h>
#include <fcntl.h>
#include <linux/rtc.h>

int main(int argc, char **argv)
{
        int fd;
        struct rtc_time rtc_tm;

        fd = open("/dev/rtc0", O_RDONLY);
        ioctl(fd, RTC_AIE_ON, &rtc_tm);
        ioctl(fd, RTC_UIE_ON, &rtc_tm);
        close(fd);
}
Compiled and run with similar udev rule. It's very unlikely, and I couldn't
get an oops anymore unless I moved dev_set_drvdata down in cmos_do_probe in
my test machine, but should be a possibility?

> 
> > But I saw another issue: looks it could be possible that as cmos_rtc_ioctl
> > (ioctl) can be run before rtc_device_register returns, the following call chain
> > could happen in current code:
> > cmos_rtc_ioctl->cmos_irq_{en,dis}able->cmos_checkintr->rtc_update_irq
> > rtc_update_irq uses cmos->rtc, which is set only at return of
> > rtc_device_register, and here we may have another problem... is it
> > possible?
> 
>  this shouldn't happen, irqs are enabled only after everything
>  has been setup to handle them.
> 

--
[]'s
Herton

Re: [rtc-linux] Re: rtc_cmos oops in cmos_rtc_ioctl

[Alessandro Zummo]

On Tue, 22 Sep 2009 15:15:26 -0300
Herton Ronaldo Krzesinski <herton@mandriva.com.br> wrote:

> 
> Something like this?:
> 
> From a94365843ab40a1904c4bc244af4e551f2f2aca9 Mon Sep 17 00:00:00 2001
> From: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
> Date: Tue, 22 Sep 2009 13:46:00 -0300
> Subject: [PATCH] rtc-cmos: convert RTC_AIE/RTC_UIE to rtc irq API
> 
> Drop ioctl function that handles RTC_AIE/RTC_UIE, and use instead the
> rtc subsystem API (alarm_irq_enable/update_irq_enable callbacks).
> 
> Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>

 Looks nice. David?

> --
> Still, couldn't an oops still trigger with this with dev_set_drvdata after
> rtc_device_register? Lets say, a small test program like this:

 dev_set_drvdata should be called before rtc_device_register. I guess
 I'll have to patch every single driver :(


 [...]

> Compiled and run with similar udev rule. It's very unlikely, and I couldn't
> get an oops anymore unless I moved dev_set_drvdata down in cmos_do_probe in
> my test machine, but should be a possibility?

 I don't think so.
 


-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it

Re: [rtc-linux] Re: rtc_cmos oops in cmos_rtc_ioctl

[Andrew Morton]

On Tue, 22 Sep 2009 21:15:46 +0200
Alessandro Zummo <alessandro.zummo@towertech.it> wrote:

> On Tue, 22 Sep 2009 15:15:26 -0300
> Herton Ronaldo Krzesinski <herton@mandriva.com.br> wrote:
> 
> > 
> > Something like this?:
> > 
> > From a94365843ab40a1904c4bc244af4e551f2f2aca9 Mon Sep 17 00:00:00 2001
> > From: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
> > Date: Tue, 22 Sep 2009 13:46:00 -0300
> > Subject: [PATCH] rtc-cmos: convert RTC_AIE/RTC_UIE to rtc irq API
> > 
> > Drop ioctl function that handles RTC_AIE/RTC_UIE, and use instead the
> > rtc subsystem API (alarm_irq_enable/update_irq_enable callbacks).
> > 
> > Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
> 
>  Looks nice. David?
> 

I couldn't bear the suspense, so in a spirit of brave curiosity, I
applied the patch!