Skip to contentSkip to conversation

indydog driver race

PATCH Dave Hansen 2 participants 2 messages Mar 26, 2002 – Mar 27, 2002 Char and misc drivers

lore
Share & tools
lore

Dave Hansen

author
#0 lore raw

indydog_open has a race

indydog_open(struct inode *inode, struct file *file)

{
        if( indydog_alive )
		return -EBUSY;	
	...
	 indydog_alive = 1;
        return 0;
}

If 2 opens happen simultaneously, there can be 2 tasks which both see indydog_alive as 0 and both set it to 1, successfully opening the device. Block and char devices hold the BKL before calling the driver's open function, but misc devices don't do the same.

The BKL is not needed in the release function, as far as I can see. Alan's softdog.c driver uses atomic ops without BKL to do the same thing as your driver. Patch to fix attached.

Signature · 2 lines
Dave Hansen
[email protected]
drivers/char/indydog.c +3 -6
@@ -24,7 +24,7 @@
 #include <asm/uaccess.h>
 #include <asm/sgi/sgimc.h>
 
-static int indydog_alive;
+static unsigned long indydog_alive;
 static struct sgimc_misc_ctrl *mcmisc_regs; 
 
 static void indydog_ping()

@@ -41,7 +41,7 @@
 {
 	u32 mc_ctrl0;
 	
-	if(indydog_alive)
+	if( test_and_set_bit(0,&indydog_alive) )
 		return -EBUSY;
 #ifdef CONFIG_WATCHDOG_NOWAYOUT
 	MOD_INC_USE_COUNT;

@@ -55,7 +55,6 @@
 	mcmisc_regs->cpuctrl0 = mc_ctrl0;
 	indydog_ping();
 			
-	indydog_alive=1;
 	printk("Started watchdog timer.\n");
 	return 0;
 }

@@ -66,7 +65,6 @@
 	 *	Shut off the timer.
 	 * 	Lock it in if it's a module and we defined ...NOWAYOUT
 	 */
-	 lock_kernel();
 #ifndef CONFIG_WATCHDOG_NOWAYOUT
 	{
 	u32 mc_ctrl0 = mcmisc_regs->cpuctrl0; 

@@ -75,8 +73,7 @@
 	printk("Stopped watchdog timer.\n");
 	}
 #endif
-	indydog_alive=0;
-	unlock_kernel();
+	clear_bit(0,&indydog_alive);
 	return 0;
 }
#1 lore raw
Quoted 11 lines · Dave Hansen

indydog_open has a race

indydog_open(struct inode *inode, struct file *file)

{
         if( indydog_alive )
		return -EBUSY;	
	...
	 indydog_alive = 1;
         return 0;
}
Quoted 4 lines

If 2 opens happen simultaneously, there can be 2 tasks which both see indydog_alive as 0 and both set it to 1, successfully opening the device. Block and char devices hold the BKL before calling the driver's open function, but misc devices don't do the same.

As Dave and me discussed in private mail this can't actually happen on non-preemtible kernels since IP22 is non-SMP(which is why I didn't bother when I wrote it). Anyway, a race is a race, no excuses.

Quoted 4 lines

The BKL is not needed in the release function, as far as I can see. Alan's softdog.c driver uses atomic ops without BKL to do the same thing as your driver. Patch to fix attached.

Tested and works on Mips/IP22. Thanks again Dave,
-- Guido