[v2,1/2] locking: Introduce __cleanup__ based guards
Commit Message
Use __attribute__((__cleanup__(func))) to buid pointer and lock
guards.
Actual usage in the next patch
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
include/linux/compiler_attributes.h | 6 +
include/linux/guards.h | 142 ++++++++++++++++++++++++++++++++++++
include/linux/irqflags.h | 7 +
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 | 27 ++++++
scripts/checkpatch.pl | 2
9 files changed, 197 insertions(+), 1 deletion(-)
Comments
On Fri, May 26, 2023 at 10:52:05PM +0200, Peter Zijlstra wrote:
> +++ b/include/linux/guards.h
> @@ -0,0 +1,142 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __LINUX_GUARDS_H
> +#define __LINUX_GUARDS_H
> +
> +#include <linux/compiler_attributes.h>
> +
> +/*
> + * Pointer Guards are special pointers (variables) with a scope bound cleanup
> + * function.
> + *
> + * Various guard types can be created using:
> + *
> + * DEFINE_PTR_GUARD(guard_type, typename, cleanup-exp)
> + *
> + * After which they can be used like so:
> + *
> + * ptr_guard(guard_type, name) = find_get_object(foo);
> + *
> + * Where the return type of find_get_object() should match the guard_type's
> + * 'typname *'. And when @name goes out of scope cleanup-exp is ran (inserted
> + * by the compiler) when !NULL at that time. Also see the __cleanup attribute.
> + */
> +
> +#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)
> +
Ha, and then I wanted to create an fdput() guard... :/
On Fri, May 26, 2023 at 2:25 PM Peter Zijlstra <peterz@infradead.org> wrote:
>> > + \
> > +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)
> > +
>
> Ha, and then I wanted to create an fdput() guard... :/
I have to say, I'm not super-convinced about the macros to create
these guard functions and types.
I suspect that it would be better to literally just declare the guard
types directly. For the fdget idiom, you really just want the guard
type to *be* the 'struct fd'.
And I think you made those macros just make too many assumptions.
It's not just "struct fd", for example. If you want to do things like
wrap 'local_irq_save', again it's not a pointer, the context is just
'unsigned long irqflags'.
And I really don't think those type-creation macros buy you anything.
What's wrong with just writing it out:
typedef struct fd guard_fdget_type_t;
static inline struct fd guard_fdget_init(int fd)
{ return fdget(fd); }
static inline void guard_fdget_exit(struct fd fd)
{ fdput(fd); }
and
typedef struct mutex *guard_mutex_type_t;
static inline struct mutex *guard_mutex_init(struct mutex *mutex)
{ mutex_lock(mutex); return mutex; }
static inline void guard_mutex_exit(struct mutex *mutex)
{ mutex_unlock(mutex); }
I don't think the latter is in the *least* less readable than doing
DEFINE_LOCK_GUARD_1(mutex, struct mutex,
mutex_lock(_G->lock),
mutex_unlock(_G->lock))
which is this magic macro that creates those, and makes them
completely ungreppable - and makes the type system very inflexible.
So I really think that it would be a lot easier to write out the
wrappers explicitly for the few types that really want this.
I dunno.
Once again, I have written example code in my MUA that I have not
tested AT ALL, and that may be fundamentally broken for some very
obvious reason, and I'm just too stupid to see it.
Linus
On Fri, May 26, 2023 at 02:54:40PM -0700, Linus Torvalds wrote:
> What's wrong with just writing it out:
>
> typedef struct fd guard_fdget_type_t;
> static inline struct fd guard_fdget_init(int fd)
> { return fdget(fd); }
> static inline void guard_fdget_exit(struct fd fd)
> { fdput(fd); }
>
(wrong guard type, ptr_guard vs lock_guard) but yeah, I had this same
realization during breakfast. Clearly the brain had already left last
night.
Specifically, I think we want ptr_guard() here (and possibly fdnull for
__to_fd(0)) for things like do_sendfile() where a struct fd is
initialized late.
The below seems to compile...
--- a/include/linux/file.h
+++ b/include/linux/file.h
@@ -10,6 +10,7 @@
#include <linux/types.h>
#include <linux/posix_types.h>
#include <linux/errno.h>
+#include <linux/guards.h>
struct file;
@@ -45,6 +46,13 @@ static inline void fdput(struct fd fd)
fput(fd.file);
}
+typedef struct fd ptr_guard_fdput_t;
+
+static inline void ptr_guard_fdput_cleanup(struct fd *fdp)
+{
+ fdput(*fdp);
+}
+
extern struct file *fget(unsigned int fd);
extern struct file *fget_raw(unsigned int fd);
extern struct file *fget_task(struct task_struct *task, unsigned int fd);
@@ -58,6 +66,8 @@ static inline struct fd __to_fd(unsigned
return (struct fd){(struct file *)(v & ~3),v & 3};
}
+#define fdnull __to_fd(0)
+
static inline struct fd fdget(unsigned int fd)
{
return __to_fd(__fdget(fd));
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1180,7 +1180,8 @@ COMPAT_SYSCALL_DEFINE6(pwritev2, compat_ulong_t, fd,
static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
size_t count, loff_t max)
{
- struct fd in, out;
+ ptr_guard(fdput, in) = fdnull;
+ ptr_guard(fdput, out) = fdnull;
struct inode *in_inode, *out_inode;
struct pipe_inode_info *opipe;
loff_t pos;
@@ -1191,35 +1192,35 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
/*
* Get input file, and verify that it is ok..
*/
- retval = -EBADF;
in = fdget(in_fd);
if (!in.file)
- goto out;
+ return -EBADF;
if (!(in.file->f_mode & FMODE_READ))
- goto fput_in;
- retval = -ESPIPE;
+ return -EBADF;
+
if (!ppos) {
pos = in.file->f_pos;
} else {
pos = *ppos;
if (!(in.file->f_mode & FMODE_PREAD))
- goto fput_in;
+ return -ESPIPE;
}
+
retval = rw_verify_area(READ, in.file, &pos, count);
if (retval < 0)
- goto fput_in;
+ return retval;
if (count > MAX_RW_COUNT)
count = MAX_RW_COUNT;
/*
* Get output file, and verify that it is ok..
*/
- retval = -EBADF;
out = fdget(out_fd);
if (!out.file)
- goto fput_in;
+ return -EBADF;
if (!(out.file->f_mode & FMODE_WRITE))
- goto fput_out;
+ return -EBADF;
+
in_inode = file_inode(in.file);
out_inode = file_inode(out.file);
out_pos = out.file->f_pos;
@@ -1228,9 +1229,8 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
max = min(in_inode->i_sb->s_maxbytes, out_inode->i_sb->s_maxbytes);
if (unlikely(pos + count > max)) {
- retval = -EOVERFLOW;
if (pos >= max)
- goto fput_out;
+ return -EOVERFLOW;
count = max - pos;
}
@@ -1249,7 +1249,7 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
if (!opipe) {
retval = rw_verify_area(WRITE, out.file, &out_pos, count);
if (retval < 0)
- goto fput_out;
+ return retval;
file_start_write(out.file);
retval = do_splice_direct(in.file, &pos, out.file, &out_pos,
count, fl);
@@ -1278,11 +1278,6 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
if (pos > max)
retval = -EOVERFLOW;
-fput_out:
- fdput(out);
-fput_in:
- fdput(in);
-out:
return retval;
}
@@ -77,6 +77,12 @@
#define __attribute_const__ __attribute__((__const__))
/*
+ * gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#index-cleanup-variable-attribute
+ * clang: https://clang.llvm.org/docs/AttributeReference.html#cleanup
+ */
+#define __cleanup(func) __attribute__((__cleanup__(func)))
+
+/*
* Optional: only supported since gcc >= 9
* Optional: not supported by clang
*
@@ -0,0 +1,142 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __LINUX_GUARDS_H
+#define __LINUX_GUARDS_H
+
+#include <linux/compiler_attributes.h>
+
+/*
+ * Pointer Guards are special pointers (variables) with a scope bound cleanup
+ * function.
+ *
+ * Various guard types can be created using:
+ *
+ * DEFINE_PTR_GUARD(guard_type, typename, cleanup-exp)
+ *
+ * After which they can be used like so:
+ *
+ * ptr_guard(guard_type, name) = find_get_object(foo);
+ *
+ * Where the return type of find_get_object() should match the guard_type's
+ * 'typname *'. And when @name goes out of scope cleanup-exp is ran (inserted
+ * by the compiler) when !NULL at that time. Also see the __cleanup attribute.
+ */
+
+#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)
+
+
+/*
+ * Lock Guards are like the pointer guards above except they also have
+ * a fixed initializor to cover both the Lock and Unlock of the lock type.
+ *
+ * Lock guards types can be created using:
+ *
+ * DEFINE_LOCK_GUARD_0(guard_type, Lock, Unlock, [extra guard members])
+ * DEFINE_LOCK_GUARD_1(guard_type, typename, Lock, Unlock, ...)
+ * DEFINE_LOCK_GUARD_2(guard_type, typename, Lock, Unlock, ...)
+ *
+ * Where the _n suffix indicates the number of arguments of 'typename *' the
+ * Lock function requires.
+ *
+ * Once defined, the lock guards can be used in one of two ways:
+ *
+ * guard(guard_type, name, var...);
+ *
+ * or:
+ *
+ * scoped (guard_type, var...) {
+ * ...
+ * }
+ *
+ * The first creates a named variable that is initialized with the Lock
+ * function and will call the Unlock function when it goes out of scope.
+ *
+ * The second creates an explicit scope, using a for-loop with an implicit
+ * named _scope variable. Again, Lock is called before the scope is entered and
+ * Unlock will be called when the scope is left.
+ *
+ * Both Lock and Unlock are expressions and can access the guard object through
+ * the _G pointer. The guard object will have _n implicit members called of
+ * type 'typename *' called 'lock' and 'lock2' as well as any additional
+ * members specified in the definition.
+ */
+
+#define DEFINE_LOCK_GUARD_0(_type, _Lock, _Unlock, ...) \
+typedef struct { \
+ __VA_ARGS__; \
+} lock_guard_##_type##_t; \
+ \
+static inline void lock_guard_##_type##_cleanup(lock_guard_##_type##_t *_G) \
+{ \
+ _Unlock; \
+} \
+ \
+static inline lock_guard_##_type##_t lock_guard_##_type##_init(void) \
+{ \
+ lock_guard_##_type##_t _g = { }, *_G __maybe_unused = &_g; \
+ _Lock; \
+ return _g; \
+}
+
+#define DEFINE_LOCK_GUARD_1(_type, _Type, _Lock, _Unlock, ...) \
+typedef struct { \
+ _Type *lock; \
+ __VA_ARGS__; \
+} lock_guard_##_type##_t; \
+ \
+static inline void lock_guard_##_type##_cleanup(lock_guard_##_type##_t *_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 DEFINE_LOCK_GUARD_2(_type, _Type, _Lock, _Unlock, ...) \
+typedef struct { \
+ _Type *lock; \
+ _Type *lock2; \
+ __VA_ARGS__; \
+} lock_guard_##_type##_t; \
+ \
+static inline void lock_guard_##_type##_cleanup(lock_guard_##_type##_t *_G) \
+{ \
+ _Unlock; \
+} \
+ \
+static inline lock_guard_##_type##_t \
+lock_guard_##_type##_init(_Type *lock, _Type *lock2) \
+{ \
+ lock_guard_##_type##_t _g = { .lock = lock, .lock2 = lock2 }, *_G = &_g;\
+ _Lock; \
+ return _g; \
+}
+
+#define variable_scope(_type, _enter, _exit) \
+ for (_type *_done = NULL, _scope __cleanup(_exit) = _enter; \
+ !_done; _done = (void *)8)
+
+#define scoped(_type, _var...) \
+ variable_scope(lock_guard_##_type##_t, \
+ lock_guard_##_type##_init(_var), \
+ lock_guard_##_type##_cleanup)
+
+#define guard(_type, _name, _var...) \
+ lock_guard_##_type##_t __cleanup(lock_guard_##_type##_cleanup) _name = \
+ lock_guard_##_type##_init(_var)
+
+#endif /* __LINUX_GUARDS_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_LOCK_GUARD_0(irq, local_irq_disable(), local_irq_enable())
+DEFINE_LOCK_GUARD_0(irqsave,
+ local_irq_save(_G->flags),
+ local_irq_restore(_G->flags),
+ unsigned long flags)
+
#endif
@@ -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_1(mutex, struct mutex,
+ mutex_lock(_G->lock),
+ mutex_unlock(_G->lock))
+
#endif /* __LINUX_MUTEX_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_LOCK_GUARD_0(preempt, preempt_disable(), preempt_enable())
+DEFINE_LOCK_GUARD_0(migrate, migrate_disable(), migrate_enable())
+
#endif /* __LINUX_PREEMPT_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_LOCK_GUARD_0(rcu, rcu_read_lock(), rcu_read_unlock())
+
#endif /* __LINUX_RCUPDATE_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))
@@ -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,31 @@ int __alloc_bucket_spinlocks(spinlock_t
void free_bucket_spinlocks(spinlock_t *locks);
+DEFINE_LOCK_GUARD_1(raw_spinlock, raw_spinlock_t,
+ raw_spin_lock(_G->lock),
+ raw_spin_unlock(_G->lock))
+
+DEFINE_LOCK_GUARD_1(raw_spinlock_irq, raw_spinlock_t,
+ raw_spin_lock_irq(_G->lock),
+ raw_spin_unlock_irq(_G->lock))
+
+DEFINE_LOCK_GUARD_1(raw_spinlock_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_1(spinlock, spinlock_t,
+ spin_lock(_G->lock),
+ spin_unlock(_G->lock))
+
+DEFINE_LOCK_GUARD_1(spinlock_irq, spinlock_t,
+ spin_lock_irq(_G->lock),
+ spin_unlock_irq(_G->lock))
+
+DEFINE_LOCK_GUARD_1(spinlock_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 */
@@ -5046,7 +5046,7 @@ sub process {
if|for|while|switch|return|case|
volatile|__volatile__|
__attribute__|format|__extension__|
- asm|__asm__)$/x)
+ asm|__asm__|scoped)$/x)
{
# cpp #define statements have non-optional spaces, ie
# if there is a space between the name and the open