Messages in this thread Patch in this message | | | Date | Tue, 15 Aug 2023 12:52:04 +0200 | From | Peter Zijlstra <> | 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@infradead.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)
| |