arm64: Fix __addr_ok and __range_ok macros

RFC PATCH In review Christopher Covington 4 participants 9 messages Mar 5, 2014 – Mar 20, 2014 arm64

lore

Share & tools

lore

Christopher Covington

author

2014-03-05 22:41 UTC #0 lore raw

RFC PATCH 1/1 arm64: Fix __addr_ok and __range_ok macros

Without this, the following scenario is incorrectly determined to be invalid.

addr 0x7f_ffffe000 size 8192 addr_limit 0x80_00000000

This behavior was observed while trying to vmsplice the stack as part of a CRIU dump of a process.

Signed-off-by Christopher Covington <[email protected]>

---
 arch/arm64/include/asm/uaccess.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

arch/arm64/include/asm/uaccess.h +4 -4

index edb3d5c..9309024 100644
@@ -66,12 +66,12 @@ static inline void set_fs(mm_segment_t fs)
 #define segment_eq(a,b)	((a) == (b))
 
 /*
- * Return 1 if addr < current->addr_limit, 0 otherwise.
+ * Return 1 if addr <= current->addr_limit, 0 otherwise.
  */
 #define __addr_ok(addr)							\
 ({									\
 	unsigned long flag;						\
-	asm("cmp %1, %0; cset %0, lo"					\
+	asm("cmp %1, %0; cset %0, ls"					\
 		: "=&r" (flag)						\
 		: "r" (addr), "0" (current_thread_info()->addr_limit)	\
 		: "cc");						\

@@ -83,7 +83,7 @@ static inline void set_fs(mm_segment_t fs)
  * Returns 1 if the range is valid, 0 otherwise.
  *
  * This is equivalent to the following test:
- * (u65)addr + (u65)size < (u65)current->addr_limit
+ * (u65)addr + (u65)size <= current->addr_limit
  *
  * This needs 65-bit arithmetic.
  */

@@ -91,7 +91,7 @@ static inline void set_fs(mm_segment_t fs)
 ({									\
 	unsigned long flag, roksum;					\
 	__chk_user_ptr(addr);						\
-	asm("adds %1, %1, %3; ccmp %1, %4, #2, cc; cset %0, cc"		\
+	asm("adds %1, %1, %3; ccmp %1, %4, #3, cc; cset %0, ls"		\
 		: "=&r" (flag), "=&r" (roksum)				\
 		: "1" (addr), "Ir" (size),				\
 		  "r" (current_thread_info()->addr_limit)		\

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by the Linux Foundation.

Michael S. Tsirkin

2014-03-06 08:20 UTC #1 lore raw

Quoted 22 lines · Christopher Covington

Without this, the following scenario is incorrectly determined to be invalid.
addr 0x7f_ffffe000 size 8192 addr_limit 0x80_00000000
This behavior was observed while trying to vmsplice the stack as part of a CRIU dump of a process.
Signed-off-by Christopher Covington <[email protected]>
---
 arch/arm64/include/asm/uaccess.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
arch/arm64/include/asm/uaccess.h +0 -1
index edb3d5c..9309024 100644
@@ -66,12 +66,12 @@ static inline void set_fs(mm_segment_t fs)
 #define segment_eq(a,b)	((a) == (b))
 
 /*
- * Return 1 if addr < current->addr_limit, 0 otherwise.

arch/arm64/include/asm/uaccess.h static inline void set_fs(mm_segment_t fs) · line 77

Quoted 10 lines

arch/arm64/include/asm/uaccess.h +2 -1

+ * Return 1 if addr <= current->addr_limit, 0 otherwise.
  */
 #define __addr_ok(addr)							\
 ({									\
 	unsigned long flag;						\
-	asm("cmp %1, %0; cset %0, lo"					\
+	asm("cmp %1, %0; cset %0, ls"					\
 		: "=&r" (flag)						\
 		: "r" (addr), "0" (current_thread_info()->addr_limit)	\
 		: "cc");						\

BTW can this use mov %0, #0 like arch/arm/include/asm/uaccess.h does? Would make it more portable ...

Quoted 21 lines

arch/arm64/include/asm/uaccess.h +2 -2

@@ -83,7 +83,7 @@ static inline void set_fs(mm_segment_t fs)
  * Returns 1 if the range is valid, 0 otherwise.
  *
  * This is equivalent to the following test:
- * (u65)addr + (u65)size < (u65)current->addr_limit
+ * (u65)addr + (u65)size <= current->addr_limit
  *
  * This needs 65-bit arithmetic.
  */

@@ -91,7 +91,7 @@ static inline void set_fs(mm_segment_t fs)
 ({									\
 	unsigned long flag, roksum;					\
 	__chk_user_ptr(addr);						\
-	asm("adds %1, %1, %3; ccmp %1, %4, #2, cc; cset %0, cc"		\
+	asm("adds %1, %1, %3; ccmp %1, %4, #3, cc; cset %0, ls"		\
 		: "=&r" (flag), "=&r" (roksum)				\
 		: "1" (addr), "Ir" (size),				\
 		  "r" (current_thread_info()->addr_limit)		\

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by the Linux Foundation.

Will Deacon

2014-03-06 16:08 UTC #2 lore raw

Quoted 21 lines · Michael S. Tsirkin

Quoted 20 lines · Christopher Covington
Without this, the following scenario is incorrectly determined to be invalid.
addr 0x7f_ffffe000 size 8192 addr_limit 0x80_00000000
This behavior was observed while trying to vmsplice the stack as part of a CRIU dump of a process.
Signed-off-by Christopher Covington <[email protected]>
---
 arch/arm64/include/asm/uaccess.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
arch/arm64/include/asm/uaccess.h +0 -0
index edb3d5c..9309024 100644
@@ -66,12 +66,12 @@ static inline void set_fs(mm_segment_t fs)
 #define segment_eq(a,b)	((a) == (b))
 

Quoted 12 lines

arch/arm64/include/asm/uaccess.h static inline void set_fs(mm_segment_t fs) · line 77

Quoted 12 lines

arch/arm64/include/asm/uaccess.h +2 -2

 /*
- * Return 1 if addr < current->addr_limit, 0 otherwise.
+ * Return 1 if addr <= current->addr_limit, 0 otherwise.
  */
 #define __addr_ok(addr)							\
 ({									\
 	unsigned long flag;						\
-	asm("cmp %1, %0; cset %0, lo"					\
+	asm("cmp %1, %0; cset %0, ls"					\
 		: "=&r" (flag)						\
 		: "r" (addr), "0" (current_thread_info()->addr_limit)	\
 		: "cc");						\

I don't think this is correct, since __addr_ok will now return true for TASK_SIZE_64.

Quoted 2 lines

BTW can this use mov %0, #0 like arch/arm/include/asm/uaccess.h does? Would make it more portable ...

How/why should this be made portable?

Quoted 10 lines

arch/arm64/include/asm/uaccess.h +1 -1

@@ -83,7 +83,7 @@ static inline void set_fs(mm_segment_t fs)
  * Returns 1 if the range is valid, 0 otherwise.
  *
  * This is equivalent to the following test:
- * (u65)addr + (u65)size < (u65)current->addr_limit
+ * (u65)addr + (u65)size <= current->addr_limit
  *
  * This needs 65-bit arithmetic.
  */

@@ -91,7 +91,7 @@ static inline void set_fs(mm_segment_t fs)

Quoted 8 lines

arch/arm64/include/asm/uaccess.h static inline void set_fs(mm_segment_t fs) · line 97

Quoted 8 lines

arch/arm64/include/asm/uaccess.h +1 -1

 ({									\
 	unsigned long flag, roksum;					\
 	__chk_user_ptr(addr);						\
-	asm("adds %1, %1, %3; ccmp %1, %4, #2, cc; cset %0, cc"		\
+	asm("adds %1, %1, %3; ccmp %1, %4, #3, cc; cset %0, ls"		\
 		: "=&r" (flag), "=&r" (roksum)				\
 		: "1" (addr), "Ir" (size),				\
 		  "r" (current_thread_info()->addr_limit)		\

Can't you just pass current_thread_info()->addr_limit) - 1 here and be done with it?

Will

Christopher Covington

author

2014-03-07 13:22 UTC #3 lore raw

In reply to Michael S. Tsirkin

Hi Michael,

Thanks for the comments.

Quoted 21 lines · Michael S. Tsirkin

Quoted 20 lines · Christopher Covington
Without this, the following scenario is incorrectly determined to be invalid.
addr 0x7f_ffffe000 size 8192 addr_limit 0x80_00000000
This behavior was observed while trying to vmsplice the stack as part of a CRIU dump of a process.
Signed-off-by Christopher Covington <[email protected]>
---
 arch/arm64/include/asm/uaccess.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
arch/arm64/include/asm/uaccess.h +0 -0
index edb3d5c..9309024 100644
@@ -66,12 +66,12 @@ static inline void set_fs(mm_segment_t fs)
 #define segment_eq(a,b)	((a) == (b))
 

Quoted 15 lines

arch/arm64/include/asm/uaccess.h static inline void set_fs(mm_segment_t fs) · line 77
Quoted 12 lines
arch/arm64/include/asm/uaccess.h +2 -2
 /*
- * Return 1 if addr < current->addr_limit, 0 otherwise.
+ * Return 1 if addr <= current->addr_limit, 0 otherwise.
  */
 #define __addr_ok(addr)							\
 ({									\
 	unsigned long flag;						\
-	asm("cmp %1, %0; cset %0, lo"					\
+	asm("cmp %1, %0; cset %0, ls"					\
 		: "=&r" (flag)						\
 		: "r" (addr), "0" (current_thread_info()->addr_limit)	\
 		: "cc");						\
BTW can this use mov %0, #0 like arch/arm/include/asm/uaccess.h does?

The A32 implementation uses "movlo", a conditional move instruction. My reading of section 3.2 Conditional Instructions of the ARMv8 ISA overview [1] and other documentation has led me to believe conditional move instructions as such are not available in A64, hence the choice of "cset".

1.
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.genc010197a/index.html

Christopher

Signature · 3 lines

Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by the Linux Foundation.

Catalin Marinas

maintainer

2014-03-13 11:20 UTC #4 lore raw

In reply to Christopher Covington

Quoted 7 lines · Christopher Covington

arch/arm64/include/asm/uaccess.h +0 -1

@@ -66,12 +66,12 @@ static inline void set_fs(mm_segment_t fs)
 #define segment_eq(a,b)	((a) == (b))
 
 /*
- * Return 1 if addr < current->addr_limit, 0 otherwise.

arch/arm64/include/asm/uaccess.h static inline void set_fs(mm_segment_t fs) · line 77

Quoted 10 lines

arch/arm64/include/asm/uaccess.h +2 -1

+ * Return 1 if addr <= current->addr_limit, 0 otherwise.
  */
 #define __addr_ok(addr)							\
 ({									\
 	unsigned long flag;						\
-	asm("cmp %1, %0; cset %0, lo"					\
+	asm("cmp %1, %0; cset %0, ls"					\
 		: "=&r" (flag)						\
 		: "r" (addr), "0" (current_thread_info()->addr_limit)	\
 		: "cc");						\

As Will said, this doesn't look right. Why do you need TASK_SIZE_64 to be valid?

Quoted 10 lines

arch/arm64/include/asm/uaccess.h +1 -1

@@ -83,7 +83,7 @@ static inline void set_fs(mm_segment_t fs)
  * Returns 1 if the range is valid, 0 otherwise.
  *
  * This is equivalent to the following test:
- * (u65)addr + (u65)size < (u65)current->addr_limit
+ * (u65)addr + (u65)size <= current->addr_limit
  *
  * This needs 65-bit arithmetic.
  */

arch/arm64/include/asm/uaccess.h static inline void set_fs(mm_segment_t fs) · line 97

Quoted 8 lines

arch/arm64/include/asm/uaccess.h +1 -1

@@ -91,7 +91,7 @@ static inline void set_fs(mm_segment_t fs)
 ({									\
 	unsigned long flag, roksum;					\
 	__chk_user_ptr(addr);						\
-	asm("adds %1, %1, %3; ccmp %1, %4, #2, cc; cset %0, cc"		\
+	asm("adds %1, %1, %3; ccmp %1, %4, #3, cc; cset %0, ls"		\
 		: "=&r" (flag), "=&r" (roksum)				\
 		: "1" (addr), "Ir" (size),				\
 		  "r" (current_thread_info()->addr_limit)		\

Just trying to understand: if adds does not set the C flag, we go on and do the ccmp. If addr + size <= addr_limit, "cset ls" sets the flag variable. If addr + size actually sets the C flag, we need to make sure that "cset ls" doesn't trigger, which would mean to set C flag and clear Z flag. So why do you change the ccmp flags from #2 to #3? It looks to me like #2 is enough.

Signature · 1 line

Catalin

Christopher Covington

author

2014-03-13 13:41 UTC #5 lore raw

Hi Catalin, Will,

Thanks for your feedback. I must admit I'm out of my depth here, so I just posted what I had, hoping to solicit comments like what you all have kindly provided.

Quoted 6 lines · Catalin Marinas

Quoted 5 lines · Christopher Covington
arch/arm64/include/asm/uaccess.h +0 -0
@@ -66,12 +66,12 @@ static inline void set_fs(mm_segment_t fs)
 #define segment_eq(a,b)	((a) == (b))
 

Quoted 15 lines

arch/arm64/include/asm/uaccess.h static inline void set_fs(mm_segment_t fs) · line 77
Quoted 12 lines
arch/arm64/include/asm/uaccess.h +2 -2
 /*
- * Return 1 if addr < current->addr_limit, 0 otherwise.
+ * Return 1 if addr <= current->addr_limit, 0 otherwise.
  */
 #define __addr_ok(addr)							\
 ({									\
 	unsigned long flag;						\
-	asm("cmp %1, %0; cset %0, lo"					\
+	asm("cmp %1, %0; cset %0, ls"					\
 		: "=&r" (flag)						\
 		: "r" (addr), "0" (current_thread_info()->addr_limit)	\
 		: "cc");						\
As Will said, this doesn't look right. Why do you need TASK_SIZE_64 to be valid?

I didn't encounter a case where this was necessary. I was just wondering if a change was needed for one macro, might it be needed for another? I'm now convinced that the answer is no.

Quoted 17 lines

arch/arm64/include/asm/uaccess.h +2 -2

@@ -83,7 +83,7 @@ static inline void set_fs(mm_segment_t fs)
  * Returns 1 if the range is valid, 0 otherwise.
  *
  * This is equivalent to the following test:
- * (u65)addr + (u65)size < (u65)current->addr_limit
+ * (u65)addr + (u65)size <= current->addr_limit
  *
  * This needs 65-bit arithmetic.
  */

@@ -91,7 +91,7 @@ static inline void set_fs(mm_segment_t fs)
 ({									\
 	unsigned long flag, roksum;					\
 	__chk_user_ptr(addr);						\
-	asm("adds %1, %1, %3; ccmp %1, %4, #2, cc; cset %0, cc"		\
+	asm("adds %1, %1, %3; ccmp %1, %4, #3, cc; cset %0, ls"		\
 		: "=&r" (flag), "=&r" (roksum)				\
 		: "1" (addr), "Ir" (size),				\

Quoted 8 lines

arch/arm64/include/asm/uaccess.h static inline void set_fs(mm_segment_t fs) · line 97
Quoted 1 line
arch/arm64/include/asm/uaccess.h +0 -0
 		  "r" (current_thread_info()->addr_limit)		\
Just trying to understand: if adds does not set the C flag, we go on and do the ccmp. If addr + size <= addr_limit, "cset ls" sets the flag variable. If addr + size actually sets the C flag, we need to make sure that "cset ls" doesn't trigger, which would mean to set C flag and clear Z flag. So why do you change the ccmp flags from #2 to #3? It looks to me like #2 is enough.

#2 is indeed sufficient. I'll respin using it.

I think Will's suggested approach could also work but I figure since I've taken the time to understand the assembly I might as well fix the problem there rather than adding another step in the calculation for developers and compilers to parse. (I don't know if this code is performance critical, but I nevertheless wanted to see how the compiler handled Will's approach. Unfortunately my initial implementation resulted in unaligned opcode errors and I haven't yet dug in.)

Thanks,
Christopher

Signature · 3 lines

Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by the Linux Foundation.

Catalin Marinas

maintainer

2014-03-13 15:53 UTC #6 lore raw

Quoted 30 lines · Christopher Covington

Quoted 18 lines · Catalin Marinas
Quoted 17 lines · Christopher Covington
arch/arm64/include/asm/uaccess.h +2 -2
@@ -83,7 +83,7 @@ static inline void set_fs(mm_segment_t fs)
  * Returns 1 if the range is valid, 0 otherwise.
  *
  * This is equivalent to the following test:
- * (u65)addr + (u65)size < (u65)current->addr_limit
+ * (u65)addr + (u65)size <= current->addr_limit
  *
  * This needs 65-bit arithmetic.
  */
@@ -91,7 +91,7 @@ static inline void set_fs(mm_segment_t fs)
 ({									\
 	unsigned long flag, roksum;					\
 	__chk_user_ptr(addr);						\
-	asm("adds %1, %1, %3; ccmp %1, %4, #2, cc; cset %0, cc"		\
+	asm("adds %1, %1, %3; ccmp %1, %4, #3, cc; cset %0, ls"		\
 		: "=&r" (flag), "=&r" (roksum)				\
 		: "1" (addr), "Ir" (size),				\
Quoted 8 lines
arch/arm64/include/asm/uaccess.h static inline void set_fs(mm_segment_t fs) · line 97
Quoted 1 line
arch/arm64/include/asm/uaccess.h +0 -0
 		  "r" (current_thread_info()->addr_limit)		\
Just trying to understand: if adds does not set the C flag, we go on and do the ccmp. If addr + size <= addr_limit, "cset ls" sets the flag variable. If addr + size actually sets the C flag, we need to make sure that "cset ls" doesn't trigger, which would mean to set C flag and clear Z flag. So why do you change the ccmp flags from #2 to #3? It looks to me like #2 is enough.
#2 is indeed sufficient. I'll respin using it.

Quoted 7 lines

I think Will's suggested approach could also work but I figure since I've taken the time to understand the assembly I might as well fix the problem there rather than adding another step in the calculation for developers and compilers to parse. (I don't know if this code is performance critical, but I nevertheless wanted to see how the compiler handled Will's approach. Unfortunately my initial implementation resulted in unaligned opcode errors and I haven't yet dug in.)

If it's only one condition change, I would prefer the inline asm fix. I haven't done any benchmarks with a C-only implementation to assess the impact.

For __addr_ok() I think the compiler should be good enough as we don't need 65-bit arithmetics but we can leave it as it is.

Signature · 1 line

Catalin

Christopher Covington

author

2014-03-19 16:29 UTC #7 lore raw

PATCH v2 1/1 arm64: Fix __range_ok macro

In reply to Christopher Covington

Without this, the following scenario is incorrectly determined to be invalid.

addr 0x7f_ffffe000 size 8192 addr_limit 0x80_00000000

This behavior was observed while trying to vmsplice the stack as part of a CRIU dump of a process on a system started with the norandmaps kernel parameter.

Signed-off-by Christopher Covington <[email protected]>

---
 arch/arm64/include/asm/uaccess.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/arm64/include/asm/uaccess.h +2 -2

index 7ecc2b2..5974459 100644
@@ -83,7 +83,7 @@ static inline void set_fs(mm_segment_t fs)
  * Returns 1 if the range is valid, 0 otherwise.
  *
  * This is equivalent to the following test:
- * (u65)addr + (u65)size < (u65)current->addr_limit
+ * (u65)addr + (u65)size <= current->addr_limit
  *
  * This needs 65-bit arithmetic.
  */

@@ -91,7 +91,7 @@ static inline void set_fs(mm_segment_t fs)
 ({									\
 	unsigned long flag, roksum;					\
 	__chk_user_ptr(addr);						\
-	asm("adds %1, %1, %3; ccmp %1, %4, #2, cc; cset %0, cc"		\
+	asm("adds %1, %1, %3; ccmp %1, %4, #2, cc; cset %0, ls"		\
 		: "=&r" (flag), "=&r" (roksum)				\
 		: "1" (addr), "Ir" (size),				\
 		  "r" (current_thread_info()->addr_limit)		\

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by the Linux Foundation.

Catalin Marinas

maintainer Applied

2014-03-20 17:42 UTC #8 lore raw

Re: [PATCH v2] arm64: Fix __range_ok macro

Quoted 10 lines · Christopher Covington

Without this, the following scenario is incorrectly determined to be invalid.
addr 0x7f_ffffe000 size 8192 addr_limit 0x80_00000000
This behavior was observed while trying to vmsplice the stack as part of a CRIU dump of a process on a system started with the norandmaps kernel parameter.
Signed-off-by Christopher Covington <[email protected]>

Thanks, applied.

Signature · 1 line

Catalin