Skip to contentSkip to conversation

annotating semaphores.

PATCH Dave Jones 2 participants 3 messages Dec 12, 2007 – Dec 13, 2007

Subsystems (5)
Library code CPU frequency scaling framework Locking primitives Sparse checker +1 more subsystems
lore
Share & tools
lore

Dave Jones

author
#0 lore raw

Today I came across a bug in the kernel cpufreq code where we were missing a bunch of up_write() calls in error paths of a function.

I've been trying to get sparse's context checking to pick up on the errors and failing. The kernel patch below is what I have so far, but it seems to report no output whatsoever. What am I missing ?

Dave

include/asm-x86/rwsem.h +5 -0
index 041906f..3111853 100644
@@ -106,6 +106,7 @@ LOCK_PREFIX	"  incl      (%%eax)\n\t" /* adds 0x00000001, returns the old value
 		: "+m" (sem->count)
 		: "a" (sem)
 		: "memory", "cc");
+	__acquire(sem);
 }
 
 /*

@@ -128,6 +129,7 @@ LOCK_PREFIX	"  cmpxchgl  %2,%0\n\t"
 		: "+m" (sem->count), "=&a" (result), "=&r" (tmp)
 		: "i" (RWSEM_ACTIVE_READ_BIAS)
 		: "memory", "cc");
+	__acquire(sem);
 	return result>=0 ? 1 : 0;
 }
 

@@ -167,6 +169,7 @@ static inline int __down_write_trylock(struct rw_semaphore *sem)
 				  RWSEM_ACTIVE_WRITE_BIAS);
 	if (ret == RWSEM_UNLOCKED_VALUE)
 		return 1;
+	__acquire(sem);
 	return 0;
 }
 

@@ -186,6 +189,7 @@ LOCK_PREFIX	"  xadd      %%edx,(%%eax)\n\t" /* subtracts 1, returns the old valu
 		: "+m" (sem->count), "=d" (tmp)
 		: "a" (sem), "1" (tmp)
 		: "memory", "cc");
+	__release(sem);
 }
 
 /*

@@ -204,6 +208,7 @@ LOCK_PREFIX	"  xaddl     %%edx,(%%eax)\n\t" /* tries to transition 0xffff0001 ->
 		: "+m" (sem->count)
 		: "a" (sem), "i" (-RWSEM_ACTIVE_WRITE_BIAS)
 		: "memory", "cc", "edx");
+	__release(sem);
 }
 
 /*
include/linux/cpuidle.h +0 -0
index c4e0016..cf110b0 100644
include/linux/rwsem-spinlock.h +6 -6
index 813cee1..90a4b2b 100644
@@ -60,13 +60,13 @@ do {								\
 	__init_rwsem((sem), #sem, &__key);			\
 } while (0)
 
-extern void FASTCALL(__down_read(struct rw_semaphore *sem));
-extern int FASTCALL(__down_read_trylock(struct rw_semaphore *sem));
-extern void FASTCALL(__down_write(struct rw_semaphore *sem));
+extern void FASTCALL(__down_read(struct rw_semaphore *sem)) __acquires(sem);
+extern int FASTCALL(__down_read_trylock(struct rw_semaphore *sem)) __acquires(sem);
+extern void FASTCALL(__down_write(struct rw_semaphore *sem)) __acquires(sem);
 extern void FASTCALL(__down_write_nested(struct rw_semaphore *sem, int subclass));
-extern int FASTCALL(__down_write_trylock(struct rw_semaphore *sem));
-extern void FASTCALL(__up_read(struct rw_semaphore *sem));
-extern void FASTCALL(__up_write(struct rw_semaphore *sem));
+extern int FASTCALL(__down_write_trylock(struct rw_semaphore *sem) __acquires(sem));
+extern void FASTCALL(__up_read(struct rw_semaphore *sem)) __releases(sem);
+extern void FASTCALL(__up_write(struct rw_semaphore *sem)) __releases(sem);
 extern void FASTCALL(__downgrade_write(struct rw_semaphore *sem));
 
 static inline int rwsem_is_locked(struct rw_semaphore *sem)
lib/rwsem-spinlock.c +6 -0
index c4cfd6c..c13d112 100644
@@ -162,6 +162,7 @@ void fastcall __sched __down_read(struct rw_semaphore *sem)
 
 	tsk->state = TASK_RUNNING;
  out:
+	__acquire(sem);
 	;
 }
 

@@ -184,6 +185,7 @@ int fastcall __down_read_trylock(struct rw_semaphore *sem)
 
 	spin_unlock_irqrestore(&sem->wait_lock, flags);
 
+	__acquire(sem);
 	return ret;
 }
 

@@ -228,6 +230,7 @@ void fastcall __sched __down_write_nested(struct rw_semaphore *sem, int subclass
 
 	tsk->state = TASK_RUNNING;
  out:
+	__acquire(sem);
 	;
 }
 

@@ -253,6 +256,7 @@ int fastcall __down_write_trylock(struct rw_semaphore *sem)
 	}
 
 	spin_unlock_irqrestore(&sem->wait_lock, flags);
+	__acquire(sem);
 
 	return ret;
 }

@@ -270,6 +274,7 @@ void fastcall __up_read(struct rw_semaphore *sem)
 		sem = __rwsem_wake_one_writer(sem);
 
 	spin_unlock_irqrestore(&sem->wait_lock, flags);
+	__release(sem);
 }
 
 /*

@@ -286,6 +291,7 @@ void fastcall __up_write(struct rw_semaphore *sem)
 		sem = __rwsem_do_wake(sem, 1);
 
 	spin_unlock_irqrestore(&sem->wait_lock, flags);
+	__release(sem);
 }
 
 /*
Signature · 1 line
http://www.codemonkey.org.uk
maintainer
#1 lore raw
Quoted 8 lines · Dave Jones

Today I came across a bug in the kernel cpufreq code where we were missing a bunch of up_write() calls in error paths of a function.

I've been trying to get sparse's context checking to pick up on the errors and failing. The kernel patch below is what I have so far, but it seems to report no output whatsoever. What am I missing ?

Can you share the example buggy cpufreq code that miss the up_write() calls?

lib/rwsem-spinlock.c void fastcall __sched __down_read(struct rw_semaphore *sem) · line 167

Quoted 9 lines
lib/rwsem-spinlock.c +1 -0
@@ -162,6 +162,7 @@ void fastcall __sched __down_read(struct rw_semaphore *sem)
 
        tsk->state = TASK_RUNNING;
  out:
+       __acquire(sem);
        ;
 }

That is not needed. Because the __down_read is not an inline function. The extern void FASTCALL(__down_read(struct rw_semaphore *sem)) __acquires(sem); declare should be good enough.

Chris

author
#2 lore raw
Quoted 11 lines · Christopher Li
Quoted 8 lines · Dave Jones

Today I came across a bug in the kernel cpufreq code where we were missing a bunch of up_write() calls in error paths of a function.

I've been trying to get sparse's context checking to pick up on the errors and failing. The kernel patch below is what I have so far, but it seems to report no output whatsoever. What am I missing ?

Can you share the example buggy cpufreq code that miss the up_write() calls?

Sure. In drivers/cpufreq/cpufreq.c cpufreq_add_dev() is missing several calls to unlock_policy_rwsem_write() in the error paths.

The patch below should make it more obvious..

Dave

drivers/cpufreq/cpufreq.c +9 -3
index 5e626b1..79581fa 100644
@@ -841,19 +841,25 @@ static int cpufreq_add_dev (struct sys_device * sys_dev)
 	drv_attr = cpufreq_driver->attr;
 	while ((drv_attr) && (*drv_attr)) {
 		ret = sysfs_create_file(&policy->kobj, &((*drv_attr)->attr));
-		if (ret)
+		if (ret) {
+			unlock_policy_rwsem_write(cpu);
 			goto err_out_driver_exit;
+		}
 		drv_attr++;
 	}
 	if (cpufreq_driver->get){
 		ret = sysfs_create_file(&policy->kobj, &cpuinfo_cur_freq.attr);
-		if (ret)
+		if (ret) {
+			unlock_policy_rwsem_write(cpu);
 			goto err_out_driver_exit;
+		}
 	}
 	if (cpufreq_driver->target){
 		ret = sysfs_create_file(&policy->kobj, &scaling_cur_freq.attr);
-		if (ret)
+		if (ret) {
+			unlock_policy_rwsem_write(cpu);
 			goto err_out_driver_exit;
+		}
 	}
 
 	spin_lock_irqsave(&cpufreq_driver_lock, flags);
Signature · 1 line
http://www.codemonkey.org.uk