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) <peterz@xxxxxxxxxxxxx>
> ---
> 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
