* [PATCH] add typecheck on irqsave and friends for correct flags
@ 2008-05-01 23:51 Steven Rostedt
2008-05-02 0:28 ` Andrew Morton
2008-05-02 8:37 ` Alexey Dobriyan
0 siblings, 2 replies; 6+ messages in thread
From: Steven Rostedt @ 2008-05-01 23:51 UTC (permalink / raw)
To: LKML
Cc: akpm, Ingo Molnar, Peter Zijlstra, Christoph Hellwig,
Alexey Dobriyan, Adrian Bunk
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.
Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
include/linux/irqflags.h | 52 ++++++++++++++++++++++++----------
include/linux/spinlock.h | 71 ++++++++++++++++++++++++++++++++++++-----------
2 files changed, 92 insertions(+), 31 deletions(-)
Index: linux-compile.git/include/linux/irqflags.h
===================================================================
--- linux-compile.git.orig/include/linux/irqflags.h 2008-05-01 18:52:52.000000000 -0400
+++ linux-compile.git/include/linux/irqflags.h 2008-05-01 19:21:05.000000000 -0400
@@ -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))
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] add typecheck on irqsave and friends for correct flags
2008-05-01 23:51 [PATCH] add typecheck on irqsave and friends for correct flags Steven Rostedt
@ 2008-05-02 0:28 ` Andrew Morton
2008-05-02 1:16 ` Steven Rostedt
2008-05-02 8:37 ` Alexey Dobriyan
1 sibling, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2008-05-02 0:28 UTC (permalink / raw)
To: Steven Rostedt; +Cc: linux-kernel, mingo, peterz, hch, adobriyan, bunk
On Thu, 1 May 2008 19:51:18 -0400 (EDT)
Steven Rostedt <rostedt@goodmis.org> 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.
hm, not exactly a thing of beauty, but it could have been worse.
If we had implemeted these things properly, as
unsigned long spin_lock_irqsave(spinlock_t *lock);
then we wouldn't be able to do this at all.
Oh well, thanks, I'll toss it in there. Maybe this should go into
git-sched (aka git-omnibus)?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] add typecheck on irqsave and friends for correct flags
2008-05-02 0:28 ` Andrew Morton
@ 2008-05-02 1:16 ` Steven Rostedt
2008-05-02 1:49 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Steven Rostedt @ 2008-05-02 1:16 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel, mingo, peterz, hch, adobriyan, bunk
On Thu, 1 May 2008, Andrew Morton wrote:
> > This patch adds a typecheck inside the irqsave and restore functions
> > to flag these cases.
>
> hm, not exactly a thing of beauty, but it could have been worse.
Beauty is in the eye of the beholder.
(a mother always thinks their kid is beautiful ;-).
>
>
> If we had implemeted these things properly, as
>
> unsigned long spin_lock_irqsave(spinlock_t *lock);
Then we would have been forced to do the
irqflags_t spin_lock_irqsave(spinlock_t *lock);
trick. Which would probably be the better long term solution, but the
biggest PITA for you in the short term.
-- Steve
>
> then we wouldn't be able to do this at all.
>
> Oh well, thanks, I'll toss it in there. Maybe this should go into
> git-sched (aka git-omnibus)?
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] add typecheck on irqsave and friends for correct flags
2008-05-02 1:16 ` Steven Rostedt
@ 2008-05-02 1:49 ` Andrew Morton
2008-05-02 1:57 ` Steven Rostedt
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2008-05-02 1:49 UTC (permalink / raw)
To: Steven Rostedt; +Cc: linux-kernel, mingo, peterz, hch, adobriyan, bunk
On Thu, 1 May 2008 21:16:48 -0400 (EDT) Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Thu, 1 May 2008, Andrew Morton wrote:
> > > This patch adds a typecheck inside the irqsave and restore functions
> > > to flag these cases.
> >
> > hm, not exactly a thing of beauty, but it could have been worse.
>
> Beauty is in the eye of the beholder.
> (a mother always thinks their kid is beautiful ;-).
As long as your patch doesn't soil its diaper.
> >
> >
> > If we had implemeted these things properly, as
> >
> > unsigned long spin_lock_irqsave(spinlock_t *lock);
>
> Then we would have been forced to do the
>
> irqflags_t spin_lock_irqsave(spinlock_t *lock);
>
> trick. Which would probably be the better long term solution, but the
> biggest PITA for you in the short term.
We should have done that from day one. If there's an architecture out
there which cannot fit its interrupt state into its unsigned long (hard to
believe) then it'll need to pull stunts with cookies and lookups or
something. And on 64-bit architectures we're using 4 more bytes of local
storage than is needed.
But I don't think we've had enough problems with this particular issue to
justify a kernel-wide edit like that. And this patch should settle the
issue.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] add typecheck on irqsave and friends for correct flags
2008-05-02 1:49 ` Andrew Morton
@ 2008-05-02 1:57 ` Steven Rostedt
0 siblings, 0 replies; 6+ messages in thread
From: Steven Rostedt @ 2008-05-02 1:57 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel, mingo, peterz, hch, adobriyan, bunk
On Thu, 1 May 2008, Andrew Morton wrote:
> >
> > Then we would have been forced to do the
> >
> > irqflags_t spin_lock_irqsave(spinlock_t *lock);
> >
> > trick. Which would probably be the better long term solution, but the
> > biggest PITA for you in the short term.
>
> We should have done that from day one. If there's an architecture out
> there which cannot fit its interrupt state into its unsigned long (hard to
> believe) then it'll need to pull stunts with cookies and lookups or
> something. And on 64-bit architectures we're using 4 more bytes of local
> storage than is needed.
>
> But I don't think we've had enough problems with this particular issue to
> justify a kernel-wide edit like that. And this patch should settle the
> issue.
I'm just thinking of all the wonderful tricks at hand if we had a special
irqflags_t type. But I'm sure you don't want any more special tricks from
us with respect to spinlocks ;-)
-- Steve
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] add typecheck on irqsave and friends for correct flags
2008-05-01 23:51 [PATCH] add typecheck on irqsave and friends for correct flags Steven Rostedt
2008-05-02 0:28 ` Andrew Morton
@ 2008-05-02 8:37 ` Alexey Dobriyan
1 sibling, 0 replies; 6+ messages in thread
From: Alexey Dobriyan @ 2008-05-02 8:37 UTC (permalink / raw)
To: Steven Rostedt
Cc: LKML, akpm, Ingo Molnar, Peter Zijlstra, Christoph Hellwig,
Adrian Bunk, linux-arch, dhowells
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))
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2008-05-02 7:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-05-01 23:51 [PATCH] add typecheck on irqsave and friends for correct flags Steven Rostedt
2008-05-02 0:28 ` Andrew Morton
2008-05-02 1:16 ` Steven Rostedt
2008-05-02 1:49 ` Andrew Morton
2008-05-02 1:57 ` Steven Rostedt
2008-05-02 8:37 ` Alexey Dobriyan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox