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?
diff -Naur linux-2.3.99-pre9-2/drivers/char/nvram.c linux-2.3.99-pre9-2+nvramfix/drivers/char/nvram.c
--- linux-2.3.99-pre9-2/drivers/char/nvram.c Mon May 15 20:30:45 2000
+++ linux-2.3.99-pre9-2+nvramfix/drivers/char/nvram.c Fri May 19 18:37:40 2000
@@ -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",
--
Cesar Eduardo Barros
[email protected]
[email protected]