* [PATCH 3/3] Enforce "unsigned long flags;" when spinlocking
@ 2006-10-20 13:15 Alexey Dobriyan
2006-10-20 18:46 ` Andrew Morton
2006-10-21 0:13 ` [PATCH 3/3] " Alexey Dobriyan
0 siblings, 2 replies; 6+ messages in thread
From: Alexey Dobriyan @ 2006-10-20 13:15 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel
Make it break or warn if you pass to spin_lock_irqsave() and friends
something different from "unsigned long flags;". Suprisingly large amount of
these was caught by recent commit c53421b18f205c5f97c604ae55c6a921f034b0f6 .
Idea is largely from FRV typechecking.
Note #1: checking with sparse is still needed, because a driver can save and
pass around flags or something. So far patch is very intrusive.
Note #2: techically, we should break only if sizeof(flags) < sizeof(unsigned long),
but hey, there is opportunity to escalate. Thus !=
Note #3: yes, would break every single buggy out-of-tree module.
Signed-off-by: Alexey "altruistic today" Dobriyan <adobriyan@gmail.com>
---
include/linux/irqflags.h | 11 ++++++-
include/linux/spinlock.h | 68 ++++++++++++++++++++++++++++++++++++++---------
2 files changed, 65 insertions(+), 14 deletions(-)
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -49,11 +49,18 @@ #define local_irq_enable() \
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 { \
+ BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long)); \
+ typecheck(unsigned long, flags); \
+ raw_local_irq_save(flags); \
+ trace_hardirqs_off(); \
+ } while (0)
#define local_irq_restore(flags) \
do { \
+ BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long)); \
+ typecheck(unsigned long, flags); \
if (raw_irqs_disabled_flags(flags)) { \
raw_local_irq_restore(flags); \
trace_hardirqs_off(); \
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -183,13 +183,43 @@ #define write_lock(lock) _write_lock(lo
#define read_lock(lock) _read_lock(lock)
#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 { \
+ BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long)); \
+ typecheck(unsigned long, flags); \
+ flags = _spin_lock_irqsave(lock); \
+ } while (0);
+#define read_lock_irqsave(lock, flags) \
+ do { \
+ BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long)); \
+ typecheck(unsigned long, flags); \
+ flags = _read_lock_irqsave(lock); \
+ } while (0)
+#define write_lock_irqsave(lock, flags) \
+ do { \
+ BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long)); \
+ typecheck(unsigned long, flags); \
+ flags = _write_lock_irqsave(lock); \
+ } while (0)
#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 { \
+ BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long)); \
+ typecheck(unsigned long, flags); \
+ _spin_lock_irqsave(lock, flags); \
+ } while (0)
+#define read_lock_irqsave(lock, flags) \
+ do { \
+ BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long)); \
+ typecheck(unsigned long, flags); \
+ _read_lock_irqsave(lock, flags); \
+ } while (0)
+#define write_lock_irqsave(lock, flags) \
+ do { \
+ BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long)); \
+ typecheck(unsigned long, flags); \
+ _write_lock_irqsave(lock, flags); \
+ } while (0)
#endif
#define spin_lock_irq(lock) _spin_lock_irq(lock)
@@ -224,22 +254,36 @@ # define write_unlock_irq(lock) \
do { __raw_write_unlock(&(lock)->raw_lock); local_irq_enable(); } while (0)
#endif
-#define spin_unlock_irqrestore(lock, flags) \
- _spin_unlock_irqrestore(lock, flags)
+#define spin_unlock_irqrestore(lock, flags) \
+ do { \
+ BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long)); \
+ 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 { \
+ BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long)); \
+ 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 { \
+ BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long)); \
+ 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))
#define spin_trylock_irq(lock) \
({ \
+ BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long)); \
+ typecheck(unsigned long, flags); \
local_irq_disable(); \
spin_trylock(lock) ? \
1 : ({ local_irq_enable(); 0; }); \
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] Enforce "unsigned long flags;" when spinlocking
2006-10-20 13:15 [PATCH 3/3] Enforce "unsigned long flags;" when spinlocking Alexey Dobriyan
@ 2006-10-20 18:46 ` Andrew Morton
2006-10-20 23:38 ` Alexey Dobriyan
2006-10-21 0:13 ` [PATCH 3/3] " Alexey Dobriyan
1 sibling, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2006-10-20 18:46 UTC (permalink / raw)
To: Alexey Dobriyan; +Cc: linux-kernel
On Fri, 20 Oct 2006 17:15:44 +0400
Alexey Dobriyan <adobriyan@gmail.com> wrote:
> Make it break or warn if you pass to spin_lock_irqsave() and friends
> something different from "unsigned long flags;". Suprisingly large amount of
> these was caught by recent commit c53421b18f205c5f97c604ae55c6a921f034b0f6 .
>
> Idea is largely from FRV typechecking.
>
> Note #1: checking with sparse is still needed, because a driver can save and
> pass around flags or something. So far patch is very intrusive.
> Note #2: techically, we should break only if sizeof(flags) < sizeof(unsigned long),
> but hey, there is opportunity to escalate. Thus !=
> Note #3: yes, would break every single buggy out-of-tree module.
>
This is a pretty ugly-looking patch.
>
> + BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long)); \
> + typecheck(unsigned long, flags); \
> ...
> + BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long)); \
> + typecheck(unsigned long, flags); \
> ...
> + BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long)); \
> + typecheck(unsigned long, flags); \
> ...
> + BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long)); \
> + typecheck(unsigned long, flags); \
> ...
> + BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long)); \
> + typecheck(unsigned long, flags); \
> ...
starting to see a pattern here?
If we're going to do this then a helper macro build_check_irq_flags() would
help clean things up. It will also allow us to centralise the
warning-vs-error policy decision.
I'm not sure that we need both, do we? If it spits a warning then it'll
get fixed soon enough.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] Enforce "unsigned long flags;" when spinlocking
2006-10-20 18:46 ` Andrew Morton
@ 2006-10-20 23:38 ` Alexey Dobriyan
2006-10-21 0:32 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Alexey Dobriyan @ 2006-10-20 23:38 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel
On Fri, Oct 20, 2006 at 11:46:40AM -0700, Andrew Morton wrote:
> On Fri, 20 Oct 2006 17:15:44 +0400
> Alexey Dobriyan <adobriyan@gmail.com> wrote:
> > Make it break or warn if you pass to spin_lock_irqsave() and friends
> > something different from "unsigned long flags;". Suprisingly large amount of
> > these was caught by recent commit c53421b18f205c5f97c604ae55c6a921f034b0f6 .
> >
> > Idea is largely from FRV typechecking.
> >
> > Note #1: checking with sparse is still needed, because a driver can save and
> > pass around flags or something. So far patch is very intrusive.
> > Note #2: techically, we should break only if sizeof(flags) < sizeof(unsigned long),
> > but hey, there is opportunity to escalate. Thus !=
> > Note #3: yes, would break every single buggy out-of-tree module.
> >
>
> This is a pretty ugly-looking patch.
>
> >
> > + BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long)); \
> > + typecheck(unsigned long, flags); \
> > ...
> > + BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long)); \
> > + typecheck(unsigned long, flags); \
> > ...
> > + BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long)); \
> > + typecheck(unsigned long, flags); \
> > ...
> > + BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long)); \
> > + typecheck(unsigned long, flags); \
> > ...
> > + BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long)); \
> > + typecheck(unsigned long, flags); \
> > ...
>
> starting to see a pattern here?
OK, a pattern.
> If we're going to do this then a helper macro build_check_irq_flags() would
> help clean things up. It will also allow us to centralise the
> warning-vs-error policy decision.
I will find a common header. kernel.h probably.
> I'm not sure that we need both, do we?
I very much ask to do both.
I want build system, compiler, headers etc to do everything against
compiling buggy code. Similar to dropping rogue packets, you drop them
at the first firewall, even if your central box runs OpenBSD with all
ports closed.
In most cases it is impossible, in some cases it's cheap and simple.
So far the tower of usefullness is
a) compiler refuses to compile
b) linker refuses to link
c) compiler spits warning
d) sparse spits (endian) warning
a) is paragon of escalation.
b) is close: developer patches code, test-compile _just_one_ file and happily
send patch. Maintatiner gets patch, does allmodconfig before sending to
Linus, swears, fixes.
c) is also very visible, except GCC 4 doing strong as default compiler in
distros, deprecation and must check threats; folks also drink coffee while
Linux compiles. IIRC, there was seriously looking bug because of missed
header => missed prototype => missed warning
=> compiler pushes junk to stack
d) is OK, except number of people runnning sparse is orders of magnitude
smaller that number of people compiling, consequently noticed by
sparse bugs stay longer in tree. Add non-x86 and endian warnings not
being default to the picture.
Enough words. The closer to a) you are, the better. There is cheap and
simple way to be there.
> If it spits a warning then it'll get fixed soon enough.
:^) You apply patch to -mm and start waiting on how soon someone will fix
gregkh-driver-nozomi.patch so it wouldn't do
u32 flags;
but
unsigned long flags;
everywhere.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] Enforce "unsigned long flags;" when spinlocking
2006-10-20 13:15 [PATCH 3/3] Enforce "unsigned long flags;" when spinlocking Alexey Dobriyan
2006-10-20 18:46 ` Andrew Morton
@ 2006-10-21 0:13 ` Alexey Dobriyan
1 sibling, 0 replies; 6+ messages in thread
From: Alexey Dobriyan @ 2006-10-21 0:13 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel
> #define spin_trylock_irq(lock ) \
???????
> ({ \
> + BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long)); \
> + typecheck(unsigned long, flags); \
And it's broken. :-\
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] Enforce "unsigned long flags;" when spinlocking
2006-10-20 23:38 ` Alexey Dobriyan
@ 2006-10-21 0:32 ` Andrew Morton
2006-10-23 17:37 ` [PATCH v2] " Alexey Dobriyan
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2006-10-21 0:32 UTC (permalink / raw)
To: Alexey Dobriyan; +Cc: linux-kernel
On Sat, 21 Oct 2006 03:38:03 +0400
Alexey Dobriyan <adobriyan@gmail.com> wrote:
> > If we're going to do this then a helper macro build_check_irq_flags() would
> > help clean things up. It will also allow us to centralise the
> > warning-vs-error policy decision.
>
> I will find a common header. kernel.h probably.
irqflags.h sounds more appropriate.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] Enforce "unsigned long flags;" when spinlocking
2006-10-21 0:32 ` Andrew Morton
@ 2006-10-23 17:37 ` Alexey Dobriyan
0 siblings, 0 replies; 6+ messages in thread
From: Alexey Dobriyan @ 2006-10-23 17:37 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel
Make it break or warn if you pass to spin_lock_irqsave() and friends
something different from "unsigned long flags;". Suprisingly large amount of
these was caught by recent commit c53421b18f205c5f97c604ae55c6a921f034b0f6
and others.
Idea is largely from FRV typechecking. Suggestions from Andrew Morton.
All stupid typos in first version fixed.
Passes allmodconfig on i386, x86_64, alpha, arm as well as my usual config.
Note #1: checking with sparse is still needed, because a driver can save
and pass around flags or something. So far patch is very intrusive.
Note #2: techically, we should break only if
sizeof(flags) < sizeof(unsigned long),
however, the more pain for getting suspicious code into kernel,
the better.
Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---
include/linux/irqflags.h | 37 ++++++++++++++++++++++++++++----
include/linux/spinlock.h | 53 +++++++++++++++++++++++++++++++++++++++--------
2 files changed, 76 insertions(+), 14 deletions(-)
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -11,6 +11,12 @@
#ifndef _LINUX_TRACE_IRQFLAGS_H
#define _LINUX_TRACE_IRQFLAGS_H
+#define BUILD_CHECK_IRQ_FLAGS(flags) \
+ do { \
+ BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long)); \
+ typecheck(unsigned long, flags); \
+ } while (0)
+
#ifdef CONFIG_TRACE_IRQFLAGS
extern void trace_hardirqs_on(void);
extern void trace_hardirqs_off(void);
@@ -50,10 +56,15 @@ #define local_irq_enable() \
#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)
+ do { \
+ BUILD_CHECK_IRQ_FLAGS(flags); \
+ raw_local_irq_save(flags); \
+ trace_hardirqs_off(); \
+ } while (0)
#define local_irq_restore(flags) \
do { \
+ BUILD_CHECK_IRQ_FLAGS(flags); \
if (raw_irqs_disabled_flags(flags)) { \
raw_local_irq_restore(flags); \
trace_hardirqs_off(); \
@@ -69,8 +80,16 @@ #else /* !CONFIG_TRACE_IRQFLAGS_SUPPORT
*/
# 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 { \
+ BUILD_CHECK_IRQ_FLAGS(flags); \
+ local_irq_save(flags); \
+ } while (0)
+# define raw_local_irq_restore(flags) \
+ do { \
+ BUILD_CHECK_IRQ_FLAGS(flags); \
+ local_irq_restore(flags); \
+ } while (0)
#endif /* CONFIG_TRACE_IRQFLAGS_SUPPORT */
#ifdef CONFIG_TRACE_IRQFLAGS_SUPPORT
@@ -80,7 +99,11 @@ #define safe_halt() \
raw_safe_halt(); \
} while (0)
-#define local_save_flags(flags) raw_local_save_flags(flags)
+#define local_save_flags(flags) \
+ do { \
+ BUILD_CHECK_IRQ_FLAGS(flags); \
+ raw_local_save_flags(flags); \
+ } while (0)
#define irqs_disabled() \
({ \
@@ -90,7 +113,11 @@ ({ \
raw_irqs_disabled_flags(flags); \
})
-#define irqs_disabled_flags(flags) raw_irqs_disabled_flags(flags)
+#define irqs_disabled_flags(flags) \
+({ \
+ BUILD_CHECK_IRQ_FLAGS(flags); \
+ raw_irqs_disabled_flags(flags); \
+})
#endif /* CONFIG_X86 */
#endif
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -52,6 +52,7 @@ #include <linux/compiler.h>
#include <linux/thread_info.h>
#include <linux/kernel.h>
#include <linux/stringify.h>
+#include <linux/irqflags.h>
#include <asm/system.h>
@@ -183,13 +184,37 @@ #define write_lock(lock) _write_lock(lo
#define read_lock(lock) _read_lock(lock)
#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 { \
+ BUILD_CHECK_IRQ_FLAGS(flags); \
+ flags = _spin_lock_irqsave(lock); \
+ } while (0)
+#define read_lock_irqsave(lock, flags) \
+ do { \
+ BUILD_CHECK_IRQ_FLAGS(flags); \
+ flags = _read_lock_irqsave(lock); \
+ } while (0)
+#define write_lock_irqsave(lock, flags) \
+ do { \
+ BUILD_CHECK_IRQ_FLAGS(flags); \
+ flags = _write_lock_irqsave(lock); \
+ } while (0)
#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 { \
+ BUILD_CHECK_IRQ_FLAGS(flags); \
+ _spin_lock_irqsave(lock, flags); \
+ } while (0)
+#define read_lock_irqsave(lock, flags) \
+ do { \
+ BUILD_CHECK_IRQ_FLAGS(flags); \
+ _read_lock_irqsave(lock, flags); \
+ } while (0)
+#define write_lock_irqsave(lock, flags) \
+ do { \
+ BUILD_CHECK_IRQ_FLAGS(flags); \
+ _write_lock_irqsave(lock, flags); \
+ } while (0)
#endif
#define spin_lock_irq(lock) _spin_lock_irq(lock)
@@ -225,15 +250,24 @@ # define write_unlock_irq(lock) \
#endif
#define spin_unlock_irqrestore(lock, flags) \
- _spin_unlock_irqrestore(lock, flags)
+ do { \
+ BUILD_CHECK_IRQ_FLAGS(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)
+ do { \
+ BUILD_CHECK_IRQ_FLAGS(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)
+ do { \
+ BUILD_CHECK_IRQ_FLAGS(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))
@@ -247,6 +281,7 @@ ({ \
#define spin_trylock_irqsave(lock, flags) \
({ \
+ BUILD_CHECK_IRQ_FLAGS(flags); \
local_irq_save(flags); \
spin_trylock(lock) ? \
1 : ({ local_irq_restore(flags); 0; }); \
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2006-10-23 17:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2006-10-20 13:15 [PATCH 3/3] Enforce "unsigned long flags;" when spinlocking Alexey Dobriyan
2006-10-20 18:46 ` Andrew Morton
2006-10-20 23:38 ` Alexey Dobriyan
2006-10-21 0:32 ` Andrew Morton
2006-10-23 17:37 ` [PATCH v2] " Alexey Dobriyan
2006-10-21 0:13 ` [PATCH 3/3] " Alexey Dobriyan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Powered by JetHome