nvram.c fixes
PATCH 1/1 nvram.c fixes
This patch fixes the following issues in nvram.c:
1. Everything in nvram.c would race with rtc.c (and maybe other code) in other CPU (SMP) called from userspace (like rtc_ioctl)
2. nvram_read and nvram_write didn't return -EFAULT when they should
To fix the second issue I had to move the copy from/to userspace to outside the critical region.
This patch doesn't fix the following issues:
1. nvram_{{read,write}_byte,{check,set}_checksum} are atomic, but they are probably used in a batch of three or four operations, and the whole batch should be atomic wrt other rtc operations (rtc_lock). This issue is worse if nvram_write is used and someone tries to read with nvram_read (the checksum is bad until nvram_set_checksum is used).
2. nvram_{{read,write}_byte,{check,set}_checksum} and a _nolock version of them all should be moved to a header file somewhere. They're trivially inlined and are very small.
3. Having every file in the whole kernel which uses rtc_lock define it as an extern is silly. It should be placed in a header somewhere.
4. The indentation is awful in some parts of nvram.c.
This has not been tested or even compiled, and something is probably wrong. Comments?
drivers/char/nvram.c +60 -63
@@ -81,7 +81,7 @@#endif /* Note that *all* calls to CMOS_READ and CMOS_WRITE must be done with -* interrupts disabled. Due to the index-port/data-port design of the RTC, we +* rtc_lock held. Due to the index-port/data-port design of the RTC, we * don't want two different things trying to get to it at once. (e.g. the * periodic 11 min sync from time.c vs. this driver.) */
@@ -96,11 +96,13 @@#include <linux/nvram.h> #include <linux/init.h> #include <linux/proc_fs.h> +#include <linux/spinlock.h> #include <asm/io.h> #include <asm/uaccess.h> #include <asm/system.h> +extern spinlock_t rtc_lock;static int nvram_open_cnt =0 ;/* #times opened */ static int nvram_open_mode;/* special open modes */
@@ -163,21 +165,20 @@unsigned long flags;unsigned char c; -save_flags (flags); -cli (); +spin_lock_irqsave (&rtc_lock, flags); c =nvram_read_int ( i ); -restore_flags (flags); +spin_unlock_irqrestore (&rtc_lock, flags);return ( c ); } +/* This races nicely with trying to read with checksum checking (nvram_read) */ void nvram_write_byte (unsigned char c,int i ) {unsigned long flags; -save_flags (flags); -cli (); +spin_lock_irqsave (&rtc_lock, flags);nvram_write_int ( c, i ); -restore_flags (flags); +spin_unlock_irqrestore (&rtc_lock, flags); }int nvram_check_checksum (void )
@@ -185,10 +186,9 @@unsigned long flags;int rv; -save_flags (flags); -cli (); +spin_lock_irqsave (&rtc_lock, flags); rv =nvram_check_checksum_int (); -restore_flags (flags); +spin_unlock_irqrestore (&rtc_lock, flags);return ( rv ); }
@@ -196,10 +196,9 @@ {unsigned long flags; -save_flags (flags); -cli (); +spin_lock_irqsave (&rtc_lock, flags);nvram_set_checksum_int (); -restore_flags (flags); +spin_unlock_irqrestore (&rtc_lock, flags); }#endif /* MACH == ATARI */
@@ -228,63 +227,67 @@static ssize_t nvram_read (struct file * file,char * buf,size_t count,loff_t *ppos ) { -unsigned long flags; +char * contents [NVRAM_BYTES ];unsigned i = *ppos; -char *tmp = buf; - -if (i != *ppos) -return -EINVAL ; +char *tmp; -save_flags (flags); -cli (); +spin_lock_irq (&rtc_lock); -if (!nvram_check_checksum_int ()) { -restore_flags (flags); -return ( -EIO ); - } +if (!nvram_check_checksum_int ()) + goto checksum_err; + +for (tmp = contents; count-- >0 && i <NVRAM_BYTES ; ++i, ++tmp) + *tmp =nvram_read_int (i); + +spin_unlock_irq (&rtc_lock); + +copy_to_user_ret (buf, contents, tmp - contents, -EFAULT ); -for ( ; count-- >0 && i <NVRAM_BYTES ; ++i, ++tmp ) -put_user (nvram_read_int (i), tmp ); *ppos = i; -restore_flags (flags); -return ( tmp - buf ); +return (tmp - contents); + +checksum_err: +spin_unlock_irq (&rtc_lock); +return -EIO ; }static ssize_t nvram_write (struct file * file,const char * buf,size_t count,loff_t *ppos ) { -unsigned long flags; +char * contents [NVRAM_BYTES ];unsigned i = *ppos; -const char *tmp = buf;char c; - -if (i != *ppos) -return -EINVAL ; -save_flags (flags); -cli (); - -if (!nvram_check_checksum_int ()) { -restore_flags (flags); -return ( -EIO ); - } +/* could comebody please help me indent this better? */ +copy_from_user_ret (contents, buf, (NVRAM_BYTES - i) < count ? + (NVRAM_BYTES - i) : count, + -EFAULT ); + +spin_lock_irq (&rtc_lock); + +if (!nvram_check_checksum_int ()) + goto checksum_err; + +for (tmp = contents; count-- >0 && i <NVRAM_BYTES ; ++i, ++tmp) +nvram_write_int (*tmp, i); -for ( ; count-- >0 && i <NVRAM_BYTES ; ++i, ++tmp ) { -get_user ( c, tmp ); -nvram_write_int ( c, i ); - }nvram_set_checksum_int (); + +spin_unlock_irq (&rtc_lock); + *ppos = i; -restore_flags (flags); -return ( tmp - buf ); +return (tmp - contents); + +checksum_err: +spin_unlock_irq (&rtc_lock); +return -EIO ; }static int nvram_ioctl (struct inode *inode,struct file *file,unsigned int cmd,unsigned long arg ) { -unsigned long flags;int i;switch ( cmd ) {
@@ -293,14 +296,13 @@if (!capable (CAP_SYS_ADMIN ))return ( -EACCES ); -save_flags (flags); -cli (); +spin_lock_irq (&rtc_lock);for ( i =0 ; i <NVRAM_BYTES ; ++i )nvram_write_int (0 , i );nvram_set_checksum_int (); -restore_flags (flags); +spin_unlock_irq (&rtc_lock);return (0 );case NVRAM_SETCKS :/* just set checksum, contents unchanged
@@ -309,10 +311,9 @@if (!capable (CAP_SYS_ADMIN ))return ( -EACCES ); -save_flags (flags); -cli (); +spin_lock_irq (&rtc_lock);nvram_set_checksum_int (); -restore_flags (flags); +spin_unlock_irq (&rtc_lock);return (0 );default :
@@ -357,16 +358,14 @@static int nvram_read_proc (char *buffer,char **start,off_t offset,int size,int *eof,void *data ) { -unsigned long flags;unsigned char contents[NVRAM_BYTES ];int i, len =0 ;off_t begin =0 ; - -save_flags (flags); -cli (); + +spin_lock_irq (&rtc_lock);for ( i =0 ; i <NVRAM_BYTES ; ++i ) contents[i] =nvram_read_int ( i ); -restore_flags (flags); +spin_unlock_irq (&rtc_lock); *eof =mach_proc_infos ( contents, buffer, &len, &begin, offset, size );
@@ -476,15 +475,13 @@static int pc_proc_infos (unsigned char *nvram,char *buffer,int *len,off_t *begin,off_t offset,int size ) { -unsigned long flags;int checksum;int type; -save_flags (flags); -cli (); +spin_lock_irq (&rtc_lock); checksum =nvram_check_checksum_int (); -restore_flags (flags); - +spin_unlock_irq (&rtc_lock); +PRINT_PROC ("Checksum status: %svalid\n" , checksum ?"" :"not " ); PRINT_PROC( "# floppies : %d\n",
Signature · 8 lines
Cesar Eduardo Barros [email protected] [email protected] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] Please read the FAQ at http://www.tux.org/lkml/