Re: [PATCH 1/2] irq_flags_t: intro and core annotations

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Oct 21, 2007 at 01:54:18AM +0100, Al Viro wrote:
> On Sun, Oct 21, 2007 at 03:55:46AM +0400, Alexey Dobriyan wrote:
> > * irq_flags_t is marked with __bitwise__ which means sparse(1) will warn
> >   developer when he accidently uses wrong type or totally wrong variable.
> > * irq_flags_t allows conversion to struct instead of typedef without flag day.
> >   This will give compile-time breakage of buggy users later.
> > * irq_flags_t allows arch maintainers to eventually switch to something
> >   smaller than "unsigned long" if they want to.
> 
> > P.S.: Anyone checking for differences in sparse logs -- don't panic,
> > 	just remove __bitwise__ .
> 
> Umm...  Could you make that conditional on something, so that it could
> be done with e.g. -D__CHECK_IRQFLAGS__?  We could make that unconditional
> closer to the end of conversion.

OK! Resending patch #1, patch #2 stays the same.



[PATCH 1/2] irq_flags_t: intro and core annotations

One type of bugs steadily being fought is the following one:

	unsigned int flags;
	spin_lock_irqsave(&lock, flags);

where "flags" should be "unsigned long". Here is far from complete list of
commits fixing such bugs:

099575b6cb7eaf18211ba72de56264f67651b90b
5efee174f8a101cafb1607d2b629bed353701457
c53421b18f205c5f97c604ae55c6a921f034b0f6	(many)
ca7e235f5eb960d83b45cef4384b490672538cd9
361f6ed1d00f666a1a7c33f3e9aaccb713f9b9e4

So far remedies were:
a) grep(1) -- obviously fragile. I tried at some point grepping for
   spin_lock_irqsave(), found quite a few, but it became booooring quickly.
b) BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long)) -- was tried,
   brutally broke some arches, survived one commit before revert :^)
   Doesn't work on i386 where sizeof(unsigned int) == sizeof(unsigned long).

So it would be nice to have something more robust.



			irq_flags_t
	
New type for use with spin_lock_irqsave() and friends.

* irq_flags_t is unsigned long which shouldn't change any code 
  (except broken one)
* irq_flags_t is marked with __bitwise__ which means sparse(1) will warn
  developer when he accidently uses wrong type or totally wrong variable.
* irq_flags_t allows conversion to struct instead of typedef without flag day.
  This will give compile-time breakage of buggy users later.
* irq_flags_t allows arch maintainers to eventually switch to something
  smaller than "unsigned long" if they want to.

Typical code after conversion:

	irq_flags_t flags;

	spin_lock_irqsave(&lock, flags);
	[stuff]
	spin_unlock_irqrestore(&lock, flags);

This patch adds type itself, annotates core locking functions in generic
headers and i386 arch. Warnings become visible by passing __CHECK_IRQFLAGS__
to sparse:

	make C=2 CF="-D__CHECK_IRQFLAGS__"

Signed-off-by: Alexey Dobriyan <[email protected]>
---

 include/asm-x86/irqflags_32.h    |   20 ++++++++++----------
 include/asm-x86/paravirt.h       |   14 +++++++-------
 include/linux/interrupt.h        |   10 +++++-----
 include/linux/irqflags.h         |    2 +-
 include/linux/spinlock_api_smp.h |   14 +++++++-------
 include/linux/spinlock_up.h      |    2 +-
 include/linux/types.h            |    5 +++++
 kernel/spinlock.c                |   24 ++++++++++++------------
 8 files changed, 48 insertions(+), 43 deletions(-)

--- a/include/asm-x86/irqflags_32.h
+++ b/include/asm-x86/irqflags_32.h
@@ -12,14 +12,14 @@
 #include <asm/processor-flags.h>
 
 #ifndef __ASSEMBLY__
-static inline unsigned long native_save_fl(void)
+static inline irq_flags_t native_save_fl(void)
 {
-	unsigned long f;
+	irq_flags_t f;
 	asm volatile("pushfl ; popl %0":"=g" (f): /* no input */);
 	return f;
 }
 
-static inline void native_restore_fl(unsigned long f)
+static inline void native_restore_fl(irq_flags_t f)
 {
 	asm volatile("pushl %0 ; popfl": /* no output */
 			     :"g" (f)
@@ -52,12 +52,12 @@ static inline void native_halt(void)
 #else
 #ifndef __ASSEMBLY__
 
-static inline unsigned long __raw_local_save_flags(void)
+static inline irq_flags_t __raw_local_save_flags(void)
 {
 	return native_save_fl();
 }
 
-static inline void raw_local_irq_restore(unsigned long flags)
+static inline void raw_local_irq_restore(irq_flags_t flags)
 {
 	native_restore_fl(flags);
 }
@@ -93,9 +93,9 @@ static inline void halt(void)
 /*
  * For spinlocks, etc:
  */
-static inline unsigned long __raw_local_irq_save(void)
+static inline irq_flags_t __raw_local_irq_save(void)
 {
-	unsigned long flags = __raw_local_save_flags();
+	irq_flags_t flags = __raw_local_save_flags();
 
 	raw_local_irq_disable();
 
@@ -118,14 +118,14 @@ static inline unsigned long __raw_local_irq_save(void)
 #define raw_local_irq_save(flags) \
 		do { (flags) = __raw_local_irq_save(); } while (0)
 
-static inline int raw_irqs_disabled_flags(unsigned long flags)
+static inline int raw_irqs_disabled_flags(irq_flags_t flags)
 {
-	return !(flags & X86_EFLAGS_IF);
+	return !((unsigned long __force)flags & X86_EFLAGS_IF);
 }
 
 static inline int raw_irqs_disabled(void)
 {
-	unsigned long flags = __raw_local_save_flags();
+	irq_flags_t flags = __raw_local_save_flags();
 
 	return raw_irqs_disabled_flags(flags);
 }
--- a/include/asm-x86/paravirt.h
+++ b/include/asm-x86/paravirt.h
@@ -136,8 +136,8 @@ struct pv_irq_ops {
 	 * returned from save_fl are undefined, and may be ignored by
 	 * restore_fl.
 	 */
-	unsigned long (*save_fl)(void);
-	void (*restore_fl)(unsigned long);
+	irq_flags_t (*save_fl)(void);
+	void (*restore_fl)(irq_flags_t);
 	void (*irq_disable)(void);
 	void (*irq_enable)(void);
 	void (*safe_halt)(void);
@@ -1014,9 +1014,9 @@ struct paravirt_patch_site {
 extern struct paravirt_patch_site __parainstructions[],
 	__parainstructions_end[];
 
-static inline unsigned long __raw_local_save_flags(void)
+static inline irq_flags_t __raw_local_save_flags(void)
 {
-	unsigned long f;
+	irq_flags_t f;
 
 	asm volatile(paravirt_alt("pushl %%ecx; pushl %%edx;"
 				  PARAVIRT_CALL
@@ -1028,7 +1028,7 @@ static inline unsigned long __raw_local_save_flags(void)
 	return f;
 }
 
-static inline void raw_local_irq_restore(unsigned long f)
+static inline void raw_local_irq_restore(irq_flags_t f)
 {
 	asm volatile(paravirt_alt("pushl %%ecx; pushl %%edx;"
 				  PARAVIRT_CALL
@@ -1062,9 +1062,9 @@ static inline void raw_local_irq_enable(void)
 		     : "memory", "eax", "cc");
 }
 
-static inline unsigned long __raw_local_irq_save(void)
+static inline irq_flags_t __raw_local_irq_save(void)
 {
-	unsigned long f;
+	irq_flags_t f;
 
 	f = __raw_local_save_flags();
 	raw_local_irq_disable();
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -122,7 +122,7 @@ static inline void disable_irq_nosync_lockdep(unsigned int irq)
 #endif
 }
 
-static inline void disable_irq_nosync_lockdep_irqsave(unsigned int irq, unsigned long *flags)
+static inline void disable_irq_nosync_lockdep_irqsave(unsigned int irq, irq_flags_t *flags)
 {
 	disable_irq_nosync(irq);
 #ifdef CONFIG_LOCKDEP
@@ -146,7 +146,7 @@ static inline void enable_irq_lockdep(unsigned int irq)
 	enable_irq(irq);
 }
 
-static inline void enable_irq_lockdep_irqrestore(unsigned int irq, unsigned long *flags)
+static inline void enable_irq_lockdep_irqrestore(unsigned int irq, irq_flags_t *flags)
 {
 #ifdef CONFIG_LOCKDEP
 	local_irq_restore(*flags);
@@ -211,17 +211,17 @@ static inline void __deprecated sti(void)
 {
 	local_irq_enable();
 }
-static inline void __deprecated save_flags(unsigned long *x)
+static inline void __deprecated save_flags(irq_flags_t *x)
 {
 	local_save_flags(*x);
 }
 #define save_flags(x) save_flags(&x)
-static inline void __deprecated restore_flags(unsigned long x)
+static inline void __deprecated restore_flags(irq_flags_t x)
 {
 	local_irq_restore(x);
 }
 
-static inline void __deprecated save_and_cli(unsigned long *x)
+static inline void __deprecated save_and_cli(irq_flags_t *x)
 {
 	local_irq_save(*x);
 }
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -84,7 +84,7 @@
 
 #define irqs_disabled()						\
 ({								\
-	unsigned long flags;					\
+	irq_flags_t flags;					\
 								\
 	raw_local_save_flags(flags);				\
 	raw_irqs_disabled_flags(flags);				\
--- a/include/linux/spinlock_api_smp.h
+++ b/include/linux/spinlock_api_smp.h
@@ -30,13 +30,13 @@ void __lockfunc _write_lock_bh(rwlock_t *lock)		__acquires(lock);
 void __lockfunc _spin_lock_irq(spinlock_t *lock)	__acquires(lock);
 void __lockfunc _read_lock_irq(rwlock_t *lock)		__acquires(lock);
 void __lockfunc _write_lock_irq(rwlock_t *lock)		__acquires(lock);
-unsigned long __lockfunc _spin_lock_irqsave(spinlock_t *lock)
+irq_flags_t __lockfunc _spin_lock_irqsave(spinlock_t *lock)
 							__acquires(lock);
-unsigned long __lockfunc _spin_lock_irqsave_nested(spinlock_t *lock, int subclass)
+irq_flags_t __lockfunc _spin_lock_irqsave_nested(spinlock_t *lock, int subclass)
 							__acquires(lock);
-unsigned long __lockfunc _read_lock_irqsave(rwlock_t *lock)
+irq_flags_t __lockfunc _read_lock_irqsave(rwlock_t *lock)
 							__acquires(lock);
-unsigned long __lockfunc _write_lock_irqsave(rwlock_t *lock)
+irq_flags_t __lockfunc _write_lock_irqsave(rwlock_t *lock)
 							__acquires(lock);
 int __lockfunc _spin_trylock(spinlock_t *lock);
 int __lockfunc _read_trylock(rwlock_t *lock);
@@ -51,11 +51,11 @@ void __lockfunc _write_unlock_bh(rwlock_t *lock)	__releases(lock);
 void __lockfunc _spin_unlock_irq(spinlock_t *lock)	__releases(lock);
 void __lockfunc _read_unlock_irq(rwlock_t *lock)	__releases(lock);
 void __lockfunc _write_unlock_irq(rwlock_t *lock)	__releases(lock);
-void __lockfunc _spin_unlock_irqrestore(spinlock_t *lock, unsigned long flags)
+void __lockfunc _spin_unlock_irqrestore(spinlock_t *lock, irq_flags_t flags)
 							__releases(lock);
-void __lockfunc _read_unlock_irqrestore(rwlock_t *lock, unsigned long flags)
+void __lockfunc _read_unlock_irqrestore(rwlock_t *lock, irq_flags_t flags)
 							__releases(lock);
-void __lockfunc _write_unlock_irqrestore(rwlock_t *lock, unsigned long flags)
+void __lockfunc _write_unlock_irqrestore(rwlock_t *lock, irq_flags_t flags)
 							__releases(lock);
 
 #endif /* __LINUX_SPINLOCK_API_SMP_H */
--- a/include/linux/spinlock_up.h
+++ b/include/linux/spinlock_up.h
@@ -26,7 +26,7 @@ static inline void __raw_spin_lock(raw_spinlock_t *lock)
 }
 
 static inline void
-__raw_spin_lock_flags(raw_spinlock_t *lock, unsigned long flags)
+__raw_spin_lock_flags(raw_spinlock_t *lock, irq_flags_t flags)
 {
 	local_irq_save(flags);
 	lock->slock = 0;
--- a/include/linux/types.h
+++ b/include/linux/types.h
@@ -188,6 +188,11 @@ typedef __u32 __bitwise __wsum;
 
 #ifdef __KERNEL__
 typedef unsigned __bitwise__ gfp_t;
+#ifdef __CHECK_IRQFLAGS__
+typedef unsigned long __bitwise__ irq_flags_t;
+#else
+typedef unsigned long irq_flags_t;
+#endif
 
 #ifdef CONFIG_RESOURCES_64BIT
 typedef u64 resource_size_t;
--- a/kernel/spinlock.c
+++ b/kernel/spinlock.c
@@ -76,9 +76,9 @@ void __lockfunc _read_lock(rwlock_t *lock)
 }
 EXPORT_SYMBOL(_read_lock);
 
-unsigned long __lockfunc _spin_lock_irqsave(spinlock_t *lock)
+irq_flags_t __lockfunc _spin_lock_irqsave(spinlock_t *lock)
 {
-	unsigned long flags;
+	irq_flags_t flags;
 
 	local_irq_save(flags);
 	preempt_disable();
@@ -115,9 +115,9 @@ void __lockfunc _spin_lock_bh(spinlock_t *lock)
 }
 EXPORT_SYMBOL(_spin_lock_bh);
 
-unsigned long __lockfunc _read_lock_irqsave(rwlock_t *lock)
+irq_flags_t __lockfunc _read_lock_irqsave(rwlock_t *lock)
 {
-	unsigned long flags;
+	irq_flags_t flags;
 
 	local_irq_save(flags);
 	preempt_disable();
@@ -145,9 +145,9 @@ void __lockfunc _read_lock_bh(rwlock_t *lock)
 }
 EXPORT_SYMBOL(_read_lock_bh);
 
-unsigned long __lockfunc _write_lock_irqsave(rwlock_t *lock)
+irq_flags_t __lockfunc _write_lock_irqsave(rwlock_t *lock)
 {
-	unsigned long flags;
+	irq_flags_t flags;
 
 	local_irq_save(flags);
 	preempt_disable();
@@ -222,9 +222,9 @@ void __lockfunc _##op##_lock(locktype##_t *lock)			\
 									\
 EXPORT_SYMBOL(_##op##_lock);						\
 									\
-unsigned long __lockfunc _##op##_lock_irqsave(locktype##_t *lock)	\
+irq_flags_t __lockfunc _##op##_lock_irqsave(locktype##_t *lock)		\
 {									\
-	unsigned long flags;						\
+	irq_flags_t flags;						\
 									\
 	for (;;) {							\
 		preempt_disable();					\
@@ -254,7 +254,7 @@ EXPORT_SYMBOL(_##op##_lock_irq);					\
 									\
 void __lockfunc _##op##_lock_bh(locktype##_t *lock)			\
 {									\
-	unsigned long flags;						\
+	irq_flags_t flags;						\
 									\
 	/*							*/	\
 	/* Careful: we must exclude softirqs too, hence the	*/	\
@@ -341,7 +341,7 @@ void __lockfunc _read_unlock(rwlock_t *lock)
 }
 EXPORT_SYMBOL(_read_unlock);
 
-void __lockfunc _spin_unlock_irqrestore(spinlock_t *lock, unsigned long flags)
+void __lockfunc _spin_unlock_irqrestore(spinlock_t *lock, irq_flags_t flags)
 {
 	spin_release(&lock->dep_map, 1, _RET_IP_);
 	_raw_spin_unlock(lock);
@@ -368,7 +368,7 @@ void __lockfunc _spin_unlock_bh(spinlock_t *lock)
 }
 EXPORT_SYMBOL(_spin_unlock_bh);
 
-void __lockfunc _read_unlock_irqrestore(rwlock_t *lock, unsigned long flags)
+void __lockfunc _read_unlock_irqrestore(rwlock_t *lock, irq_flags_t flags)
 {
 	rwlock_release(&lock->dep_map, 1, _RET_IP_);
 	_raw_read_unlock(lock);
@@ -395,7 +395,7 @@ void __lockfunc _read_unlock_bh(rwlock_t *lock)
 }
 EXPORT_SYMBOL(_read_unlock_bh);
 
-void __lockfunc _write_unlock_irqrestore(rwlock_t *lock, unsigned long flags)
+void __lockfunc _write_unlock_irqrestore(rwlock_t *lock, irq_flags_t flags)
 {
 	rwlock_release(&lock->dep_map, 1, _RET_IP_);
 	_raw_write_unlock(lock);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux