* [RFC PATCH] arm64: Fix __addr_ok and __range_ok macros
2014-03-05 22:41 [RFC PATCH] arm64: Fix __addr_ok and __range_ok macros Christopher Covington
@ 2014-03-06 8:20 ` Michael S. Tsirkin
2014-03-06 16:08 ` Will Deacon
2014-03-07 13:22 ` Christopher Covington
2014-03-13 11:20 ` Catalin Marinas
2014-03-19 16:29 ` [PATCH v2] arm64: Fix __range_ok macro Christopher Covington
2 siblings, 2 replies; 9+ messages in thread
From: Michael S. Tsirkin @ 2014-03-06 8:20 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Mar 05, 2014 at 05:41:28PM -0500, Christopher Covington wrote:
> 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 <cov@codeaurora.org>
> ---
> arch/arm64/include/asm/uaccess.h | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> index edb3d5c..9309024 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -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"); \
BTW can this use mov %0, #0 like arch/arm/include/asm/uaccess.h does?
Would make it more portable ...
> @@ -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) \
> --
> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by the Linux Foundation.
^ permalink raw reply [flat|nested] 9+ messages in thread* [RFC PATCH] arm64: Fix __addr_ok and __range_ok macros
2014-03-06 8:20 ` Michael S. Tsirkin
@ 2014-03-06 16:08 ` Will Deacon
2014-03-07 13:22 ` Christopher Covington
1 sibling, 0 replies; 9+ messages in thread
From: Will Deacon @ 2014-03-06 16:08 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Mar 06, 2014 at 08:20:23AM +0000, Michael S. Tsirkin wrote:
> On Wed, Mar 05, 2014 at 05:41:28PM -0500, Christopher Covington wrote:
> > 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 <cov@codeaurora.org>
> > ---
> > arch/arm64/include/asm/uaccess.h | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> > index edb3d5c..9309024 100644
> > --- a/arch/arm64/include/asm/uaccess.h
> > +++ b/arch/arm64/include/asm/uaccess.h
> > @@ -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"); \
I don't think this is correct, since __addr_ok will now return true for
TASK_SIZE_64.
> 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?
> > @@ -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) \
Can't you just pass current_thread_info()->addr_limit) - 1 here and be done
with it?
Will
^ permalink raw reply [flat|nested] 9+ messages in thread* [RFC PATCH] arm64: Fix __addr_ok and __range_ok macros
2014-03-06 8:20 ` Michael S. Tsirkin
2014-03-06 16:08 ` Will Deacon
@ 2014-03-07 13:22 ` Christopher Covington
1 sibling, 0 replies; 9+ messages in thread
From: Christopher Covington @ 2014-03-07 13:22 UTC (permalink / raw)
To: linux-arm-kernel
Hi Michael,
Thanks for the comments.
On 03/06/2014 03:20 AM, Michael S. Tsirkin wrote:
> On Wed, Mar 05, 2014 at 05:41:28PM -0500, Christopher Covington wrote:
>> 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 <cov@codeaurora.org>
>> ---
>> arch/arm64/include/asm/uaccess.h | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
>> index edb3d5c..9309024 100644
>> --- a/arch/arm64/include/asm/uaccess.h
>> +++ b/arch/arm64/include/asm/uaccess.h
>> @@ -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"); \
>
>
> 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
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by the Linux Foundation.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [RFC PATCH] arm64: Fix __addr_ok and __range_ok macros
2014-03-05 22:41 [RFC PATCH] arm64: Fix __addr_ok and __range_ok macros Christopher Covington
2014-03-06 8:20 ` Michael S. Tsirkin
@ 2014-03-13 11:20 ` Catalin Marinas
2014-03-13 13:41 ` Christopher Covington
2014-03-19 16:29 ` [PATCH v2] arm64: Fix __range_ok macro Christopher Covington
2 siblings, 1 reply; 9+ messages in thread
From: Catalin Marinas @ 2014-03-13 11:20 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Mar 05, 2014 at 10:41:28PM +0000, Christopher Covington wrote:
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -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"); \
As Will said, this doesn't look right. Why do you need TASK_SIZE_64 to
be valid?
> @@ -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) \
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.
--
Catalin
^ permalink raw reply [flat|nested] 9+ messages in thread* [RFC PATCH] arm64: Fix __addr_ok and __range_ok macros
2014-03-13 11:20 ` Catalin Marinas
@ 2014-03-13 13:41 ` Christopher Covington
2014-03-13 15:53 ` Catalin Marinas
0 siblings, 1 reply; 9+ messages in thread
From: Christopher Covington @ 2014-03-13 13:41 UTC (permalink / raw)
To: linux-arm-kernel
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.
On 03/13/2014 07:20 AM, Catalin Marinas wrote:
> On Wed, Mar 05, 2014 at 10:41:28PM +0000, Christopher Covington wrote:
>> --- a/arch/arm64/include/asm/uaccess.h
>> +++ b/arch/arm64/include/asm/uaccess.h
>> @@ -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"); \
>
> 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.
>> @@ -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) \
>
> 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
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by the Linux Foundation.
^ permalink raw reply [flat|nested] 9+ messages in thread* [RFC PATCH] arm64: Fix __addr_ok and __range_ok macros
2014-03-13 13:41 ` Christopher Covington
@ 2014-03-13 15:53 ` Catalin Marinas
0 siblings, 0 replies; 9+ messages in thread
From: Catalin Marinas @ 2014-03-13 15:53 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Mar 13, 2014 at 01:41:01PM +0000, Christopher Covington wrote:
> On 03/13/2014 07:20 AM, Catalin Marinas wrote:
> > On Wed, Mar 05, 2014 at 10:41:28PM +0000, Christopher Covington wrote:
> >> @@ -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) \
> >
> > 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.)
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.
--
Catalin
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] arm64: Fix __range_ok macro
2014-03-05 22:41 [RFC PATCH] arm64: Fix __addr_ok and __range_ok macros Christopher Covington
2014-03-06 8:20 ` Michael S. Tsirkin
2014-03-13 11:20 ` Catalin Marinas
@ 2014-03-19 16:29 ` Christopher Covington
2014-03-20 17:42 ` Catalin Marinas
2 siblings, 1 reply; 9+ messages in thread
From: Christopher Covington @ 2014-03-19 16:29 UTC (permalink / raw)
To: linux-arm-kernel
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 <cov@codeaurora.org>
---
arch/arm64/include/asm/uaccess.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index 7ecc2b2..5974459 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -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) \
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by the Linux Foundation.
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v2] arm64: Fix __range_ok macro
2014-03-19 16:29 ` [PATCH v2] arm64: Fix __range_ok macro Christopher Covington
@ 2014-03-20 17:42 ` Catalin Marinas
0 siblings, 0 replies; 9+ messages in thread
From: Catalin Marinas @ 2014-03-20 17:42 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Mar 19, 2014 at 04:29:37PM +0000, Christopher Covington wrote:
> 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 <cov@codeaurora.org>
Thanks, applied.
--
Catalin
^ permalink raw reply [flat|nested] 9+ messages in thread