Messages in this thread | | | Date | Fri, 26 May 2023 10:05:41 -0700 | From | Kees Cook <> | Subject | Re: [RFC][PATCH 1/2] locking: Introduce __cleanup__ based guards |
| |
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@infradead.org> > --- > 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
| |