Re: [PATCH v2 0/2] Lock and Pointer guards

Re: [PATCH v2 0/2] Lock and Pointer guards

[Peter Zijlstra]

On Tue, May 30, 2023 at 11:23:42AM +0200, Peter Zijlstra wrote:

> Yes, it's a wee bit more involved, but I'm thinking it gives a fair
> amount of flexibility and we don't need to ret rid of
> -Wdeclaration-after-statement.

So I made all that work and .. Yes, you're absolutely right.

Busting -Wdeclaration-after-statement is the right thing to do for
guards.

So then I came up with:

#define __ptr_guard(_guard, _name)                                     \
       guard_##_guard##_t _name __cleanup(guard_##_guard##_cleanup)

#define ptr_guard(_guard, _name)                                       \
       __diag(push)                                                    \
       __diag(ignored "-Wdeclaration-after-statement")                 \
       __ptr_guard(_guard, _name)                                      \
       __diag(pop)

#define guard_init(_guard, _var...)                                    \
       guard_##_guard##_init(_var)

#define named_guard(_guard, _name, _var...)                            \
       ptr_guard(_guard, _name) = guard_init(_guard, _var)

#define guard(_guard, _var...)                                         \
       named_guard(_guard, __UNIQUE_ID(guard), _var)

#define scoped_guard(_guard, _var...)                                  \
       for (__ptr_guard(_guard, scope) = guard_init(_guard, _var),     \
            *done = NULL; !done; done = (void *)1)


And that all (mostly) works on clang, but not GCC :-( GCC refuses to
accept _Pragma() inside an expression.

So I now have that ptr_guard() with push/pop for clang but without for
GCC, which means that only clang has a fighting chance to report
-Wdeclaration-after-statement warns until such a time as that we can get
GCC 'fixed'.

  https://godbolt.org/z/5MPeq5W6K


FWIW: the work-in-progress patches I have are here:

  https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git/log/?h=core/guards

Re: [PATCH v2 0/2] Lock and Pointer guards

[Linus Torvalds]

On Tue, Jun 6, 2023 at 2:43 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> ( GCC refuses to accept _Pragma() inside an expression.

If we really want this all, I think we'd just stop using
-Wdeclaration-after-statement entirely.

There are other uses for it, and people have asked for mixing
declarations and code before.

I think that particular straightjacket has been a good thing, but I
also think that it's ok to just let it go as a hard rule, and just try
to make it a coding style issue for the common case, but allow mixed
declarations and code when it makes sense.

For the whole "automatic release case it definitely makes sense, but
it's not like it isn't possible useful elsewhere. I just don't want
for it to become some global pattern for everything.

That said, I still don't understand why you lke the name "guard" for
this.  I understand not liking "auto", but "guard" doesn't seem any
better. In fact, much worse. Guarded expressions means something
completely different both in real life and in computer science.

I'm assuming there's some history there, but it makes no sense to me
as a name here.

              Linus

Re: [PATCH v2 0/2] Lock and Pointer guards

[Peter Zijlstra]

On Tue, Jun 06, 2023 at 06:17:33AM -0700, Linus Torvalds wrote:

> That said, I still don't understand why you lke the name "guard" for
> this.  I understand not liking "auto", but "guard" doesn't seem any
> better. In fact, much worse. Guarded expressions means something
> completely different both in real life and in computer science.
> 
> I'm assuming there's some history there, but it makes no sense to me
> as a name here.

I know the name from C++ where it is std::lock_guard<> (well, back when
I still did C++ it wasn't std, but whatever), and Rust seems to have
std::sync::MutexGuard<>.

But if that's the sole objection left, lets have a bike-shed party and
pick a colour :-)

'shield' 'sentry' 'sentinel' 'keeper' 'custodian' 'warden' ?

Re: [PATCH v2 0/2] Lock and Pointer guards

[Linus Torvalds]

On Tue, Jun 6, 2023 at 6:40 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> I know the name from C++ where it is std::lock_guard<> (well, back when
> I still did C++ it wasn't std, but whatever), and Rust seems to have
> std::sync::MutexGuard<>.
>
> But if that's the sole objection left, lets have a bike-shed party and
> pick a colour :-)
>
> 'shield' 'sentry' 'sentinel' 'keeper' 'custodian' 'warden' ?

I feel like you seem entirely too fixated on locking.

That's not even _remotely_ the most interesting case, I think.

For the common locking patterns , maybe "guard" makes sense as part of
the name, but it damn well shouldn't be about any "ptr_guard" or
whatever.

I think it should be some trivial

        mutex_scope(mymytex) {
                ..
        }

and yes, in that case scoping makes tons of sense and might even be
required, because you do want to visually see the scope of the
locking.

I'm not seeing the point of "guard" in that name either, but I don't
think the above is the complex case for naming, for use, or for
syntax.

In contrast, I think reference counting and allocations are the much
more interesting one, and the case where you are also more likely to
have multiple consecutive ones, and where it's likely less about "this
region is protected" and much more about the RAII patterns and
resources that you just want cleanup for.

So that's why I had that "auto_release" name - to make it clear that
it's about the *cleanup*, not about some "guarded region". See my
argument?

The locking cases can probably be handled with just a couple of macros
(mutex, rcu, maybe spinlock?) and I would violently argue that the
locking cases will have to have unconditional cleanup (ie if you
*ever* have the situation where some path is supposed to be able to
return with the lock held and no cleanup, then you should simply not
use the "mutex_scope()" kind of helper AT ALL).

So locking is, I feel, almost uninteresting. There are only a few
cases, and only trivial patterns for it, because anything non-trivial
had better be done very explicitly, and very much visibly by hand.

But look at the cases where we currently use "goto exit" kind of code,
or have duplicated cleanups because we do "cleanup(x); return -ERRNO"
kinds of patterns.

Some of them are locking, yes. But a lot of them are more generic "do
this before returning" kinds of things: freeing (possibly complex)
data structures, uncharging statistcs, or even things like writing
results back to user space.

And, unlike locking, those often (but not always) end up having the
"don't do this in the single success case" situation, so that you need
to have some way to show "ok, don't release it after all". Yes, that
ends up in practice likely being setting that special pointer to NULL,
but I think we need to have a much better syntax for it to show that
"now we're *not* doing the auto-release".

So it's that more complex case that I

 (a) don't think "guard" makes any sense for at all

 (b) think it's where the bigger payoffs are

 (c) think that generally are not "nested" (you release the resources
you've allocated, but you don't "nest" the allocations - they are just
serial.

 (d) think that the syntax could be pretty nasty, because the cleanup
is not just a trivial fixed "unlock" function

Just as an example of something that has a bit of everything, look at
kernel/cpu.c and the _cpu_down() function in particular, which has
that "goto out" to do all the cleanup.

That looks like a perfect case for having some nice "associate the
cleanup with the initialization" RAII patterns, but while it does have
one lock (cpus_write_lock/unlock()), even that one is abstracted away
so that it may be a no-op, and might be a percpu rwlock, and so having
to create a special macro for that would be more work than is worth
it.

Because what I *don't* want to happen is that people create some
one-time-use "helper" macro infrastructure to use this all. Again,
that cpus_write_lock/unlock is a good example of this.

End result: to avoid having crazy indentation, and crazy one-time
helper inline functions or whatever, I think you'd really want some
fairly generic model for

  "I am doing X, and I want to to Y as a cleanup when you exit the function"

where both X and Y can be *fairly* complicated things. They might be
as simple (and reasonably common) as "fd = fdget(f)" and "fdput(fd)",
but what about the one-offs like that cpus_write_lock/unlock pattern?

That one only happens in two places (_cpu_down() and _cpu_up()), and
maybe the answer is "we can't reasonably do it for that, because the
complexity is not worth it".

But maybe we *can*.

For example, this is likely the only realistic *good* case for the
horrible "nested function" thing that gcc supports. In my "look, we
could clean up a 'struct fd' example from last week, I made it do
something like this:

    /* Trivial "getfd()" wrapper */
    static inline void release_fd(struct fd *fd)
    { fdput(*fd); }

    #define auto_getfd(name, n) \
        auto_release_name(struct fd, name, fdget(n), release_fd)

but it would possibly be much more generic, and much more useful, if
that "release_fd()" function was generated by the macro as a local
nested inline function.

End result: you could use any arbitrary local cleanup code.

So you could have something like

  #define RAII(type, var, init, exit) \
        __RAII(type, var, init, exit, __UNIQUE_ID(fn)

  #define __RAII(type, var, init, exit, exitname) \
        void exitname(type *p) { exit } \
        type var __attribute__((__cleanup__(exitname))) = (init)

and do all of the above with

    RAII(struct fd, fd, fdget(f), fdput(fd));

because that macro would literally expand to create a (uniquely named)
nested function that then contains that "fdput(fd)".

I dunno. The syntax looks pretty bad, and I'm not even convinced clang
supports nested functions, so the above may not be an option.

But wouldn't it be nice to just be able to declare that arbitrary
cleanup in-place?

Then the lock cases would really just be trivial helper wrappers. And
you can use "guard" there if you want, but I feel it makes absolutely
*no* sense in the generic case. There is absolutely nothng that is
being "guarded" by having an automatic cleanup of some random
variable.

              Linus

Re: [PATCH v2 0/2] Lock and Pointer guards

[Kees Cook]

On Tue, Jun 06, 2023 at 07:50:47AM -0700, Linus Torvalds wrote:
> So you could have something like
> 
>   #define RAII(type, var, init, exit) \
>         __RAII(type, var, init, exit, __UNIQUE_ID(fn)
> 
>   #define __RAII(type, var, init, exit, exitname) \
>         void exitname(type *p) { exit } \
>         type var __attribute__((__cleanup__(exitname))) = (init)
> 
> and do all of the above with
> 
>     RAII(struct fd, fd, fdget(f), fdput(fd));

"fdput(fd)" needs to be "fdput(*p)", since otherwise "fdput(fd)" is
referencing "fd" before it has been declared.

But regardless, yes, Clang is angry about the nested function. Also,
while my toy[1] example doesn't show it, GCC may also generate code
that requires an executable stack for some instances (or at least it
did historically) that need trampolines.

[1] https://godbolt.org/z/WTjx6Gs7x

Also, more nits on naming: isn't this more accurately called Scope-based
Resource Management (SBRM) not RAII? (RAII is technically object lifetime,
and SBRM is scope entry/exit.)

-- 
Kees Cook

Re: [PATCH v2 0/2] Lock and Pointer guards

[Peter Zijlstra]

On Tue, Jun 06, 2023 at 07:50:47AM -0700, Linus Torvalds wrote:
> I feel like you seem entirely too fixated on locking.

It's what I started out with... but it's certainly not everything.

The thing I have removes pretty much all the error gotos from
sched/core.c and events/core.c and while locking is ofcourse a fair
amount of that there's significant non-locking usage.

>  (c) think that generally are not "nested" (you release the resources
> you've allocated, but you don't "nest" the allocations - they are just
> serial.
> 
>  (d) think that the syntax could be pretty nasty, because the cleanup
> is not just a trivial fixed "unlock" function

So I'm not sure you've seen the actual convertions I've done, but yes,
there's lots of those.

I've just used the guard naming because locks is what I started out
with.

+	ptr_guard(kfree, alloc) = kzalloc(event->read_size, GFP_KERNEL);
+	if (!alloc)
 		return -ENOMEM;


 	event = perf_event_alloc(&attr, cpu, task, group_leader, NULL,
 				 NULL, NULL, cgroup_fd);
+	if (IS_ERR(event))
+		return PTR_ERR(event);
+
+	ptr_guard(free_event, event_guard) = event;


+	ptr_guard(put_task, p) = find_get_task(pid);
+	if (!p)
+		return -ESRCH;


So it does all that... you just hate the naming -- surely we can fix
that.


Would it all be less offensive if I did: s/guard/cleanup/ on the whole
thing? Then we'd have things like:


DEFINE_PTR_CLEANUP(put_task, struct task_struct *,
		   if (_C) put_task_struct(_C))

	ptr_cleanup(put_task, p) = find_get_task(pid);
	if (!p)
		return -ESRCH;


DEFINE_PTR_CLEANUP(free_event, struct perf_event *,
		   if (!IS_ERR_OR_NULL(_C)) free_event(_C))

	ptr_cleanup(free_event, event) = perf_event_alloc(...);
	if (IS_ERR(event))
		return PTR_ERR(event);


DEFINE_PTR_CLEANUP(kfree, void *, kfree(_C))

	ptr_cleanup(kfree, foo) = kzalloc(...);
	if (!foo)
		return -ENOMEM;


But do we then continue with:

DEFINE_CLEANUP(mutex, struct mutex *, mutex_lock(_C), mutex_unlock(_C))

	cleanup(mutex, &my_mutex);


	scoped_cleanup (mutex, &my_mutex) {
		...
	}

etc..? or do we keep guard() there, but based internally on
ptr_cleanup() with the cleanup_## naming.

Re: [PATCH v2 0/2] Lock and Pointer guards

[Linus Torvalds]

On Tue, Jun 6, 2023 at 11:08 AM Peter Zijlstra <peterz@infradead.org> wrote:>
> Would it all be less offensive if I did: s/guard/cleanup/ on the whole
> thing?

It's more than "guard" for me.

What is "ptr"? Why? We already know of at least one case where it's
not a pointer at all, ie 'struct fd'.

So I *really* hate the naming. Absolutely none of it makes sense to
me. One part is a nonsensical name apparently based on a special-case
operation, and the other part is a nonsensical type from just one
random - if common - implementation issue.

What you want to do is to have a way to define and name a
"constructor/desctructor" pair for an arbitrary type - *not*
necessarily a pointer - and then optionally a way to say "Oh, don't do
the destructor, because I'm actually going to use it long-term".

I said "cleanup", but that's not right either, since we always have to
have that initializer too.

Maybe just bite the bullet, and call the damn thing a "class", and
have some syntax like

     DEFINE_CLASS(name, type, exit, init, initarg...);

to create the infrastructure for some named 'class'. So you'd have

    DEFINE_CLASS(mutex, struct mutex *,
        mutex_unlock(*_P),
        ({mutex_lock(mutex); mutex;}), struct mutex *mutex)

to define the mutex "class", and do

    DEFINE_CLASS(fd, struct fd,
        fdput(*_P),
        fdget(f), int f)

for the 'struct fd' thing.

The above basically would just create the wrapper functions (and
typedefs) for the constructor and destructor.

So the 'DEFINE_CLASS(mutex ..)' thing would basically just expand to

    typedef struct mutex *class_mutex_type;
    static inline void class_mutex_destructor(class_mutex_type *_C)
    { mutex_unlock(*_C); }
    static inline class_mutex_type class_mutex_constructor(struct mutex *mutex)
    { return ({mutex_lock(mutex); mutex;}); }

Then to _instantiate_ one of those, you'd do

    INSTANTIATE_CLASS(name, var)

which would expand to

    class_name_type var
        __attribute__((__cleanup__(class_name_destructor))) =
class_name_constructor

and the magic of that syntax is that you'd actually use that
"INSTANTIATE_CLASS()" with the argument to the init function
afterwards, so you'd actually do

    INSTANTIATE_CLASS(mutex, n)(&sched_domains_mutex);

to create a variable 'n' of class 'mutex', where the
class_mutex_constructor gets the pointer to 'sched_domain_mutex' as
the argument.

And at THAT point, you can do this:

    #define mutex_guard(m) \
        INSTANTIATE_CLASS(mutex, __UNIQUE_ID(mutex))(m)

and now you can do

       mutex_guard(&sched_domains_mutex);

to basically start a guarded section where you hold 'sched_domains_mutex'.

And in that *very* least situation, 'guard' makes sense in the name.
But no earlier. And there is absolutely no 'ptr' anywhere here.

The above should work also for the 'struct fd' case, so you can do

       INSTANTIATE(fd, f)(fd);

to create a 'struct fd' named 'f' that is initialized with
'fdget(fd)', and will DTRT when going out of scope. Modulo any stupid
errors I did.

And ok, I didn't write out the exact macro magic to *get* those
expansions above (I just tried to write out the approximate end
result), but it should be mostly fairly straightforward.

So it would be the usual token pasting tricks to get the 'class type',
the 'class destructor' and the 'class constructor' functions.

Finally, note that the "initarg" part is a macro vararg thing. The
initargs can be empty (for things like RCU), but it could also
possibly be multiple arguments (like a "size and gfp_flags" for an
allocation?).

I'm sure there's something horribly wrong in the above, but my point
is that I'd really like this to make naming and conceptual sense.

And if somebody goes "The above is basically exactly what the original
C++ compiler did back when it was really just a pre-processor for C",
then you'd be 100% right. The above is basically (a part of) what
Bjarne did, except he did it as a separate pass.

And to answer the next question: why not just give up and do C++ -
it's because of all the *other* things Bjarne did.

             Linus

Re: [PATCH v2 0/2] Lock and Pointer guards

[Peter Zijlstra]

On Tue, Jun 06, 2023 at 04:22:26PM -0700, Linus Torvalds wrote:
> On Tue, Jun 6, 2023 at 11:08 AM Peter Zijlstra <peterz@infradead.org> wrote:>
> > Would it all be less offensive if I did: s/guard/cleanup/ on the whole
> > thing?
> 
> It's more than "guard" for me.
> 
> What is "ptr"? Why? We already know of at least one case where it's
> not a pointer at all, ie 'struct fd'.

(so in my view struct fd is nothing more than a fat pointer)

> So I *really* hate the naming. Absolutely none of it makes sense to
> me. One part is a nonsensical name apparently based on a special-case
> operation, and the other part is a nonsensical type from just one
> random - if common - implementation issue.
> 
> What you want to do is to have a way to define and name a
> "constructor/desctructor" pair for an arbitrary type - *not*
> necessarily a pointer - and then optionally a way to say "Oh, don't do
> the destructor, because I'm actually going to use it long-term".

Yes, so when it's a 'pointer', that part becomes assigning it NULL (or
fdnull in the struct fd case). For example:

DEFINE_PTR_CLEANUP(kfree, void *, kfree(_C))

	ptr_cleanup(kfree, mem) = kzalloc(....);
	if (!mem)
		return -ENOMEM;

	object = mem;

	// build object with more failure cases

	mem = NULL;          // object is a success, we keep it.
	return object;

> I said "cleanup", but that's not right either, since we always have to
> have that initializer too.

I've found that for most things the initializer part isn't actually that
important. Consider that struct fd thing again; perf has a helper:

static inline struct fd perf_fget_light(int fd)
{
	struct fd f = fdget(fd);
	if (!f.file)
		return fdnull;

	if (f.file->f_op != &perf_fops) {
		fdput(f);
		return fdnull;
	}
	return f;
}

So now we have both fdget() and perf_fget_light() to obtain a struct fd,
both need fdput().

The pointer with destructor semantics works for both:

DEFINE_PTR_CLEANUP(fdput, struct fd, fdput(_C))

	ptr_cleanup(fdput, f) = perf_fget_light(fd);

or, somewhere else:

	ptr_cleanup(fdput, f) = fdget(fd);


The same is true for kfree(), we have a giant pile of allocation
functions that all are freed with kfree(): kmalloc(), kzalloc(),
kmalloc_node(), kzalloc_node(), krealloc(), kmalloc_array(),
krealloc_array(), kcalloc(), etc..

> Maybe just bite the bullet, and call the damn thing a "class", and
> have some syntax like
> 
>      DEFINE_CLASS(name, type, exit, init, initarg...);
> 
> to create the infrastructure for some named 'class'. So you'd have
> 
>     DEFINE_CLASS(mutex, struct mutex *,
>         mutex_unlock(*_P),
>         ({mutex_lock(mutex); mutex;}), struct mutex *mutex)
> 
> to define the mutex "class", and do
> 
>     DEFINE_CLASS(fd, struct fd,
>         fdput(*_P),
>         fdget(f), int f)
> 
> for the 'struct fd' thing.

Right; that is very close to what I have. And certainly useful --
although as per the above, perhaps not so for the struct fd case.

> Then to _instantiate_ one of those, you'd do
> 
>     INSTANTIATE_CLASS(name, var)
> 
> which would expand to
> 
>     class_name_type var
>         __attribute__((__cleanup__(class_name_destructor))) =
> class_name_constructor
> 
> and the magic of that syntax is that you'd actually use that
> "INSTANTIATE_CLASS()" with the argument to the init function
> afterwards, so you'd actually do
> 
>     INSTANTIATE_CLASS(mutex, n)(&sched_domains_mutex);
> 
> to create a variable 'n' of class 'mutex', where the
> class_mutex_constructor gets the pointer to 'sched_domain_mutex' as
> the argument.

Yes, I had actually considered this syntax, and I really like it. The
only reason I hadn't done that is because the for-loop thing, there I
couldn't make it work.

> I'm sure there's something horribly wrong in the above, but my point
> is that I'd really like this to make naming and conceptual sense.

Right, I hear ya. So the asymmetric case (iow destructor only) could be
seen as using the copy-constructor.

#define DEFINE_CLASS(name, type, exit, init, init_args...)		\
typedef type class_##name##_t;						\
static inline void class_##name##_destructor(type *this)		\
{ type THIS = *this; exit; }						\
static inline type class_##name##_constructor(init_args)		\
{ type THIS = init; return THIS; }

#define __INSTANTIATE_VAR(name, var)					\
	class_##name##_t var __cleanup(class_##name##_destructor)

#define INSTANTIATE_CLASS(name, var)					\
	__INSTANTIATE_VAR(name, var) = class_##name##_constructor


DEFINE_CLASS(fd, struct fd, fdput(THIS), f, struct fd f)

	INSTANTIATE_CLASS(fd, f)(perf_fget_light(fd));


Alternatively, you be OK with exposing INSTANTIATE_VAR() to easily
circumvent the default constructor?

And/or how about EXTEND_CLASS(), something like so?

#define EXTEND_CLASS(name, ext, init, init_args...)			\
typedef class_##name##_t class_##name##ext##_t;				\
static inline void class_##name##ext##_destructor(class_##name##_t *this) \
{ class_##name##_destructor(this); }					\
static inline type class_##name##ext##_constructor(init_args)		\
{ type THIS = init; return THIS; }


DEFINE_CLASS(fd, struct fd, fdput(THIS), fdget(fd), int fd)
EXTEND_CLASS(fd, _perf, perf_fget_light(fd), int fd)

	INSTANTIATE_CLASS(fd_perf, f)(fd);


> And at THAT point, you can do this:
> 
>     #define mutex_guard(m) \
>         INSTANTIATE_CLASS(mutex, __UNIQUE_ID(mutex))(m)
> 
> and now you can do
> 
>        mutex_guard(&sched_domains_mutex);

So the 'problem' is the amount of different guards I ended up having and
you can't have macro's define more macros :/

Which is how I ended up with the:

	guard(mutex, &sched_domains_mutex);

syntax.

This can ofcourse be achieved using the above CLASS thing like:

DEFINE_CLASS(mutex, struct mutex *, mutex_unlock(THIS),
	     ({ mutex_lock(m); m; }), struct mutex *m)

#define named_guard(name, var, args...)					\
	INSTANTIATE_CLASS(name, var)(args)

#define guard(name, args...)						\
	named_guard(name, __UNIQUE_ID(guard), args)

#define scoped_guard(name, args...)					\
	for (named_guard(name, scope, args),				\
	     *done = NULL; !done; done = (void *)1)


With the understanding they're only to be used for locks.

Also, I'm already tired of writing INSTANTIATE.. would:

	CLASS(fd, f)(fd);

	VAR(kfree, mem) = kzalloc_node(...);

be acceptable shorthand?

Re: [PATCH v2 0/2] Lock and Pointer guards

[Peter Zijlstra]

On Wed, Jun 07, 2023 at 11:41:01AM +0200, Peter Zijlstra wrote:


> > I'm sure there's something horribly wrong in the above, but my point
> > is that I'd really like this to make naming and conceptual sense.
> 
> Right, I hear ya. So the asymmetric case (iow destructor only) could be
> seen as using the copy-constructor.
> 
> #define DEFINE_CLASS(name, type, exit, init, init_args...)		\
> typedef type class_##name##_t;						\
> static inline void class_##name##_destructor(type *this)		\
> { type THIS = *this; exit; }						\
> static inline type class_##name##_constructor(init_args)		\
> { type THIS = init; return THIS; }
> 
> #define __INSTANTIATE_VAR(name, var)					\
> 	class_##name##_t var __cleanup(class_##name##_destructor)
> 
> #define INSTANTIATE_CLASS(name, var)					\
> 	__INSTANTIATE_VAR(name, var) = class_##name##_constructor
> 
> 
> DEFINE_CLASS(fd, struct fd, fdput(THIS), f, struct fd f)
> 
> 	INSTANTIATE_CLASS(fd, f)(perf_fget_light(fd));
> 
> 
> Alternatively, you be OK with exposing INSTANTIATE_VAR() to easily
> circumvent the default constructor?

Or perhaps use the smart-pointer concept applied to our classes like:

#define smart_ptr(name, var) \
	__INSTANTIATE_VAR(name, var)

To mean a pointer that calls the destructor for class 'name'. I think
the nearest thing C++ has is std::unique_ptr<>.


Then we can write:


DEFINE_CLASS(kfree, void *, kfree(THIS), p, void *p)


	smart_ptr(kfree, mem) = kzalloc_node(...);
	if (!mem)
		return -ENOMEM;

	object = mem;

	// further initiatlize object with error cases etc..

	mem = NULL; // success, we keep it.
	return object;

Re: [PATCH v2 0/2] Lock and Pointer guards

[Greg KH]

On Thu, Jun 08, 2023 at 10:52:48AM +0200, Peter Zijlstra wrote:
> On Wed, Jun 07, 2023 at 11:41:01AM +0200, Peter Zijlstra wrote:
> 
> 
> > > I'm sure there's something horribly wrong in the above, but my point
> > > is that I'd really like this to make naming and conceptual sense.
> > 
> > Right, I hear ya. So the asymmetric case (iow destructor only) could be
> > seen as using the copy-constructor.
> > 
> > #define DEFINE_CLASS(name, type, exit, init, init_args...)		\
> > typedef type class_##name##_t;						\
> > static inline void class_##name##_destructor(type *this)		\
> > { type THIS = *this; exit; }						\
> > static inline type class_##name##_constructor(init_args)		\
> > { type THIS = init; return THIS; }
> > 
> > #define __INSTANTIATE_VAR(name, var)					\
> > 	class_##name##_t var __cleanup(class_##name##_destructor)
> > 
> > #define INSTANTIATE_CLASS(name, var)					\
> > 	__INSTANTIATE_VAR(name, var) = class_##name##_constructor
> > 
> > 
> > DEFINE_CLASS(fd, struct fd, fdput(THIS), f, struct fd f)
> > 
> > 	INSTANTIATE_CLASS(fd, f)(perf_fget_light(fd));
> > 
> > 
> > Alternatively, you be OK with exposing INSTANTIATE_VAR() to easily
> > circumvent the default constructor?
> 
> Or perhaps use the smart-pointer concept applied to our classes like:
> 
> #define smart_ptr(name, var) \
> 	__INSTANTIATE_VAR(name, var)
> 
> To mean a pointer that calls the destructor for class 'name'. I think
> the nearest thing C++ has is std::unique_ptr<>.
> 
> 
> Then we can write:
> 
> 
> DEFINE_CLASS(kfree, void *, kfree(THIS), p, void *p)
> 
> 
> 	smart_ptr(kfree, mem) = kzalloc_node(...);
> 	if (!mem)
> 		return -ENOMEM;
> 
> 	object = mem;
> 
> 	// further initiatlize object with error cases etc..
> 
> 	mem = NULL; // success, we keep it.
> 	return object;

I like the idea, as we need a way to say "don't clean this up, it was
passed to somewhere else" for these types of allocations, but have it
"automatically" cleaned up on the error paths.

I have no say in the naming, though I always disliked the idea of a
pointer being "smart" as they are just a dumb memory register :)

thanks,

greg k-h

Re: [PATCH v2 0/2] Lock and Pointer guards

[Linus Torvalds]

On Thu, Jun 8, 2023 at 1:53 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> Or perhaps use the smart-pointer concept applied to our classes like:
>
> #define smart_ptr(name, var) \
>         __INSTANTIATE_VAR(name, var)

So this is the only situation where I think "ptr" makes sense (your
"fat pointer" argument is nonsensical - sure, you can treat anything
as a pointer if you're brave enough, but that just means that
"pointer" becomes a meaningless word).

However, I think that for "smart pointers", we don't need any of this
complexity at all, and we don't need that ugly syntax.

> Then we can write:
>
> 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.

And no, you do *not* want that "kfree(THIS)" kind of interface,
because you want the compiler to inline the freeing function wrapper,
and notice _statically_ when somebody zeroed the variable and not even
call "kfree()", because otherwise you'd have a pointless call to
kfree(NULL) in the success path too.

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

And yes, it needs a couple of different versions of that
"__automatic_kfree()" wrapper for the different freeing cases (kvfree,
rcu_free, whatever), but those are all trivial one-liners.

And no, I didn't think too much about those names. "__automatic_kfree"
is too damn long to type, but you hated "auto". And "no_free_ptr()" is
not wonderful name either. But I tried to make the naming at least be
obvious, if not wonderful.

               Linus

Re: [PATCH v2 0/2] Lock and Pointer guards

[Kees Cook]

On Thu, Jun 08, 2023 at 08:45:53AM -0700, Linus Torvalds wrote:
> 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);

I am a little worried about how (any version so far of) this API could go
wrong, e.g. if someone uses this and does "return p" instead of "return
no_free_ptr(p)", it'll return a freed pointer. I was hoping we could do
something like this to the end of automatic_kfree_wrapper():

	*(void **)pp = NULL;

i.e. if no_free_ptr() goes missing, "return p" will return NULL, which
is much easier to track down that dealing with later use-after-free bugs,
etc. Unfortunately, the __cleanup ordering is _after_ the compiler stores
the return value...

static inline void cleanup_info(struct info **p)
{
	free(*p);
	*p = NULL; /* this is effectively ignored */
}

struct info *do_something(int f)
{
	struct info *var __attribute__((__cleanup__(cleanup_info))) =
		malloc(1024);

	process(var);

	return var; /* oops, forgot to disable cleanup */
}

compile down to:

do_something:
        pushq   %rbx
        movl    $1024, %edi
        call    malloc
        movq    %rax, %rbx
        movq    %rax, %rdi
        call    process
        movq    %rbx, %rdi
        call    free
        movq    %rbx, %rax	; uses saved copy of malloc return
        popq    %rbx
        ret

The point being, if we can proactively make this hard to shoot ourselves in
the foot, that would be nice. :)

-- 
Kees Cook

Re: [PATCH v2 0/2] Lock and Pointer guards

[Linus Torvalds]

On Thu, Jun 8, 2023 at 9:47 AM Kees Cook <keescook@chromium.org> wrote:
>
> I am a little worried about how (any version so far of) this API could go
> wrong, e.g. if someone uses this and does "return p" instead of "return
> no_free_ptr(p)",

Absolutely. I think the whole "don't always cleanup" is quite
dangerous, and maybe not worth it, but it _is_ a fairly common
pattern.

>     I was hoping we could do
> something like this to the end of automatic_kfree_wrapper():
>
>        *(void **)pp = NULL;

That would be a lovely thing, but as you found out, it fundamentally
cannot work.

The whole point of "cleanup" is that it is done when the variable -
not trh *value* - goes out of scope.

So when you have that

        return var; /* oops, forgot to disable cleanup */

situation, by definition "var" hasn't gone out of scope until _after_
you read the value and return that value, so pretty much by definition
it cannot make a difference to assign something to 'pp' in the cleanup
code.

> The point being, if we can proactively make this hard to shoot ourselves in
> the foot, that would be nice. :)

So the good thing is that things like KASAN would immediately notice,
and since this all happens (presumably) for the success case, it's not
about some unlikely error case.

I also think that -fanalyzer might just catch it (as part of
-Wanalyzer-use-after-free) at compile-time, but I didn't check. So
if/when people start using -fanalyze on the kernel, those things would
be caught early.

So while this "forget to avoid cleanup" is a worry, I think it ends up
likely being one that is fairly easy to avoid with other checks in
place.

Famous last words.

             Linus

Re: [PATCH v2 0/2] Lock and Pointer guards

[Nick Desaulniers]

On Thu, Jun 8, 2023 at 9:47 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Jun 08, 2023 at 08:45:53AM -0700, Linus Torvalds wrote:
> > 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);
>
> I am a little worried about how (any version so far of) this API could go
> wrong, e.g. if someone uses this and does "return p" instead of "return
> no_free_ptr(p)", it'll return a freed pointer.

Presumably, one could simply just not use RAII(/SBRM someone else
corrected me about this recently coincidentally; I taught them my fun
C++ acronym IDGAF) when working with a value that conditionally
"escapes" the local scope.

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2 0/2] Lock and Pointer guards

[Peter Zijlstra]

On Thu, Jun 08, 2023 at 10:20:19AM -0700, Nick Desaulniers wrote:
> Presumably, one could simply just not use RAII(/SBRM someone else
> corrected me about this recently coincidentally; I taught them my fun
> C++ acronym IDGAF) when working with a value that conditionally
> "escapes" the local scope.

But then you're back to the error goto :/

Re: [PATCH v2 0/2] Lock and Pointer guards

[Nick Desaulniers]

On Thu, Jun 8, 2023 at 11:51 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Jun 08, 2023 at 10:20:19AM -0700, Nick Desaulniers wrote:
> > Presumably, one could simply just not use RAII when working with a value that conditionally
> > "escapes" the local scope.
>
> But then you're back to the error goto :/

Thinking more about the expected ergonomics here over lunch...no
meaningful insights, just thoughts...

For something like a mutex/lock; I'd expect those to be acquired then
released within the same function, yeah? In which case
__attribute__((cleanup())) has fewer hazards since the resource
doesn't escape.

For a pointer to a dynamically allocated region that may get returned
to the caller...

I mean, people do this in C++. It is safe and canonical to return a
std::unique_ptr.  When created locally the destructor does the
expected thing regardless of control flow.  IIUC, std::unique_ptr's
move constructor basically does what Kees suggested earlier (trigger
warning: C++): https://github.com/llvm/llvm-project/blob/7a52f79126a59717012d8039ef875f68e3c637fd/libcxx/include/__memory/unique_ptr.h#L429-L430.
example: https://godbolt.org/z/51s49G9f1

A recent commit to clang https://reviews.llvm.org/rG301eb6b68f30
raised an interesting point (deficiency is perhaps too strong a word)
about GNU-style attributes; they generally have no meaning on an ABI.

Consider a function that returns a locally constructed
std::unique_ptr.  If the function returns a type where the caller
knows what destructor functions to run.  This is part of the ABI.

Here, we're talking about using __attribute__((cleanup())) to DTR
locally, but then we return a "raw" pointer to a caller. What cleanup
function should the caller run, implicitly, if at all?  If we use
__attribute__((cleanup())) that saves us a few gotos locally, but the
caller perhaps now needs the same treatment.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2 0/2] Lock and Pointer guards

[Paolo Bonzini]

On 6/8/23 22:14, Nick Desaulniers wrote:
> Here, we're talking about using __attribute__((cleanup())) to DTR
> locally, but then we return a "raw" pointer to a caller. What cleanup
> function should the caller run, implicitly, if at all?  If we use
> __attribute__((cleanup())) that saves us a few gotos locally, but the
> caller perhaps now needs the same treatment.

But this is only a problem when you return a void*; and in general in C 
you will return a struct more often than a raw pointer (and in C++ you 
also have the issue of delete vs. delete[], that does not exist in C).

Returning a struct doesn't protect against use-after-free bugs in the 
way std::unique_ptr<> or Rust lifetimes do, but it at least tries to 
protect against calling the wrong cleanup function if you provide a 
typed "destructor" function that does the right thing---for example by 
handling reference counting or by freeing sub-structs before calling 
kfree/vfree.

Of course it's not a silver bullet, but then that's why people are 
looking into Rust for Linux.

Paolo

Re: [PATCH v2 0/2] Lock and Pointer guards

[Peter Zijlstra]

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.

Re: [PATCH v2 0/2] Lock and Pointer guards

[Linus Torvalds]

On Thu, Jun 8, 2023 at 1:06 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
>         struct obj *p __free(kfree) = kmalloc(...);

Yeah, the above actually looks really good to me - I like the naming
here, and the use looks very logical to me.

Of course, maybe once I see the patches that use this I go "uhh", but
at least for now I think you've hit on a rather legible syntax.

I'm still a bit unsure of the "no_free_ptr(p)" naming, but at least
it's pretty clear about what it does.

So my only worry is that it's not pretty and to the point like your
"__free(kfree)" syntax.

But it feels workable and not misleading, so unless somebody can come
up with a better name, I think it's ok.

           Linus

Re: [PATCH v2 0/2] Lock and Pointer guards

[Peter Zijlstra]

On Thu, Jun 08, 2023 at 07:25:27PM -0700, Linus Torvalds wrote:
> On Thu, Jun 8, 2023 at 1:06 PM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> >         struct obj *p __free(kfree) = kmalloc(...);
> 
> Yeah, the above actually looks really good to me - I like the naming
> here, and the use looks very logical to me.
> 
> Of course, maybe once I see the patches that use this I go "uhh", but
> at least for now I think you've hit on a rather legible syntax.
> 
> I'm still a bit unsure of the "no_free_ptr(p)" naming, but at least
> it's pretty clear about what it does.
> 
> So my only worry is that it's not pretty and to the point like your
> "__free(kfree)" syntax.
> 
> But it feels workable and not misleading, so unless somebody can come
> up with a better name, I think it's ok.

#define return_ptr(p) \
	return no_free_ptr(p)


	struct obj *p __free(kfree) = kmalloc(...);
	if (!p)
		return ERR_PTR(-ENOMEM);

	...

	return_ptr(p);

?

Re: [PATCH v2 0/2] Lock and Pointer guards

[Kees Cook]

On June 8, 2023 7:25:27 PM PDT, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>On Thu, Jun 8, 2023 at 1:06 PM Peter Zijlstra <peterz@infradead.org> wrote:
>>
>>         struct obj *p __free(kfree) = kmalloc(...);
>
>Yeah, the above actually looks really good to me - I like the naming
>here, and the use looks very logical to me.
>
>Of course, maybe once I see the patches that use this I go "uhh", but
>at least for now I think you've hit on a rather legible syntax.
>
>I'm still a bit unsure of the "no_free_ptr(p)" naming, but at least
>it's pretty clear about what it does.
>
>So my only worry is that it's not pretty and to the point like your
>"__free(kfree)" syntax.
>
>But it feels workable and not misleading, so unless somebody can come
>up with a better name, I think it's ok.

I like the proposed "take" naming, and before reading that reply I was going to suggest "keep". *shrug*


-- 
Kees Cook

Re: [PATCH v2 0/2] Lock and Pointer guards

[Rasmus Villemoes]

On 08/06/2023 17.45, Linus Torvalds wrote:

> And no, I didn't think too much about those names. "__automatic_kfree"
> is too damn long to type, but you hated "auto". And "no_free_ptr()" is
> not wonderful name either. But I tried to make the naming at least be
> obvious, if not wonderful.

No opinion either way, just throwing a data point out there:

So the systemd codebase makes quite heavy use of this automatic cleanup
stuff. Their name for no_free_ptr is TAKE_PTR(), and the verb "take"
apparently comes from Rust:

//// src/fundamental/macro-fundamental.h

/* Takes inspiration from Rust's Option::take() method: reads and
returns a pointer, but at the same time
 * resets it to NULL. See:
https://doc.rust-lang.org/std/option/enum.Option.html#method.take */
#define TAKE_GENERIC(var, type, nullvalue)                       \
        ({                                                       \
                type *_pvar_ = &(var);                           \
                type _var_ = *_pvar_;                            \
                type _nullvalue_ = nullvalue;                    \
                *_pvar_ = _nullvalue_;                           \
                _var_;                                           \
        })
#define TAKE_PTR_TYPE(ptr, type) TAKE_GENERIC(ptr, type, NULL)
#define TAKE_PTR(ptr) TAKE_PTR_TYPE(ptr, typeof(ptr))
#define TAKE_STRUCT_TYPE(s, type) TAKE_GENERIC(s, type, {})
#define TAKE_STRUCT(s) TAKE_STRUCT_TYPE(s, typeof(s))


and then they also have a

/* Like TAKE_PTR() but for file descriptors, resetting them to -EBADF */
#define TAKE_FD(fd) TAKE_GENERIC(fd, int, -EBADF)

with their "auto-close fd" helper of course knowing to ignore any
negative value.

Rasmus

Re: [PATCH v2 0/2] Lock and Pointer guards

[Kees Cook]

On Tue, Jun 06, 2023 at 11:42:51AM +0200, Peter Zijlstra wrote:
> [...]
> #define scoped_guard(_guard, _var...)                                  \
>        for (__ptr_guard(_guard, scope) = guard_init(_guard, _var),     \
>             *done = NULL; !done; done = (void *)1)

nit: Linus's example was "(void *)8" (instead of 1) because we've had
issues in the past with alignment warnings on archs that are sensitive
to it. (e.g. see the __is_constexpr() macro which is doing NULL/!NULL
comparisons.)

-- 
Kees Cook

Re: [PATCH v2 0/2] Lock and Pointer guards

[Linus Torvalds]

On Tue, Jun 6, 2023 at 8:31 AM Kees Cook <keescook@chromium.org> wrote:
>
> nit: Linus's example was "(void *)8" (instead of 1) because we've had
> issues in the past with alignment warnings on archs that are sensitive
> to it. (e.g. see the __is_constexpr() macro which is doing NULL/!NULL
> comparisons.)

Note that I don't think we ever saw such a warning, it was just a
theoretical observation that depending on type, the compiler might
warn about known mis-aligned pointer bits.

So I'm not sure the 1-vs-8 actually matters. We do other things that
assume that low bits in a pointer are retained and valid, even if in
theory the C type system might have issues with it.

But maybe I mis-remember - if you did get an actual warning, maybe we
should document that warning just to keep the memory alive.

            Linus

Re: [PATCH v2 0/2] Lock and Pointer guards

[Kees Cook]

On Tue, Jun 06, 2023 at 08:45:49AM -0700, Linus Torvalds wrote:
> On Tue, Jun 6, 2023 at 8:31 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > nit: Linus's example was "(void *)8" (instead of 1) because we've had
> > issues in the past with alignment warnings on archs that are sensitive
> > to it. (e.g. see the __is_constexpr() macro which is doing NULL/!NULL
> > comparisons.)
> 
> Note that I don't think we ever saw such a warning, it was just a
> theoretical observation that depending on type, the compiler might
> warn about known mis-aligned pointer bits.
> 
> So I'm not sure the 1-vs-8 actually matters. We do other things that
> assume that low bits in a pointer are retained and valid, even if in
> theory the C type system might have issues with it.
> 
> But maybe I mis-remember - if you did get an actual warning, maybe we
> should document that warning just to keep the memory alive.

I've never seen a warning, but since this came up in the dissection of
the __is_constexpr() behavior, it's been burned into my mind. ;)

-- 
Kees Cook

RE: [PATCH v2 0/2] Lock and Pointer guards

[David Laight]

From: Linus Torvalds
> Sent: 06 June 2023 16:46
> 
> On Tue, Jun 6, 2023 at 8:31 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > nit: Linus's example was "(void *)8" (instead of 1) because we've had
> > issues in the past with alignment warnings on archs that are sensitive
> > to it. (e.g. see the __is_constexpr() macro which is doing NULL/!NULL
> > comparisons.)

__is_constexpr() is playing entirely different games.
Basically the type of (x ? (void *)y : (int *)z)
depends on whether 'y' is a compile-time 0 (int *) or not (void *).

...
> So I'm not sure the 1-vs-8 actually matters. We do other things that
> assume that low bits in a pointer are retained and valid, even if in
> theory the C type system might have issues with it.

Yes, given that gcc will assume a pointer is aligned if you try
to memcpy() from it, I'm surprised it doesn't always assume that.
In which case (long)ptr_to_int & 3 can be validly assumed to be zero.

I've found some 'day job' code that passed the address of a
member of a 'packed' structure to a function which then used
host ordered unsigned char[] accesses.
The compiler is certainly allowed to convert that back to
a word write - which would then fault.
(I've not looked to see if any modern compilers do.)
Of course the simple ptr->member would have be fine.

	David

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