Use __attribute__((__cleanup__(func))) to buid various guards:
- ptr_guard()
- void_guard() / void_scope()
- lock_guard() / lock_scope()
- double_lock_guard() / double_lock_scope()
Where the _guard thingies are variables with scope-based cleanup and
the _scope thingies are basically do-once for-loops with the same.
The CPP is rather impenetrable -- but I'll attempt to write proper
comments if/when people think this is worth pursuing.
Actual usage in the next patch
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
include/linux/compiler_attributes.h | 2
include/linux/irqflags.h | 7 ++
include/linux/guards.h | 118 ++++++++++++++++++++++++++++++++++++
include/linux/mutex.h | 5 +
include/linux/preempt.h | 4 +
include/linux/rcupdate.h | 3
include/linux/sched/task.h | 2
include/linux/spinlock.h | 23 +++++++
8 files changed, 164 insertions(+)
--- a/include/linux/compiler_attributes.h
+++ b/include/linux/compiler_attributes.h
@@ -366,4 +366,6 @@
*/
#define __fix_address noinline __noclone
+#define __cleanup(func) __attribute__((__cleanup__(func)))
+
#endif /* __LINUX_COMPILER_ATTRIBUTES_H */
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -13,6 +13,7 @@
#define _LINUX_TRACE_IRQFLAGS_H
#include <linux/typecheck.h>
+#include <linux/guards.h>
#include <asm/irqflags.h>
#include <asm/percpu.h>
@@ -267,4 +268,10 @@ extern void warn_bogus_irq_restore(void)
#define irqs_disabled_flags(flags) raw_irqs_disabled_flags(flags)
+DEFINE_VOID_GUARD(irq, local_irq_disable(), local_irq_enable())
+DEFINE_VOID_GUARD(irqsave,
+ local_irq_save(_G->flags),
+ local_irq_restore(_G->flags),
+ unsigned long flags;)
+
#endif
--- /dev/null
+++ b/include/linux/guards.h
@@ -0,0 +1,118 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __LINUX_GUARDS_H
+#define __LINUX_GUARDS_H
+
+#include <linux/compiler_attributes.h>
+
+/* Pointer Guard */
+
+#define DEFINE_PTR_GUARD(_type, _Type, _Put) \
+typedef _Type *ptr_guard_##_type##_t; \
+static inline void ptr_guard_##_type##_cleanup(_Type **_ptr) \
+{ \
+ _Type *_G = *_ptr; \
+ if (_G) \
+ _Put(_G); \
+}
+
+#define ptr_guard(_type, _name) \
+ ptr_guard_##_type##_t _name __cleanup(ptr_guard_##_type##_cleanup)
+
+
+/* Void Guard */
+
+#define DEFINE_VOID_GUARD(_type, _Lock, _Unlock, ...) \
+typedef struct { \
+ __VA_ARGS__ \
+} void_guard_##_type##_t; \
+ \
+static inline void void_guard_##_type##_cleanup(void *_g) \
+{ \
+ void_guard_##_type##_t *_G __maybe_unused = _g; \
+ _Unlock; \
+} \
+ \
+static inline void_guard_##_type##_t void_guard_##_type##_init(void) \
+{ \
+ void_guard_##_type##_t _g = { }, *_G __maybe_unused = &_g; \
+ _Lock; \
+ return _g; \
+}
+
+#define void_guard(_type, _name) \
+ void_guard_##_type##_t _name __cleanup(void_guard_##_type##_cleanup) = \
+ void_guard_##_type##_init()
+
+#define void_scope(_type) \
+ for (struct { void_guard_##_type##_t guard; bool done; } _scope \
+ __cleanup(void_guard_##_type##_cleanup) = \
+ { .guard = void_guard_##_type##_init() }; !_scope.done; \
+ _scope.done = true)
+
+
+/* Lock Guard */
+
+#define DEFINE_LOCK_GUARD(_type, _Type, _Lock, _Unlock, ...) \
+typedef struct { \
+ _Type *lock; \
+ __VA_ARGS__ \
+} lock_guard_##_type##_t; \
+ \
+static inline void lock_guard_##_type##_cleanup(void *_g) \
+{ \
+ lock_guard_##_type##_t *_G = _g; \
+ _Unlock; \
+} \
+ \
+static inline lock_guard_##_type##_t lock_guard_##_type##_init(_Type *lock) \
+{ \
+ lock_guard_##_type##_t _g = { .lock = lock }, *_G = &_g; \
+ _Lock; \
+ return _g; \
+}
+
+#define lock_guard(_type, _name, _ptr) \
+ lock_guard_##_type##_t _name __cleanup(lock_guard_##_type##_cleanup) = \
+ lock_guard_##_type##_init(_ptr)
+
+#define lock_scope(_type, _ptr) \
+ for (struct { lock_guard_##_type##_t guard; bool done; } _scope \
+ __cleanup(lock_guard_##_type##_cleanup) = \
+ { .guard = lock_guard_##_type##_init(_ptr) }; !_scope.done; \
+ _scope.done = true)
+
+
+/* Double Lock Guard */
+
+#define DEFINE_DOUBLE_LOCK_GUARD(_type, _Type, _Lock, _Unlock, ...) \
+typedef struct { \
+ _Type *lock; \
+ _Type *lock2; \
+ __VA_ARGS__ \
+} double_lock_guard_##_type##_t; \
+ \
+static inline void double_lock_guard_##_type##_cleanup(void *_g) \
+{ \
+ double_lock_guard_##_type##_t *_G = _g; \
+ _Unlock; \
+} \
+ \
+static inline double_lock_guard_##_type##_t double_lock_guard_##_type##_init(_Type *lock, _Type *lock2) \
+{ \
+ double_lock_guard_##_type##_t _g = { .lock = lock, .lock2 = lock2 }, *_G = &_g;\
+ _Lock; \
+ return _g; \
+}
+
+#define double_lock_guard(_type, _name, _ptr, _ptr2) \
+ double_lock_guard_##_type##_t _name __cleanup(double_lock_guard_##_type##_cleanup) = \
+ double_lock_guard_##_type##_init(_ptr, _ptr2)
+
+#define double_lock_scope(_type, _ptr, _ptr2) \
+ for (struct { double_lock_guard_##_type##_t guard; bool done; } _scope \
+ __cleanup(double_lock_guard_##_type##_cleanup) = \
+ { .guard = double_lock_guard_##_type##_init(_ptr, _ptr2) }; \
+ !_scope.done; _scope.done = true)
+
+
+#endif /* __LINUX_GUARDS_H */
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -19,6 +19,7 @@
#include <asm/processor.h>
#include <linux/osq_lock.h>
#include <linux/debug_locks.h>
+#include <linux/guards.h>
#ifdef CONFIG_DEBUG_LOCK_ALLOC
# define __DEP_MAP_MUTEX_INITIALIZER(lockname) \
@@ -219,4 +220,8 @@ extern void mutex_unlock(struct mutex *l
extern int atomic_dec_and_mutex_lock(atomic_t *cnt, struct mutex *lock);
+DEFINE_LOCK_GUARD(mutex, struct mutex,
+ mutex_lock(_G->lock),
+ mutex_unlock(_G->lock))
+
#endif /* __LINUX_MUTEX_H */
--- a/include/linux/preempt.h
+++ b/include/linux/preempt.h
@@ -8,6 +8,7 @@
*/
#include <linux/linkage.h>
+#include <linux/guards.h>
#include <linux/list.h>
/*
@@ -463,4 +464,7 @@ static __always_inline void preempt_enab
preempt_enable();
}
+DEFINE_VOID_GUARD(preempt, preempt_disable(), preempt_enable())
+DEFINE_VOID_GUARD(migrate, migrate_disable(), migrate_enable())
+
#endif /* __LINUX_PREEMPT_H */
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -27,6 +27,7 @@
#include <linux/preempt.h>
#include <linux/bottom_half.h>
#include <linux/lockdep.h>
+#include <linux/guards.h>
#include <asm/processor.h>
#include <linux/cpumask.h>
#include <linux/context_tracking_irq.h>
@@ -1095,4 +1096,6 @@ rcu_head_after_call_rcu(struct rcu_head
extern int rcu_expedited;
extern int rcu_normal;
+DEFINE_VOID_GUARD(rcu, rcu_read_lock(), rcu_read_unlock())
+
#endif /* __LINUX_RCUPDATE_H */
--- a/include/linux/sched/task.h
+++ b/include/linux/sched/task.h
@@ -126,6 +126,8 @@ static inline void put_task_struct(struc
__put_task_struct(t);
}
+DEFINE_PTR_GUARD(put_task, struct task_struct, put_task_struct)
+
static inline void put_task_struct_many(struct task_struct *t, int nr)
{
if (refcount_sub_and_test(nr, &t->usage))
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -61,6 +61,7 @@
#include <linux/stringify.h>
#include <linux/bottom_half.h>
#include <linux/lockdep.h>
+#include <linux/guards.h>
#include <asm/barrier.h>
#include <asm/mmiowb.h>
@@ -502,5 +503,27 @@ int __alloc_bucket_spinlocks(spinlock_t
void free_bucket_spinlocks(spinlock_t *locks);
+DEFINE_LOCK_GUARD(raw, raw_spinlock_t,
+ raw_spin_lock(_G->lock),
+ raw_spin_unlock(_G->lock))
+DEFINE_LOCK_GUARD(raw_irq, raw_spinlock_t,
+ raw_spin_lock_irq(_G->lock),
+ raw_spin_unlock_irq(_G->lock))
+DEFINE_LOCK_GUARD(raw_irqsave, raw_spinlock_t,
+ raw_spin_lock_irqsave(_G->lock, _G->flags),
+ raw_spin_unlock_irqrestore(_G->lock, _G->flags),
+ unsigned long flags;)
+
+DEFINE_LOCK_GUARD(spin, spinlock_t,
+ spin_lock(_G->lock),
+ spin_unlock(_G->lock))
+DEFINE_LOCK_GUARD(spin_irq, spinlock_t,
+ spin_lock_irq(_G->lock),
+ spin_unlock_irq(_G->lock))
+DEFINE_LOCK_GUARD(spin_irqsave, spinlock_t,
+ spin_lock_irqsave(_G->lock, _G->flags),
+ spin_unlock_irqrestore(_G->lock, _G->flags),
+ unsigned long flags;)
+
#undef __LINUX_INSIDE_SPINLOCK_H
#endif /* __LINUX_SPINLOCK_H */
On Fri, May 26, 2023 at 05:05:50PM +0200, Peter Zijlstra wrote:
> Use __attribute__((__cleanup__(func))) to buid various guards:
>
> - ptr_guard()
> - void_guard() / void_scope()
> - lock_guard() / lock_scope()
> - double_lock_guard() / double_lock_scope()
>
> Where the _guard thingies are variables with scope-based cleanup and
> the _scope thingies are basically do-once for-loops with the same.
This makes things much easier to deal with, rather than forcing loops
into separate functions, etc, and hoping to get the cleanup right.
>
> The CPP is rather impenetrable -- but I'll attempt to write proper
> comments if/when people think this is worth pursuing.
Yes please. Comments would help a lot. I was scratching my head over _G
for a bit before I realized what was happening. :)
>
> Actual usage in the next patch
>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> ---
> include/linux/compiler_attributes.h | 2
> include/linux/irqflags.h | 7 ++
> include/linux/guards.h | 118 ++++++++++++++++++++++++++++++++++++
> include/linux/mutex.h | 5 +
> include/linux/preempt.h | 4 +
> include/linux/rcupdate.h | 3
> include/linux/sched/task.h | 2
> include/linux/spinlock.h | 23 +++++++
> 8 files changed, 164 insertions(+)
>
> --- a/include/linux/compiler_attributes.h
> +++ b/include/linux/compiler_attributes.h
> @@ -366,4 +366,6 @@
> */
> #define __fix_address noinline __noclone
>
> +#define __cleanup(func) __attribute__((__cleanup__(func)))
> +
> #endif /* __LINUX_COMPILER_ATTRIBUTES_H */
nitpick: sorting. This needs to be moved up alphabetically; the comment
at the start of the file says:
...
* This file is meant to be sorted (by actual attribute name,
* not by #define identifier). ...
> [...]
> +#define DEFINE_VOID_GUARD(_type, _Lock, _Unlock, ...) \
> +typedef struct { \
> + __VA_ARGS__ \
> +} void_guard_##_type##_t; \
> + \
> [...]
> +DEFINE_VOID_GUARD(irq, local_irq_disable(), local_irq_enable())
> +DEFINE_VOID_GUARD(irqsave,
> + local_irq_save(_G->flags),
> + local_irq_restore(_G->flags),
> + unsigned long flags;)
Yeah, good trick for defining 0-or-more members to the guard struct. I
expect the common cases to be 0 or 1, so perhaps move the final ";" to
after __VA_ARGS__ to avoid needing it in the DEFINEs? (And even in this
initial patch, there's only 1 non-empty argument...)
> [...]
> --- /dev/null
> +++ b/include/linux/guards.h
> @@ -0,0 +1,118 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __LINUX_GUARDS_H
> +#define __LINUX_GUARDS_H
> +
> +#include <linux/compiler_attributes.h>
> +
> +/* Pointer Guard */
> +
> +#define DEFINE_PTR_GUARD(_type, _Type, _Put) \
> +typedef _Type *ptr_guard_##_type##_t; \
> +static inline void ptr_guard_##_type##_cleanup(_Type **_ptr) \
> +{ \
> + _Type *_G = *_ptr; \
> + if (_G) \
> + _Put(_G); \
> +}
*loud forehead-smacking noise* __cleanup with inlines! I love it!
> [...]
> +#define void_scope(_type) \
> + for (struct { void_guard_##_type##_t guard; bool done; } _scope \
> + __cleanup(void_guard_##_type##_cleanup) = \
> + { .guard = void_guard_##_type##_init() }; !_scope.done; \
> + _scope.done = true)
Heh, yes, that'll work for a forced scope, and I bet compiler
optimizations will collapse a bunch of this into a very clean execution
path.
> [...]
> +DEFINE_VOID_GUARD(preempt, preempt_disable(), preempt_enable())
> +DEFINE_VOID_GUARD(migrate, migrate_disable(), migrate_enable())
> [...]
> +DEFINE_VOID_GUARD(rcu, rcu_read_lock(), rcu_read_unlock())
> [...]
> +DEFINE_PTR_GUARD(put_task, struct task_struct, put_task_struct)
> [...]
It seems like there are some _really_ common code patterns you're
targeting here, and I bet we could do some mechanical treewide changes
with Coccinelle to remove a ton of boilerplate code.
I like this API, and the CPP isn't very obfuscated at all, compared to
some stuff we've already got in the tree. :)
-Kees
--
Kees Cook
On Fri, May 26, 2023 at 8:23 AM Peter Zijlstra <[email protected]> wrote:
>
> The CPP is rather impenetrable -- but I'll attempt to write proper
> comments if/when people think this is worth pursuing.
Ugh.
It's not only impenetrable, it seems _unnecessarily_ so.
Yes, yes, 'for()' loops can only declare one type, and if you want
multiple typed variables you declare a struct that contains all the
types.
But you don't actually *need* multiple types.
Yes, you think you do, because you want to use that 'bool done' to
make the for-loop only execute once. Nasty limitation of the for
syntax.
But you can actually do the 'bool done' using the exact same type you
have for the guard - just make it a pointer instead, and use NULL for
"not done" and non-NULL for "done". It ends up acting exactly like a
boolean.
But that extra structure is only a detail. The real ugliness comes
from using different scoping macros.
And I think you don't actually need to have those different forms of
"scoped()" macros for different cases. I think you can just use
variable macro arguments.
IOW, something like this:
#define variable_scope(type, enter, exit) \
for (type *_done = NULL, _scope __cleanup(exit) = enter;
!_done; _done = (void *)8)
#define scoped(type, init...) \
variable_scope(scope_##type##_t, scope_##type##_init(init),
scope_##type##_cleanup)
and then you can do
scoped (rcu) {
...
}
and it will call "scope_rcu_init()" on entry, and
"scope_rcu_exit(_scope)" on exit.
And just doing
scoped (mutex, mymutex) { ... }
will call "scope_mytex_init(mymutex)" on entry, and
"scope_mytex_exit(_scope)" on exit.
And if you just make the scope_##type##_init() functions return the
right values, it all works very naturally.
I think you can also do things like
scoped(irqsave) { ... }
scoped(irqoff) { ... }
scoped(preempt) { ... }
very naturally. No need for that odd "one scope for 'void', one scope
for 'lock'" nonsense.
I dunno. I didn't *test* the above. Maybe you already tried something
like the above, and there's a reason why it doesn't work.
Linus
On 5/26/23 11:05, Peter Zijlstra wrote:
> Use __attribute__((__cleanup__(func))) to buid various guards:
>
> - ptr_guard()
> - void_guard() / void_scope()
> - lock_guard() / lock_scope()
> - double_lock_guard() / double_lock_scope()
>
> Where the _guard thingies are variables with scope-based cleanup and
> the _scope thingies are basically do-once for-loops with the same.
>
> The CPP is rather impenetrable -- but I'll attempt to write proper
> comments if/when people think this is worth pursuing.
>
> Actual usage in the next patch
>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> ---
> include/linux/compiler_attributes.h | 2
> include/linux/irqflags.h | 7 ++
> include/linux/guards.h | 118 ++++++++++++++++++++++++++++++++++++
> include/linux/mutex.h | 5 +
> include/linux/preempt.h | 4 +
> include/linux/rcupdate.h | 3
> include/linux/sched/task.h | 2
> include/linux/spinlock.h | 23 +++++++
> 8 files changed, 164 insertions(+)
That is an interesting idea and may help to simplify some of the common
code patterns that we have in the kernel. The macros are a bit hard to
read and understand though I thought I got a rough idea of what they are
trying to do.
BTW, do we have a use case for double_lock_guard/double_lock_scope? I
can envision a nested lock_scope inside a lock_scope, but taking 2 auto
locks of the same type at init time and then unlock them at exit just
doesn't make sense to me.
Cheers,
Longman
On Fri, May 26, 2023 at 11:22:36AM -0700, Linus Torvalds wrote:
> But you can actually do the 'bool done' using the exact same type you
> have for the guard - just make it a pointer instead, and use NULL for
> "not done" and non-NULL for "done". It ends up acting exactly like a
> boolean.
Damn; I've actually seen that and should've thought of it.
> IOW, something like this:
>
> #define variable_scope(type, enter, exit) \
> for (type *_done = NULL, _scope __cleanup(exit) = enter;
> !_done; _done = (void *)8)
>
> #define scoped(type, init...) \
> variable_scope(scope_##type##_t, scope_##type##_init(init),
> scope_##type##_cleanup)
>
> I dunno. I didn't *test* the above. Maybe you already tried something
> like the above, and there's a reason why it doesn't work.
I have not; let me go try that. That does look *much* nicer.
On 5/26/23 14:49, Waiman Long wrote:
[...]
>
> BTW, do we have a use case for double_lock_guard/double_lock_scope? I
> can envision a nested lock_scope inside a lock_scope, but taking 2 auto
> locks of the same type at init time and then unlock them at exit just
> doesn't make sense to me.
AFAIU taking both runqueue locks for source and destination runqueues on
migration is one use-case for double_lock_guard/scope.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com
On Fri, May 26, 2023 at 7:05 PM Kees Cook <[email protected]> wrote:
>
> > --- a/include/linux/compiler_attributes.h
> > +++ b/include/linux/compiler_attributes.h
> > @@ -366,4 +366,6 @@
> > */
> > #define __fix_address noinline __noclone
> >
> > +#define __cleanup(func) __attribute__((__cleanup__(func)))
> > +
> > #endif /* __LINUX_COMPILER_ATTRIBUTES_H */
>
> nitpick: sorting. This needs to be moved up alphabetically; the comment
> at the start of the file says:
>
> ...
> * This file is meant to be sorted (by actual attribute name,
> * not by #define identifier). ...
+1, also please add:
/*
* gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#index-cleanup-variable-attribute
* clang: https://clang.llvm.org/docs/AttributeReference.html#cleanup
*/
Cheers,
Miguel
On 5/26/23 14:58, Mathieu Desnoyers wrote:
> On 5/26/23 14:49, Waiman Long wrote:
> [...]
>>
>> BTW, do we have a use case for double_lock_guard/double_lock_scope? I
>> can envision a nested lock_scope inside a lock_scope, but taking 2
>> auto locks of the same type at init time and then unlock them at exit
>> just doesn't make sense to me.
>
> AFAIU taking both runqueue locks for source and destination runqueues
> on migration is one use-case for double_lock_guard/scope.
>
I see. Thanks for the clarification. I forgot about that special case.
Cheers,
Longman