[PATCH] arm64: fix add kasan bug

[PATCH] arm64: fix add kasan bug

[zhongjiang]

From: zhong jiang <zhongjiang@huawei.com>

In general, each process have 16kb stack space to use, but
stack need extra space to store red_zone when kasan enable.
the patch fix above question.

Signed-off-by: zhong jiang <zhongjiang@huawei.com>
---
 arch/arm64/include/asm/thread_info.h | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
index 90c7ff2..45b5a7e 100644
--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h
@@ -23,13 +23,24 @@
 
 #include <linux/compiler.h>
 
+#ifdef CONFIG_KASAN
+#define KASAN_STACK_ORDER 1
+#else
+#define KASAN_STACK_ORDER 0
+#endif
+
 #ifdef CONFIG_ARM64_4K_PAGES
-#define THREAD_SIZE_ORDER	2
+#define THREAD_SIZE_ORDER	(2 + KASAN_STACK_ORDER)
 #elif defined(CONFIG_ARM64_16K_PAGES)
-#define THREAD_SIZE_ORDER	0
+#define THREAD_SIZE_ORDER	(0 + KASAN_STACK_ORDER)
 #endif
 
+#ifdef CONFIG_KASAN
+#define THREAD_SIZE		32768
+#else
 #define THREAD_SIZE		16384
+#endif
+
 #define THREAD_START_SP		(THREAD_SIZE - 16)
 
 #ifndef __ASSEMBLY__
-- 
2.0.0

[PATCH] arm64: fix add kasan bug

[zhongjiang]

From: zhong jiang <zhongjiang@huawei.com>

In general, each process have 16kb stack space to use, but
stack need extra space to store red_zone when kasan enable.
the patch fix above question.

Signed-off-by: zhong jiang <zhongjiang@huawei.com>
---
 arch/arm64/include/asm/thread_info.h | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
index 90c7ff2..45b5a7e 100644
--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h
@@ -23,13 +23,24 @@
 
 #include <linux/compiler.h>
 
+#ifdef CONFIG_KASAN
+#define KASAN_STACK_ORDER 1
+#else
+#define KASAN_STACK_ORDER 0
+#endif
+
 #ifdef CONFIG_ARM64_4K_PAGES
-#define THREAD_SIZE_ORDER	2
+#define THREAD_SIZE_ORDER	(2 + KASAN_STACK_ORDER)
 #elif defined(CONFIG_ARM64_16K_PAGES)
-#define THREAD_SIZE_ORDER	0
+#define THREAD_SIZE_ORDER	(0 + KASAN_STACK_ORDER)
 #endif
 
+#ifdef CONFIG_KASAN
+#define THREAD_SIZE		32768
+#else
 #define THREAD_SIZE		16384
+#endif
+
 #define THREAD_START_SP		(THREAD_SIZE - 16)
 
 #ifndef __ASSEMBLY__
-- 
2.0.0

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] arm64: fix add kasan bug

[Andrew Morton]

On Thu, 31 Dec 2015 18:09:09 +0800 zhongjiang <zhongjiang@huawei.com> wrote:

> From: zhong jiang <zhongjiang@huawei.com>
> 
> In general, each process have 16kb stack space to use, but
> stack need extra space to store red_zone when kasan enable.
> the patch fix above question.

Thanks.  I grabbed this, but would prefer that the arm64 people handle
it?

Re: [PATCH] arm64: fix add kasan bug

[Andrew Morton]

On Thu, 31 Dec 2015 18:09:09 +0800 zhongjiang <zhongjiang@huawei.com> wrote:

> From: zhong jiang <zhongjiang@huawei.com>
> 
> In general, each process have 16kb stack space to use, but
> stack need extra space to store red_zone when kasan enable.
> the patch fix above question.

Thanks.  I grabbed this, but would prefer that the arm64 people handle
it?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] arm64: fix add kasan bug

[Catalin Marinas]

On Thu, Dec 31, 2015 at 10:09:09AM +0000, zhongjiang wrote:
> From: zhong jiang <zhongjiang@huawei.com>
> 
> In general, each process have 16kb stack space to use, but
> stack need extra space to store red_zone when kasan enable.
> the patch fix above question.
> 
> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
> ---
>  arch/arm64/include/asm/thread_info.h | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
> index 90c7ff2..45b5a7e 100644
> --- a/arch/arm64/include/asm/thread_info.h
> +++ b/arch/arm64/include/asm/thread_info.h
[...]
> +#ifdef CONFIG_KASAN
> +#define THREAD_SIZE		32768
> +#else
>  #define THREAD_SIZE		16384
> +#endif

I'm not really keen on increasing the stack size to 32KB when KASan is
enabled (that's 8 4K pages). Have you actually seen a real problem with
the default size? How large is the red_zone?

With 4.5 we are going for separate IRQ stack on arm64, so the typical
stack overflow case no longer exists.

-- 
Catalin

Re: [PATCH] arm64: fix add kasan bug

[Catalin Marinas]

On Thu, Dec 31, 2015 at 10:09:09AM +0000, zhongjiang wrote:
> From: zhong jiang <zhongjiang@huawei.com>
> 
> In general, each process have 16kb stack space to use, but
> stack need extra space to store red_zone when kasan enable.
> the patch fix above question.
> 
> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
> ---
>  arch/arm64/include/asm/thread_info.h | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
> index 90c7ff2..45b5a7e 100644
> --- a/arch/arm64/include/asm/thread_info.h
> +++ b/arch/arm64/include/asm/thread_info.h
[...]
> +#ifdef CONFIG_KASAN
> +#define THREAD_SIZE		32768
> +#else
>  #define THREAD_SIZE		16384
> +#endif

I'm not really keen on increasing the stack size to 32KB when KASan is
enabled (that's 8 4K pages). Have you actually seen a real problem with
the default size? How large is the red_zone?

With 4.5 we are going for separate IRQ stack on arm64, so the typical
stack overflow case no longer exists.

-- 
Catalin

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] arm64: fix add kasan bug

[Catalin Marinas]

On Mon, Jan 04, 2016 at 01:13:33PM -0800, Andrew Morton wrote:
> On Thu, 31 Dec 2015 18:09:09 +0800 zhongjiang <zhongjiang@huawei.com> wrote:
> 
> > From: zhong jiang <zhongjiang@huawei.com>
> > 
> > In general, each process have 16kb stack space to use, but
> > stack need extra space to store red_zone when kasan enable.
> > the patch fix above question.
> 
> Thanks.  I grabbed this, but would prefer that the arm64 people handle
> it?

I would also prefer taking such fix via the arm64 tree, though we are
currently still going through the post-holiday email backlog.

-- 
Catalin

Re: [PATCH] arm64: fix add kasan bug

[Catalin Marinas]

On Mon, Jan 04, 2016 at 01:13:33PM -0800, Andrew Morton wrote:
> On Thu, 31 Dec 2015 18:09:09 +0800 zhongjiang <zhongjiang@huawei.com> wrote:
> 
> > From: zhong jiang <zhongjiang@huawei.com>
> > 
> > In general, each process have 16kb stack space to use, but
> > stack need extra space to store red_zone when kasan enable.
> > the patch fix above question.
> 
> Thanks.  I grabbed this, but would prefer that the arm64 people handle
> it?

I would also prefer taking such fix via the arm64 tree, though we are
currently still going through the post-holiday email backlog.

-- 
Catalin

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] arm64: fix add kasan bug

[Andrey Ryabinin]

2016-01-05 13:10 GMT+03:00 Catalin Marinas <catalin.marinas@arm.com>:
> On Thu, Dec 31, 2015 at 10:09:09AM +0000, zhongjiang wrote:
>> From: zhong jiang <zhongjiang@huawei.com>
>>
>> In general, each process have 16kb stack space to use, but
>> stack need extra space to store red_zone when kasan enable.
>> the patch fix above question.
>>
>> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
>> ---
>>  arch/arm64/include/asm/thread_info.h | 15 +++++++++++++--
>>  1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
>> index 90c7ff2..45b5a7e 100644
>> --- a/arch/arm64/include/asm/thread_info.h
>> +++ b/arch/arm64/include/asm/thread_info.h
> [...]
>> +#ifdef CONFIG_KASAN
>> +#define THREAD_SIZE          32768
>> +#else
>>  #define THREAD_SIZE          16384
>> +#endif
>
> I'm not really keen on increasing the stack size to 32KB when KASan is
> enabled (that's 8 4K pages). Have you actually seen a real problem with
> the default size?

> How large is the red_zone?
>

Typical stack frame layout looks like this:
    | 32-byte redzone | variable-1| padding-redzone to the next
32-byte boundary| variable-2|padding |.... | 32-byte redzone|

AFAIK gcc creates redzones  only if it can't prove that all accesses
to variable are valid (e.g. reference to variable passed to external
function).
Besides redzones, stack could be increased due to additional spilling.
Although arm64 should be less affected by this since it has more
registers than x86_64.
On x86_64 I've seen few bad cases where stack frame of a single
function was bloated up to 6K.


> With 4.5 we are going for separate IRQ stack on arm64, so the typical
> stack overflow case no longer exists.
>
> --
> Catalin

Re: [PATCH] arm64: fix add kasan bug

[Andrey Ryabinin]

2016-01-05 13:10 GMT+03:00 Catalin Marinas <catalin.marinas@arm.com>:
> On Thu, Dec 31, 2015 at 10:09:09AM +0000, zhongjiang wrote:
>> From: zhong jiang <zhongjiang@huawei.com>
>>
>> In general, each process have 16kb stack space to use, but
>> stack need extra space to store red_zone when kasan enable.
>> the patch fix above question.
>>
>> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
>> ---
>>  arch/arm64/include/asm/thread_info.h | 15 +++++++++++++--
>>  1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
>> index 90c7ff2..45b5a7e 100644
>> --- a/arch/arm64/include/asm/thread_info.h
>> +++ b/arch/arm64/include/asm/thread_info.h
> [...]
>> +#ifdef CONFIG_KASAN
>> +#define THREAD_SIZE          32768
>> +#else
>>  #define THREAD_SIZE          16384
>> +#endif
>
> I'm not really keen on increasing the stack size to 32KB when KASan is
> enabled (that's 8 4K pages). Have you actually seen a real problem with
> the default size?

> How large is the red_zone?
>

Typical stack frame layout looks like this:
    | 32-byte redzone | variable-1| padding-redzone to the next
32-byte boundary| variable-2|padding |.... | 32-byte redzone|

AFAIK gcc creates redzones  only if it can't prove that all accesses
to variable are valid (e.g. reference to variable passed to external
function).
Besides redzones, stack could be increased due to additional spilling.
Although arm64 should be less affected by this since it has more
registers than x86_64.
On x86_64 I've seen few bad cases where stack frame of a single
function was bloated up to 6K.


> With 4.5 we are going for separate IRQ stack on arm64, so the typical
> stack overflow case no longer exists.
>
> --
> Catalin

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] arm64: fix add kasan bug

[Catalin Marinas]

On Wed, Jan 06, 2016 at 12:17:17AM +0300, Andrey Ryabinin wrote:
> 2016-01-05 13:10 GMT+03:00 Catalin Marinas <catalin.marinas@arm.com>:
> > On Thu, Dec 31, 2015 at 10:09:09AM +0000, zhongjiang wrote:
> >> From: zhong jiang <zhongjiang@huawei.com>
> >>
> >> In general, each process have 16kb stack space to use, but
> >> stack need extra space to store red_zone when kasan enable.
> >> the patch fix above question.
> >>
> >> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
> >> ---
> >>  arch/arm64/include/asm/thread_info.h | 15 +++++++++++++--
> >>  1 file changed, 13 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
> >> index 90c7ff2..45b5a7e 100644
> >> --- a/arch/arm64/include/asm/thread_info.h
> >> +++ b/arch/arm64/include/asm/thread_info.h
> > [...]
> >> +#ifdef CONFIG_KASAN
> >> +#define THREAD_SIZE          32768
> >> +#else
> >>  #define THREAD_SIZE          16384
> >> +#endif
> >
> > I'm not really keen on increasing the stack size to 32KB when KASan is
> > enabled (that's 8 4K pages). Have you actually seen a real problem with
> > the default size?
>
> > How large is the red_zone?
>
> Typical stack frame layout looks like this:
>     | 32-byte redzone | variable-1| padding-redzone to the next
> 32-byte boundary| variable-2|padding |.... | 32-byte redzone|
>
> AFAIK gcc creates redzones  only if it can't prove that all accesses
> to variable are valid (e.g. reference to variable passed to external
> function).
> Besides redzones, stack could be increased due to additional spilling.
> Although arm64 should be less affected by this since it has more
> registers than x86_64.
> On x86_64 I've seen few bad cases where stack frame of a single
> function was bloated up to 6K.

I think on arm64 we shouldn't be affected that badly. I did some tests
(well, running LTP and checking the maximum stack usage). Without KASan,
I get about 5-6KB usage maximum. Once KASan is enabled, the maximum
stack utilisation is around 8KB.

I also changed FRAME_WARN to be 2048 with KASAN but it didn't trigger
any warning on arm64 (defconfig + KASAN).

Of course, there is a risk of IRQ followed by softirq which is what led
us to increase the stack size to 16KB. However, in 4.5 we'll have
separate IRQ stacks while still keeping THREAD_SIZE to 16KB. In 4.6, the
plan is to try to reduce default THREAD_SIZE to 8KB.

So it's only in 4.6 (if we go for 8KB THREAD_SIZE) that we should
increase the stack when KASAN is enabled (though to 16KB rather than
32KB).

I don't think 4.5 needs any adjustments and for 4.4 I would only do this
*if* there is actually a regression. However, I haven't seen any such
report yet, in which case I NAK this patch (at least until further
information emerges).

--
Catalin
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH] arm64: fix add kasan bug

[Catalin Marinas]

On Wed, Jan 06, 2016 at 12:17:17AM +0300, Andrey Ryabinin wrote:
> 2016-01-05 13:10 GMT+03:00 Catalin Marinas <catalin.marinas@arm.com>:
> > On Thu, Dec 31, 2015 at 10:09:09AM +0000, zhongjiang wrote:
> >> From: zhong jiang <zhongjiang@huawei.com>
> >>
> >> In general, each process have 16kb stack space to use, but
> >> stack need extra space to store red_zone when kasan enable.
> >> the patch fix above question.
> >>
> >> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
> >> ---
> >>  arch/arm64/include/asm/thread_info.h | 15 +++++++++++++--
> >>  1 file changed, 13 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
> >> index 90c7ff2..45b5a7e 100644
> >> --- a/arch/arm64/include/asm/thread_info.h
> >> +++ b/arch/arm64/include/asm/thread_info.h
> > [...]
> >> +#ifdef CONFIG_KASAN
> >> +#define THREAD_SIZE          32768
> >> +#else
> >>  #define THREAD_SIZE          16384
> >> +#endif
> >
> > I'm not really keen on increasing the stack size to 32KB when KASan is
> > enabled (that's 8 4K pages). Have you actually seen a real problem with
> > the default size?
>
> > How large is the red_zone?
>
> Typical stack frame layout looks like this:
>     | 32-byte redzone | variable-1| padding-redzone to the next
> 32-byte boundary| variable-2|padding |.... | 32-byte redzone|
>
> AFAIK gcc creates redzones  only if it can't prove that all accesses
> to variable are valid (e.g. reference to variable passed to external
> function).
> Besides redzones, stack could be increased due to additional spilling.
> Although arm64 should be less affected by this since it has more
> registers than x86_64.
> On x86_64 I've seen few bad cases where stack frame of a single
> function was bloated up to 6K.

I think on arm64 we shouldn't be affected that badly. I did some tests
(well, running LTP and checking the maximum stack usage). Without KASan,
I get about 5-6KB usage maximum. Once KASan is enabled, the maximum
stack utilisation is around 8KB.

I also changed FRAME_WARN to be 2048 with KASAN but it didn't trigger
any warning on arm64 (defconfig + KASAN).

Of course, there is a risk of IRQ followed by softirq which is what led
us to increase the stack size to 16KB. However, in 4.5 we'll have
separate IRQ stacks while still keeping THREAD_SIZE to 16KB. In 4.6, the
plan is to try to reduce default THREAD_SIZE to 8KB.

So it's only in 4.6 (if we go for 8KB THREAD_SIZE) that we should
increase the stack when KASAN is enabled (though to 16KB rather than
32KB).

I don't think 4.5 needs any adjustments and for 4.4 I would only do this
*if* there is actually a regression. However, I haven't seen any such
report yet, in which case I NAK this patch (at least until further
information emerges).

--
Catalin
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH] arm64: fix add kasan bug

[zhongjiang]

From: zhong jiang <zhongjiang@huawei.com>

In general, each process have 16kb stack space to use, but
stack need extra space to store red_zone when kasan enable.
the patch fix above question.

Signed-off-by: zhong jiang <zhongjiang@huawei.com>
---
 arch/arm64/include/asm/thread_info.h | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
index 90c7ff2..45b5a7e 100644
--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h
@@ -23,13 +23,24 @@
 
 #include <linux/compiler.h>
 
+#ifdef CONFIG_KASAN
+#define KASAN_STACK_ORDER 1
+#else
+#define KASAN_STACK_ORDER 0
+#endif
+
 #ifdef CONFIG_ARM64_4K_PAGES
-#define THREAD_SIZE_ORDER	2
+#define THREAD_SIZE_ORDER	(2 + KASAN_STACK_ORDER)
 #elif defined(CONFIG_ARM64_16K_PAGES)
-#define THREAD_SIZE_ORDER	0
+#define THREAD_SIZE_ORDER	(0 + KASAN_STACK_ORDER)
 #endif
 
+#ifdef CONFIG_KASAN
+#define THREAD_SIZE		32768
+#else
 #define THREAD_SIZE		16384
+#endif
+
 #define THREAD_START_SP		(THREAD_SIZE - 16)
 
 #ifndef __ASSEMBLY__
-- 
2.0.0