Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 8 Jun 2023 22:06:18 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     keescook@chromium.org, gregkh@linuxfoundation.org,
        pbonzini@redhat.com, linux-kernel@vger.kernel.org,
        ojeda@kernel.org, ndesaulniers@google.com, mingo@redhat.com,
        will@kernel.org, longman@redhat.com, boqun.feng@gmail.com,
        juri.lelli@redhat.com, vincent.guittot@linaro.org,
        dietmar.eggemann@arm.com, rostedt@goodmis.org, bsegall@google.com,
        mgorman@suse.de, bristot@redhat.com, vschneid@redhat.com,
        paulmck@kernel.org, frederic@kernel.org, quic_neeraju@quicinc.com,
        joel@joelfernandes.org, josh@joshtriplett.org,
        mathieu.desnoyers@efficios.com, jiangshanlai@gmail.com,
        rcu@vger.kernel.org, tj@kernel.org, tglx@linutronix.de,
        linux-toolchains@vger.kernel.org
Subject: Re: [PATCH v2 0/2] Lock and Pointer guards
Message-ID: <20230608200618.GA1020752@hirez.programming.kicks-ass.net>
References: <20230530092342.GA149947@hirez.programming.kicks-ass.net>
 <20230606094251.GA907347@hirez.programming.kicks-ass.net>
 <CAHk-=wi-RyoUhbChiVaJZoZXheAwnJ7OO=Gxe85BkPAd93TwDA@mail.gmail.com>
 <20230606134005.GE905437@hirez.programming.kicks-ass.net>
 <CAHk-=wgQ5m+SnWTYGHu0JgYXTk2dkGF+msX=ARfYoo3t1_fX9g@mail.gmail.com>
 <20230606180806.GA942082@hirez.programming.kicks-ass.net>
 <CAHk-=wgXN1YxGMUFeuC135aeUvqduF8zJJiZZingzS1Pao5h0A@mail.gmail.com>
 <20230607094101.GA964354@hirez.programming.kicks-ass.net>
 <20230608085248.GA1002251@hirez.programming.kicks-ass.net>
 <CAHk-=wj-BGgTF0YgY+L7_G8Jb0UO38Cd8dwrfMqFMEh93B3D7g@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHk-=wj-BGgTF0YgY+L7_G8Jb0UO38Cd8dwrfMqFMEh93B3D7g@mail.gmail.com>
Precedence: bulk

On Thu, Jun 08, 2023 at 08:45:53AM -0700, Linus Torvalds wrote:

> > DEFINE_CLASS(kfree, void *, kfree(THIS), p, void *p)
> >
> >         smart_ptr(kfree, mem) = kzalloc_node(...);
> >         if (!mem)
> >                 return -ENOMEM;
> 
> No, the above is broken, and would result in us using "void *" in
> situations where we really *really* don't want that.
> 
> For automatic freeing, you want something that can handle different
> types properly, and without having to constantly declare the types
> somewhere else before use.

Ah, I see what you did with the no_free_ptr(), that avoids having to
have two pointers around, nice!

> So for convenient automatic pointer freeing, you want an interface
> much more akin to
> 
>         struct whatever *ptr __automatic_kfree = kmalloc(...);
> 
> which is much more legible, doesn't have any type mis-use issues, and
> is also just trivially dealt with by a
> 
>   static inline void automatic_kfree_wrapper(void *pp)
>   { void *p = *(void **)pp; if (p) kfree(p); }
>   #define __automatic_kfree \
>         __attribute__((__cleanup__(automatic_kfree_wrapper)))
>   #define no_free_ptr(p) \
>         ({ __auto_type __ptr = (p); (p) = NULL; __ptr; })
> 
> which I just tested generates the sane code even for the "set the ptr
> to NULL and return success" case.
> 
> The above allows you to trivially do things like
> 
>         struct whatever *p __automatic_kfree = kmalloc(..);
> 
>         if (!do_something(p))
>                 return -ENOENT;
> 
>         return no_free_ptr(p);
> 
> and it JustWorks(tm).

OK, something like so then?


#define DEFINE_FREE(name, type, free) \
	static inline __free_##name(type *p) { type _P = *p; free; }

#define __free(name)	__cleanup(__free_##name)

#define no_free_ptr(p) \
	({ __auto_type __ptr = (p); (p) = NULL; __ptr; })


DEFINE_FREE(kfree, void *, if (_P) kfree(_P));

	struct obj *p __free(kfree) = kmalloc(...);

	if (!do_something(p))
		return -ENOENT;

	return no_free_ptr(p);


DEFINE_CLASS(find_get_context, struct perf_event_context *,
	     if (!IS_ERR_OR_NULL(_C)) put_ctx(_C),
	     find_get_context(task, event), struct task_struct *task, struct perf_event *event)

DEFINE_FREE(free_event, struct perf_event *,
	    if (!IS_ERR_OR_NULL(_P)) free_event(_P));


	struct perf_event *event __free(free_event) = perf_event_alloc(...)
	if (IS_ERR(event))
		return event;

	class(find_get_context, ctx)(task, event);
	if (IS_ERR(ctx))
		return (void*)ctx;

	if (!task && !container_of(ctx, struct perf_cpu_context, ctx)->online)
		return -ENODEV;

	...

	event->ctx = get_ctx(ctx);

	return no_free_ptr(event);


works for me, I'll go make it happen.