Re: [PATCH] add typecheck on irqsave and friends for correct flags

[Alexey Dobriyan]

On Thu, May 01, 2008 at 07:51:18PM -0400, Steven Rostedt wrote:
> There has been several areas in the kernel where an int has been used
> for flags in local_irq_save and friends instead of a long. This can
> cause some hard to debug problems on some architectures.
> 
> This patch adds a typecheck inside the irqsave and restore functions
> to flag these cases.

OK, at least something that uncovers such bugs.
Same checks in include/asm-frv/system.h should be removed then.

> --- linux-compile.git.orig/include/linux/irqflags.h
> +++ linux-compile.git/include/linux/irqflags.h
> @@ -49,18 +49,24 @@
>  	do { trace_hardirqs_on(); raw_local_irq_enable(); } while (0)
>  #define local_irq_disable() \
>  	do { raw_local_irq_disable(); trace_hardirqs_off(); } while (0)
> -#define local_irq_save(flags) \
> -	do { raw_local_irq_save(flags); trace_hardirqs_off(); } while (0)
> +#define local_irq_save(flags)				\
> +	do {						\
> +		typecheck(unsigned long, flags);	\
> +		raw_local_irq_save(flags);		\
> +		trace_hardirqs_off();			\
> +	} while (0)
> 
> -#define local_irq_restore(flags)				\
> -	do {							\
> -		if (raw_irqs_disabled_flags(flags)) {		\
> -			raw_local_irq_restore(flags);		\
> -			trace_hardirqs_off();			\
> -		} else {					\
> -			trace_hardirqs_on();			\
> -			raw_local_irq_restore(flags);		\
> -		}						\
> +
> +#define local_irq_restore(flags)			\
> +	do {						\
> +		typecheck(unsigned long, flags);	\
> +		if (raw_irqs_disabled_flags(flags)) {	\
> +			raw_local_irq_restore(flags);	\
> +			trace_hardirqs_off();		\
> +		} else {				\
> +			trace_hardirqs_on();		\
> +			raw_local_irq_restore(flags);	\
> +		}					\
>  	} while (0)
>  #else /* !CONFIG_TRACE_IRQFLAGS_SUPPORT */
>  /*
> @@ -69,8 +75,16 @@
>   */
>  # define raw_local_irq_disable()	local_irq_disable()
>  # define raw_local_irq_enable()		local_irq_enable()
> -# define raw_local_irq_save(flags)	local_irq_save(flags)
> -# define raw_local_irq_restore(flags)	local_irq_restore(flags)
> +# define raw_local_irq_save(flags)			\
> +	do {						\
> +		typecheck(unsigned long, flags);	\
> +		local_irq_save(flags);			\
> +	} while (0)
> +# define raw_local_irq_restore(flags)			\
> +	do {						\
> +		typecheck(unsigned long, flags);	\
> +		local_irq_restore(flags);		\
> +	} while (0)
>  #endif /* CONFIG_TRACE_IRQFLAGS_SUPPORT */
> 
>  #ifdef CONFIG_TRACE_IRQFLAGS_SUPPORT
> @@ -80,7 +94,11 @@
>  		raw_safe_halt();				\
>  	} while (0)
> 
> -#define local_save_flags(flags)		raw_local_save_flags(flags)
> +#define local_save_flags(flags)				\
> +	do {						\
> +		typecheck(unsigned long, flags);	\
> +		raw_local_save_flags(flags);		\
> +	} while(0)
> 
>  #define irqs_disabled()						\
>  ({								\
> @@ -90,7 +108,11 @@
>  	raw_irqs_disabled_flags(_flags);			\
>  })
> 
> -#define irqs_disabled_flags(flags)	raw_irqs_disabled_flags(flags)
> +#define irqs_disabled_flags(flags)		\
> +({						\
> +	typecheck(unsigned long, flags);	\
> +	raw_irqs_disabled_flags(flags);		\
> +})
>  #endif		/* CONFIG_X86 */
> 
>  #endif
> Index: linux-compile.git/include/linux/spinlock.h
> ===================================================================
> --- linux-compile.git.orig/include/linux/spinlock.h	2008-05-01 16:58:58.000000000 -0400
> +++ linux-compile.git/include/linux/spinlock.h	2008-05-01 19:16:21.000000000 -0400
> @@ -191,23 +191,53 @@ do {								\
> 
>  #if defined(CONFIG_SMP) || defined(CONFIG_DEBUG_SPINLOCK)
> 
> -#define spin_lock_irqsave(lock, flags)	flags = _spin_lock_irqsave(lock)
> -#define read_lock_irqsave(lock, flags)	flags = _read_lock_irqsave(lock)
> -#define write_lock_irqsave(lock, flags)	flags = _write_lock_irqsave(lock)
> +#define spin_lock_irqsave(lock, flags)			\
> +	do {						\
> +		typecheck(unsigned long, flags);	\
> +		flags = _spin_lock_irqsave(lock);	\
> +	} while (0)
> +#define read_lock_irqsave(lock, flags)			\
> +	do {						\
> +		typecheck(unsigned long, flags);	\
> +		flags = _read_lock_irqsave(lock);	\
> +	} while (0)
> +#define write_lock_irqsave(lock, flags)			\
> +	do {						\
> +		typecheck(unsigned long, flags);	\
> +		flags = _write_lock_irqsave(lock);	\
> +	} while (0)
> 
>  #ifdef CONFIG_DEBUG_LOCK_ALLOC
> -#define spin_lock_irqsave_nested(lock, flags, subclass) \
> -	flags = _spin_lock_irqsave_nested(lock, subclass)
> +#define spin_lock_irqsave_nested(lock, flags, subclass)			\
> +	do {								\
> +		typecheck(unsigned long, flags);			\
> +		flags = _spin_lock_irqsave_nested(lock, subclass);	\
> +	} while (0)
>  #else
> -#define spin_lock_irqsave_nested(lock, flags, subclass) \
> -	flags = _spin_lock_irqsave(lock)
> +#define spin_lock_irqsave_nested(lock, flags, subclass)			\
> +	do {								\
> +		typecheck(unsigned long, flags);			\
> +		flags = _spin_lock_irqsave(lock);			\
> +	} while (0)
>  #endif
> 
>  #else
> 
> -#define spin_lock_irqsave(lock, flags)	_spin_lock_irqsave(lock, flags)
> -#define read_lock_irqsave(lock, flags)	_read_lock_irqsave(lock, flags)
> -#define write_lock_irqsave(lock, flags)	_write_lock_irqsave(lock, flags)
> +#define spin_lock_irqsave(lock, flags)			\
> +	do {						\
> +		typecheck(unsigned long, flags);	\
> +		_spin_lock_irqsave(lock, flags);	\
> +	} while (0)
> +#define read_lock_irqsave(lock, flags)			\
> +	do {						\
> +		typecheck(unsigned long, flags);	\
> +		_read_lock_irqsave(lock, flags);	\
> +	} while (0)
> +#define write_lock_irqsave(lock, flags)			\
> +	do {						\
> +		typecheck(unsigned long, flags);	\
> +		_write_lock_irqsave(lock, flags);	\
> +	} while (0)
>  #define spin_lock_irqsave_nested(lock, flags, subclass)	\
>  	spin_lock_irqsave(lock, flags)
> 
> @@ -260,16 +290,25 @@ do {						\
>  } while (0)
>  #endif
> 
> -#define spin_unlock_irqrestore(lock, flags) \
> -					_spin_unlock_irqrestore(lock, flags)
> +#define spin_unlock_irqrestore(lock, flags)		\
> +	do {						\
> +		typecheck(unsigned long, flags);	\
> +		_spin_unlock_irqrestore(lock, flags);	\
> +	} while (0)
>  #define spin_unlock_bh(lock)		_spin_unlock_bh(lock)
> 
> -#define read_unlock_irqrestore(lock, flags) \
> -					_read_unlock_irqrestore(lock, flags)
> +#define read_unlock_irqrestore(lock, flags)		\
> +	do {						\
> +		typecheck(unsigned long, flags);	\
> +		_read_unlock_irqrestore(lock, flags);	\
> +	} while (0)
>  #define read_unlock_bh(lock)		_read_unlock_bh(lock)
> 
> -#define write_unlock_irqrestore(lock, flags) \
> -					_write_unlock_irqrestore(lock, flags)
> +#define write_unlock_irqrestore(lock, flags)		\
> +	do {						\
> +		typecheck(unsigned long, flags);	\
> +		_write_unlock_irqrestore(lock, flags);	\
> +	} while (0)
>  #define write_unlock_bh(lock)		_write_unlock_bh(lock)
> 
>  #define spin_trylock_bh(lock)	__cond_lock(lock, _spin_trylock_bh(lock))

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/