[v2,1/2] locking: Introduce __cleanup__ based guards

Commit Message

Peter Zijlstra May 26, 2023, 8:52 p.m. UTC

Use __attribute__((__cleanup__(func))) to buid pointer and lock
guards.

Actual usage in the next patch

Signed-off-by: Peter Zijlstra (Intel) <[e-mail]>
---
 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

Peter Zijlstra May 26, 2023, 9:24 p.m. UTC | #1

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... :/

Linus Torvalds May 26, 2023, 9:54 p.m. UTC | #2

On Fri, May 26, 2023 at 2:25 PM Peter Zijlstra <[e-mail]> 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

Peter Zijlstra May 27, 2023, 8:57 a.m. UTC | #3

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;
 }

Patch

--- a/include/linux/compiler_attributes.h
+++ b/include/linux/compiler_attributes.h
@@ -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
  *
--- /dev/null
+++ 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)
+
+
+/*
+ * 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 */
--- 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_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
--- 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_1(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_LOCK_GUARD_0(preempt, preempt_disable(), preempt_enable())
+DEFINE_LOCK_GUARD_0(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_LOCK_GUARD_0(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,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 */
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -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