annotating semaphores.

annotating semaphores.

[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 ?

	Dave

diff --git a/include/asm-x86/rwsem.h b/include/asm-x86/rwsem.h
index 041906f..3111853 100644
--- a/include/asm-x86/rwsem.h
+++ b/include/asm-x86/rwsem.h
@@ -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);
 }
 
 /*
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index c4e0016..cf110b0 100644
diff --git a/include/linux/rwsem-spinlock.h b/include/linux/rwsem-spinlock.h
index 813cee1..90a4b2b 100644
--- a/include/linux/rwsem-spinlock.h
+++ b/include/linux/rwsem-spinlock.h
@@ -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)
diff --git a/lib/rwsem-spinlock.c b/lib/rwsem-spinlock.c
index c4cfd6c..c13d112 100644
--- a/lib/rwsem-spinlock.c
+++ b/lib/rwsem-spinlock.c
@@ -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);
 }
 
 /*
-- 
http://www.codemonkey.org.uk

Re: annotating semaphores.

[Christopher Li]

On Dec 12, 2007 12:00 PM, Dave Jones <davej@redhat.com> wrote:
> 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?

> --- a/lib/rwsem-spinlock.c
> +++ b/lib/rwsem-spinlock.c
> @@ -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

Re: annotating semaphores.

[Dave Jones]

On Thu, Dec 13, 2007 at 02:00:09PM -0800, Christopher Li wrote:
 > On Dec 12, 2007 12:00 PM, Dave Jones <davej@redhat.com> wrote:
 > > 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

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 5e626b1..79581fa 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -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);
-- 
http://www.codemonkey.org.uk