Re: A few proposals, this time from the C++ standards committee

["Paul E. McKenney" <paulmck@kernel.org>]

On Sun, Mar 17, 2024 at 02:44:09PM -0700, Linus Torvalds wrote:
> On Sun, 17 Mar 2024 at 13:50, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Looking closer at this one, it seems to be purely a compiler bug.
> 
> Side note: it may be that all that protects us in the kernel from this
> compiler bug is the fact that we do not let the compiler know that
> "kmalloc()" returns some private memory. So for that particular
> pattern, the compiler doesn't actuially know that 'p' is some private
> pointer and not visible to anybody else.

Sadly, we really do let the compiler know:

static __always_inline __alloc_size(1) void *kmalloc(size_t size, gfp_t flags)

#define __alloc_size(x, ...) __alloc_size__(x, ## __VA_ARGS__) __malloc

#define __malloc                        __attribute__((__malloc__))

Maybe we should stop doing so:

------------------------------------------------------------------------

diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
index 28566624f008f..7b4db0cd093a2 100644
--- a/include/linux/compiler_attributes.h
+++ b/include/linux/compiler_attributes.h
@@ -181,7 +181,7 @@
  *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-malloc-function-attribute
  * clang: https://clang.llvm.org/docs/AttributeReference.html#malloc
  */
-#define __malloc                        __attribute__((__malloc__))
+#define __malloc
 
 /*
  *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html#index-mode-type-attribute

------------------------------------------------------------------------

> In C++, particularly with 'new', the compiler might be much more aware
> of the fact that nobody can possibly see 'p' outside of that function
> until after the return.
> 
> So a scarier example without that kind of issue might be something like this:
> 
>     extern void unlock(void);
>     extern void lock(void);
>     extern void wait_for_x(int *);
> 
>     void buggy(void)
>     {
>         int p = 5;
>         unlock();
>         wait_for_x(&p);
>         lock();
>     }
> 
> where the basic theory of operation is that we're calling that
> function with a lock held, and then that "wait_for_x()" thing does
> something that exposes the value and waits for it to be changed.
> 
> And at the time of the "unlock()", a buggy compiler *might* think that
> the value of "p" is entirely private to that function, so the compiler
> might decide to compile this as
> 
>  - call "unlock()"
> 
>  - *then* set 'p' to 5, and pass off the address to the wait function
> 
> and that is very buggy on weakly ordered machines for the very same
> reasons that that github issue was raised - it is re-ordering the
> store wrt the store-release inherent in the 'unlock()'.
> 
> Now, in my quick tests, that doesn't actually happen. I sincerely hope
> it is because the compiler sees "Oh, somebody is taking the address of
> 'p'" and just the act of that address-of will make the compiler know
> that it has to serialize stores to 'p' with any function calls - even
> function calls that happen before the address is taken.
> 
> But that
> 
>      https://github.com/llvm/llvm-project/issues/64188
> 
> that you linked to certainly seems to imply that some versions of
> clang have made the equivalent of that mistake, and could possibly
> hoist the assignment to 'p' to after the 'unlock()' call.

Yes, it really does happen in some cases.

And I agree that there are likely a great many failure cases.

The initial examples were user error where the pointer was handed off to
some other thread without synchronizing the lifetime of the pointed-to
object.  I chastised them for this, and they eventually came up with
the external function hiding the atomic_thread_fence() from the compiler.

							Thanx, Paul