[PATCH] Fix for rtc.c non-atomic access to rtc_status

From: Cesar Eduardo Barros (cesarb@web4u.com.br)
Date: Thu Apr 20 2000 - 08:47:01 EST

(not on the list, please CC: replies to me)

Using atomic_* seems to cause a false sense of security in some people.
atomic_set(...,atomic_read(...)) is not atomic, and rtc.c did it everywhere.
Also, testing for a bit and later enabling it (like in rtc_open) causes races
(two proccesses in SMP could open the rtc at the same time if they got the
right timing). I patched it to use a spinlock instead.

This is my first kernel patch, so I probably missed something (spinlocks are
tricky). Also, I only tested compiling it.

--- linux-2.3.99-pre5/drivers/char/rtc.c Fri Apr 14 14:31:08 2000
+++ linux-rtcfix/drivers/char/rtc.c Thu Apr 20 10:25:57 2000
@@ -124,7 +124,10 @@
 #define RTC_IS_OPEN 0x01 /* means /dev/rtc is in use */
 #define RTC_TIMER_ON 0x02 /* missed irq timer active */
 
-static atomic_t rtc_status = ATOMIC_INIT(0); /* bitmapped status byte. */
+/* Doing read-modify-write with atomic_set(...,atomic_read(...)) is evil */
+/* rtc_status_lock > rtc_lock */
+static spinlock_t rtc_status_lock = SPIN_LOCK_UNLOCKED;
+static unsigned int rtc_status = 0; /* bitmapped status byte. */
 static unsigned long rtc_freq = 0; /* Current periodic IRQ rate */
 static unsigned long rtc_irq_data = 0; /* our output to the world */
 
@@ -167,10 +170,13 @@
         wake_up_interruptible(&rtc_wait);
 
         if (rtc_async_queue)
+ /* Is this OK on interrupt? */
                 kill_fasync (rtc_async_queue, SIGIO, POLL_IN);
 
- if (atomic_read(&rtc_status) & RTC_TIMER_ON)
+ spin_lock (&rtc_status_lock);
+ if (rtc_status & RTC_TIMER_ON)
                 mod_timer(&rtc_irq_timer, jiffies + HZ/rtc_freq + 2*HZ/100);
+ spin_unlock (&rtc_status_lock);
 }
 #endif
 
@@ -245,11 +251,12 @@
         case RTC_PIE_OFF: /* Mask periodic int. enab. bit */
         {
                 mask_rtc_irq_bit(RTC_PIE);
- if (atomic_read(&rtc_status) & RTC_TIMER_ON) {
+ spin_lock_irqsave (&rtc_status_lock, flags);
+ if (rtc_status & RTC_TIMER_ON) {
                         del_timer(&rtc_irq_timer);
- atomic_set(&rtc_status,
- atomic_read(&rtc_status) & ~RTC_TIMER_ON);
+ rtc_status &= ~RTC_TIMER_ON;
                 }
+ spin_unlock_irqrestore (&rtc_status_lock, flags);
                 return 0;
         }
         case RTC_PIE_ON: /* Allow periodic ints */
@@ -262,13 +269,14 @@
                 if ((rtc_freq > 64) && (!capable(CAP_SYS_RESOURCE)))
                         return -EACCES;
 
- if (!(atomic_read(&rtc_status) & RTC_TIMER_ON)) {
- atomic_set(&rtc_status,
- atomic_read(&rtc_status) | RTC_TIMER_ON);
+ spin_lock_irqsave (&rtc_status_lock, flags);
+ if (!(rtc_status & RTC_TIMER_ON)) {
+ rtc_status |= RTC_TIMER_ON;
                         rtc_irq_timer.expires = jiffies + HZ/rtc_freq + 2*HZ/100;
                         add_timer(&rtc_irq_timer);
                 }
                 set_rtc_irq_bit(RTC_PIE);
+ spin_unlock_irqrestore (&rtc_status_lock, flags);
                 return 0;
         }
         case RTC_UIE_OFF: /* Mask ints from RTC updates. */
@@ -491,17 +499,23 @@
 {
         unsigned long flags;
 
- if(atomic_read(&rtc_status) & RTC_IS_OPEN)
- return -EBUSY;
+ spin_lock_irqsave (&rtc_status_lock, flags);
+ if(rtc_status & RTC_IS_OPEN)
+ goto out_busy;
 
         MOD_INC_USE_COUNT;
 
- atomic_set(&rtc_status, atomic_read(&rtc_status) | RTC_IS_OPEN);
+ rtc_status |= RTC_IS_OPEN;
 
- spin_lock_irqsave (&rtc_lock, flags);
+ spin_lock (&rtc_lock); /* Already in irqsave */
         rtc_irq_data = 0;
- spin_unlock_irqrestore (&rtc_lock, flags);
+ spin_unlock (&rtc_lock);
+ spin_unlock_irqrestore (&rtc_status_lock, flags);
         return 0;
+
+out_busy:
+ spin_unlock_irqrestore (&rtc_status_lock, flags);
+ return -EBUSY;
 }
 
 static int rtc_fasync (int fd, struct file *filp, int on)
@@ -514,24 +528,27 @@
 {
         unsigned long flags;
 #ifndef __alpha__
+ unsigned char tmp;
+#endif
+
+ spin_lock_irqsave (&rtc_status_lock, flags);
+#ifndef __alpha__
         /*
          * Turn off all interrupts once the device is no longer
          * in use, and clear the data.
          */
 
- unsigned char tmp;
-
- spin_lock_irqsave(&rtc_lock, flags);
+ spin_lock(&rtc_lock); /* Already in irqsave */
         tmp = CMOS_READ(RTC_CONTROL);
         tmp &= ~RTC_PIE;
         tmp &= ~RTC_AIE;
         tmp &= ~RTC_UIE;
         CMOS_WRITE(tmp, RTC_CONTROL);
         CMOS_READ(RTC_INTR_FLAGS);
- spin_unlock_irqrestore(&rtc_lock, flags);
+ spin_unlock(&rtc_lock);
 
- if (atomic_read(&rtc_status) & RTC_TIMER_ON) {
- atomic_set(&rtc_status, atomic_read(&rtc_status) & ~RTC_TIMER_ON);
+ if (rtc_status & RTC_TIMER_ON) {
+ rtc_status &= ~RTC_TIMER_ON;
                 del_timer(&rtc_irq_timer);
         }
 
@@ -542,10 +559,11 @@
 #endif
         MOD_DEC_USE_COUNT;
 
- spin_lock_irqsave (&rtc_lock, flags);
+ spin_lock (&rtc_lock);
         rtc_irq_data = 0;
- spin_unlock_irqrestore (&rtc_lock, flags);
- atomic_set(&rtc_status, atomic_read(&rtc_status) & ~RTC_IS_OPEN);
+ spin_unlock (&rtc_lock);
+ rtc_status &= ~RTC_IS_OPEN;
+ spin_unlock_irqrestore (&rtc_status_lock, flags);
         return 0;
 }
 
@@ -699,8 +717,13 @@
 {
         /* interrupts and maybe timer disabled at this point by rtc_release */
 
- if (atomic_read(&rtc_status) & RTC_TIMER_ON)
+ /* FIXME: how rtc_open here could be avoided? */
+ /* No need for locking, if we know for sure nothing else can open() us*/
+ spin_lock (&rtc_status_lock); /* No need for irqsave; interrupts are
+ disabled (I hope) */
+ if (rtc_status & RTC_TIMER_ON)
                 del_timer(&rtc_irq_timer);
+ spin_unlock (&rtc_status_lock);
 
         remove_proc_entry ("driver/rtc", NULL);
         misc_deregister(&rtc_dev); 

-- 
Cesar Eduardo Barros
cesarb@web4u.com.br
cesarb@dcc.ufrj.br

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/

This archive was generated by hypermail 2b29 : Sun Apr 23 2000 - 21:00:17 EST