Re: [PATCH 0/2] fix vma->anon_vma check for per-VMA locking; fix anon_vma memory ordering

[Nadav Amit]

> On Jul 27, 2023, at 1:11 PM, Nadav Amit <nadav.amit@xxxxxxxxx> wrote:
> 
> 
> 
>> On Jul 27, 2023, at 12:39 PM, Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxx> wrote:
>> 
>> On Thu, 27 Jul 2023 at 12:10, Nadav Amit <nadav.amit@xxxxxxxxx> wrote:
>>> 
>>> Interesting. I wonder if you considered adding to READ_ONCE() something
>>> like:
>>> 
>>>       asm volatile("" : "+g" (x) );
>>> 
>>> So later loads (such as baz = *ptr) would reload the updated value.
>> 
>> Not necessarily a bad idea.  Although I suspect you'd want to add
>> *two* of them - on either side - to make sure any previous loads
>> wouldn't be moved around it either.
> 
> You are right, two are needed.
> 
> I’ll give it a shot and see if I see changes to the binary.

Just for the record (so nobody will make my mistakes):

I implemented it, but it works poorly. It appears to communicate the
constraints but the generated code is much worse.

The problem is that if the GCC inline asm decides to use a memory operand
(e.g. “+m”), it computes the address (uses LEA first before the MOV) and
allocates a register for the address. Using “+g” also causes the compiler
to allocate a register.

-- >8 --

---
 include/asm-generic/rwonce.h | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/include/asm-generic/rwonce.h b/include/asm-generic/rwonce.h
index 8d0a6280e982..c6a2f8e3013c 100644
--- a/include/asm-generic/rwonce.h
+++ b/include/asm-generic/rwonce.h
@@ -44,10 +44,34 @@
 #define __READ_ONCE(x)	(*(const volatile __unqual_scalar_typeof(x) *)&(x))
 #endif
 
-#define READ_ONCE(x)							\
-({									\
-	compiletime_assert_rwonce_type(x);				\
-	__READ_ONCE(x);							\
+#define as_scalar(x)							\
+    __builtin_choose_expr(sizeof(x) == sizeof(char),  			\
+				(__force char *)&(x), 			\
+    __builtin_choose_expr(sizeof(x) == sizeof(short),			\
+				(__force short *)&(x),			\
+    __builtin_choose_expr(sizeof(x) == sizeof(int),			\
+				(__force int *)&(x), 			\
+    __builtin_choose_expr(sizeof(x) == sizeof(long),			\
+				(__force long *)&(x),			\
+    __builtin_choose_expr(sizeof(x) == sizeof(long long),		\
+				(__force long long *)&(x),		\
+    (void*)0)))))
+
+#define READ_ONCE(x)                                                    \
+({                                                                      \
+    typeof(x) * px = &(x);						\
+    union {								\
+        typeof(x) __orig;						\
+        typeof(*as_scalar(x)) __alias;					\
+    } __u;								\
+									\
+    compiletime_assert_rwonce_type(x);					\
+    asm volatile ("" :							\
+               "+g" (*(__force typeof(*as_scalar(x)) *)(px)));		\
+    __u.__alias = __READ_ONCE(*as_scalar(*px));				\
+    asm volatile ("" :							\
+               "+g" (*(__force typeof(*as_scalar(x)) *)(px)));		\
+    __u.__orig;								\
 })
 
 #define __WRITE_ONCE(x, val)						\
-- 
2.34.1