[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230815105204.GA927051@hirez.programming.kicks-ass.net>
Date: Tue, 15 Aug 2023 12:52:04 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Linus Torvalds <torvalds@...ux-foundation.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
dan.j.williams@...el.com
Cc: linux-kernel@...r.kernel.org, linux@...musvillemoes.dk,
Nick Desaulniers <ndesaulniers@...gle.com>
Subject: cleanup: Make no_free_ptr() __must_check
recent discussion brought about the realization that it makes sense for
no_free_ptr() to have __must_check semantics in order to avoid leaking
the resource.
Additionally, add a few comments to clarify why/how things work.
Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
---
I'm not particularly happy with the way the __must_check stuff works,
but I could not get a statement-expression to have __must_check
semantics.
include/linux/cleanup.h | 34 +++++++++++++++++++++++++++++++---
1 file changed, 31 insertions(+), 3 deletions(-)
diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index 53f1a7a932b0..d2a2905c463e 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -7,8 +7,9 @@
/*
* DEFINE_FREE(name, type, free):
* simple helper macro that defines the required wrapper for a __free()
- * based cleanup function. @free is an expression using '_T' to access
- * the variable.
+ * based cleanup function. @free is an expression using '_T' to access the
+ * variable. @free should typically include a NULL test before calling a
+ * function, see the example below.
*
* __free(name):
* variable attribute to add a scoped based cleanup to the variable.
@@ -17,6 +18,9 @@
* like a non-atomic xchg(var, NULL), such that the cleanup function will
* be inhibited -- provided it sanely deals with a NULL value.
*
+ * NOTE: this has __must_check semantics so that it is harder to accidentally
+ * leak the resource.
+ *
* return_ptr(p):
* returns p while inhibiting the __free().
*
@@ -24,6 +28,8 @@
*
* DEFINE_FREE(kfree, void *, if (_T) kfree(_T))
*
+ * void *alloc_obj(...)
+ * {
* struct obj *p __free(kfree) = kmalloc(...);
* if (!p)
* return NULL;
@@ -32,6 +38,24 @@
* return NULL;
*
* return_ptr(p);
+ * }
+ *
+ * NOTE: the DEFINE_FREE()'s @free expression includes a NULL test even though
+ * kfree() is fine to be called with a NULL value. This is on purpose. This way
+ * the compiler sees the end of our alloc_obj() function as:
+ *
+ * tmp = p;
+ * p = NULL;
+ * if (p)
+ * kfree(p);
+ * return tmp;
+ *
+ * And through the magic of value-propagation and dead-code-elimination, it
+ * eliminates the actual cleanup call and compiles into:
+ *
+ * return p;
+ *
+ * Without the NULL test it turns into a mess and the compiler can't help us.
*/
#define DEFINE_FREE(_name, _type, _free) \
@@ -39,8 +63,12 @@
#define __free(_name) __cleanup(__free_##_name)
+static inline __must_check void * __no_free_ptr(void **pp)
+{ void *p = *pp; *pp = NULL; return p; }
+
#define no_free_ptr(p) \
- ({ __auto_type __ptr = (p); (p) = NULL; __ptr; })
+ (({ void * __maybe_unused ___t = (p); }), \
+ ((typeof(p))__no_free_ptr((void **)&(p))))
#define return_ptr(p) return no_free_ptr(p)
Powered by blists - more mailing lists