cleanup: Make no_free_ptr() __must_check

cleanup: Make no_free_ptr() __must_check

[Peter Zijlstra]

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)
 

Re: cleanup: Make no_free_ptr() __must_check

[Rasmus Villemoes]

On 15/08/2023 12.52, Peter Zijlstra wrote:
> 
> 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.
> 

> +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))))

So this does seem to work as advertised, but it could perhaps use some
comments. Because at first I read this as one big statement expression,
and I had this memory of a __must_check function call being the last
expression in such had no effect at all [1]. But this is actually a
comma expression.

Also, isn't it more complicated than necessary? Can we get rid of the
inner stmt expr and tmp var by just making it

  ((void) (p), ((typeof(p))__no_free_ptr((void **)&(p)))

which is more or less the whole reason comma expressions is a thing.

Rasmus

[1]
https://lore.kernel.org/lkml/6d190601-68f1-c086-97ac-2ee1c08f5a34@rasmusvillemoes.dk/

Re: cleanup: Make no_free_ptr() __must_check

[Peter Zijlstra]

On Tue, Aug 15, 2023 at 01:28:37PM +0200, Rasmus Villemoes wrote:
> On 15/08/2023 12.52, Peter Zijlstra wrote:
> > 
> > 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.
> > 
> 
> > +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))))
> 
> So this does seem to work as advertised, but it could perhaps use some
> comments. Because at first I read this as one big statement expression,
> and I had this memory of a __must_check function call being the last
> expression in such had no effect at all [1]. But this is actually a
> comma expression.

Right, I can into that as well, that was infact the first thing I tried.
Most vexing indeed.

> 
> Also, isn't it more complicated than necessary? Can we get rid of the
> inner stmt expr and tmp var by just making it
> 
>   ((void) (p), ((typeof(p))__no_free_ptr((void **)&(p)))
> 
> which is more or less the whole reason comma expressions is a thing.

Ah, so the point of the statement expression before the comma is to
validate that (p) is in fact a pointer, and to that effect we assign it
to a 'void *' temporary.

If that case is invalid, we'll get a compile fail with a dodgy message.

I did this, because (void **)&(p) looses type integrity due to the cast.

But yeah, I suppose it all needs a wee comment.

RE: cleanup: Make no_free_ptr() __must_check

[David Laight]

From: Peter Zijlstra
> Sent: 15 August 2023 14:54
...
> > Also, isn't it more complicated than necessary? Can we get rid of the
> > inner stmt expr and tmp var by just making it
> >
> >   ((void) (p), ((typeof(p))__no_free_ptr((void **)&(p)))
> >
> > which is more or less the whole reason comma expressions is a thing.
> 
> Ah, so the point of the statement expression before the comma is to
> validate that (p) is in fact a pointer, and to that effect we assign it
> to a 'void *' temporary.
> 
> If that case is invalid, we'll get a compile fail with a dodgy message.
> 
> I did this, because (void **)&(p) looses type integrity due to the cast.
> 
> But yeah, I suppose it all needs a wee comment.

Perhaps add an is_pointer_type() along with is_signed_type()
(and really is_constexpr()) to a global header.

Various checks can be used including:
#define is_pointer_type(t) (!is_constexpr((t)0))
which gives a 0/1 to play with rather than an immediate error.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: cleanup: Make no_free_ptr() __must_check

[Rasmus Villemoes]

On 15/08/2023 15.53, Peter Zijlstra wrote:
> On Tue, Aug 15, 2023 at 01:28:37PM +0200, Rasmus Villemoes wrote:
>
>> Also, isn't it more complicated than necessary? Can we get rid of the
>> inner stmt expr and tmp var by just making it
>>
>>   ((void) (p), ((typeof(p))__no_free_ptr((void **)&(p)))
>>
>> which is more or less the whole reason comma expressions is a thing.
> 
> Ah, so the point of the statement expression before the comma is to
> validate that (p) is in fact a pointer, and to that effect we assign it
> to a 'void *' temporary.
> 
> If that case is invalid, we'll get a compile fail with a dodgy message.
> 
> I did this, because (void **)&(p) looses type integrity due to the cast.
> 
> But yeah, I suppose it all needs a wee comment.

Ah, ok, I thought the purpose was to ensure the p expression gets
evaluated. Well, we can still do without the temp var and weird comma or
statement expressions:

static inline __must_check void * __no_free_ptr(void **pp, const void
*must_be_pointer_dummy)
{ void *p = *pp; *pp = NULL; return p; }

#define no_free_ptr(p) (__no_free_ptr((void **)&(p), p) )

Rasmus

Re: cleanup: Make no_free_ptr() __must_check

[Linus Torvalds]

On Wed, 16 Aug 2023 at 07:23, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
>
> Ah, ok, I thought the purpose was to ensure the p expression gets
> evaluated. Well, we can still do without the temp var and weird comma or
> statement expressions:

Well, the statement expression version with that types temporary
pointer was much better than yours, which just randomly returns 'void
*' and then lets the user assign it to any random pointer with no
warning.

But I think you can add just a 'typeof(p)' cast to it and that should
work out ok. I do absolutely hate how

But all of those macros seem to be fundamentally buggy because 'p'
gets evaluated twice. It could have side effects, even when just
having its address taken.

At that point the whole "comma or statement expression" discussion is
entirely immaterial.

So I think it needs to be something like

  #define __get_and_free_ptr(p) \
      ({ __auto_type __ptr = &(p); \
         __auto_type __val = *__ptr; \
         *__ptr = NULL;  __val; })

to deal with the "assign NULL and return old value without double evaluation".

And then you can have a wrapper macro to do the __must_check part,
something like

  static inline __must_check
  const volatile void * __must_check_fn(const volatile void *val)
  { return val; }

    #define no_free_ptr(p) \
        ((typeof(p)) __must_check_fn(__get_and_free_ptr(p)))

the above is entirely untested. Of course.

               Linus

Re: cleanup: Make no_free_ptr() __must_check

[Rasmus Villemoes]

On 16/08/2023 10.00, Linus Torvalds wrote:
> On Wed, 16 Aug 2023 at 07:23, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
>>
>> Ah, ok, I thought the purpose was to ensure the p expression gets
>> evaluated. Well, we can still do without the temp var and weird comma or
>> statement expressions:
> 
> Well, the statement expression version with that types temporary
> pointer was much better than yours, which just randomly returns 'void
> *' and then lets the user assign it to any random pointer with no
> warning.
> 
> But I think you can add just a 'typeof(p)' cast to it and that should
> work out ok. 

Yes, that was an omission, and probably any version will need to have
that cast.

> But all of those macros seem to be fundamentally buggy because 'p'
> gets evaluated twice. It could have side effects, even when just
> having its address taken.

True. I don't know how I convinced myself that & behaved like sizeof(),
probably because I ran with my first "this seems to just be to ensure p
gets evaluated" interpretation. For my own education, '&(foo[bar])' is
by definition equivalent to 'foo + bar', and both foo and bar can be
mostly arbitrary expressions (subject to one being a pointer and the
other an integer). Similarly for &(*foo).

> At that point the whole "comma or statement expression" discussion is
> entirely immaterial.
> 
> So I think it needs to be something like
> 
>   #define __get_and_free_ptr(p) \
>       ({ __auto_type __ptr = &(p); \
>          __auto_type __val = *__ptr; \
>          *__ptr = NULL;  __val; })
> 
> to deal with the "assign NULL and return old value without double evaluation".
> 
> And then you can have a wrapper macro to do the __must_check part,
> something like
> 
>   static inline __must_check
>   const volatile void * __must_check_fn(const volatile void *val)
>   { return val; }
> 
>     #define no_free_ptr(p) \
>         ((typeof(p)) __must_check_fn(__get_and_free_ptr(p)))
> 
> the above is entirely untested. Of course.

I think it should work, the final cast both gives the right type and
removes any unwanted const or volatile qualifiers. It's the same thing
we ended up doing in overflow.h.

While I'm embarrassing myself in public: since p is really only supposed
to be a local auto variable with __cleanup attribute, one could throw in
some ## games to try to prevent no_free_ptr() from being used on other
kinds of lvalues, something like adding "extern int bla_ ## p ## _bla;"
inside __get_and_free_ptr().

Rasmus

Re: cleanup: Make no_free_ptr() __must_check

[Peter Zijlstra]

On Wed, Aug 16, 2023 at 08:00:17AM +0000, Linus Torvalds wrote:

> So I think it needs to be something like
> 
>   #define __get_and_free_ptr(p) \
>       ({ __auto_type __ptr = &(p); \
>          __auto_type __val = *__ptr; \
>          *__ptr = NULL;  __val; })
> 
>   static inline __must_check
>   const volatile void * __must_check_fn(const volatile void *val)
>   { return val; }
> 
>     #define no_free_ptr(p) \
>         ((typeof(p)) __must_check_fn(__get_and_free_ptr(p)))
> 
> the above is entirely untested. Of course.

That works and is *much* saner than the horrible thing I did.