* [PATCH] string.h: work around for increased stack usage
@ 2017-12-05 21:51 Arnd Bergmann
2017-12-05 22:02 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2017-12-05 21:51 UTC (permalink / raw)
To: Andrew Morton
Cc: Kees Cook, Mauro Carvalho Chehab, linux-media, kasan-dev,
Dmitry Vyukov, Alexander Potapenko, Andrey Ryabinin,
linux-kbuild, Arnd Bergmann, stable, Daniel Micay,
Greg Kroah-Hartman, Martin Wilck, Dan Williams, linux-kernel
The hardened strlen() function causes rather large stack usage in at
least one file in the kernel, in particular when CONFIG_KASAN is enabled:
drivers/media/usb/em28xx/em28xx-dvb.c: In function 'em28xx_dvb_init':
drivers/media/usb/em28xx/em28xx-dvb.c:2062:1: error: the frame size of 3256 bytes is larger than 204 bytes [-Werror=frame-larger-than=]
Analyzing this problem led to the discovery that gcc fails to merge the
stack slots for the i2c_board_info[] structures after we strlcpy() into
them, due to the 'noreturn' attribute on the source string length check.
I reported this as a gcc bug, but it is unlikely to get fixed for gcc-8,
since it is relatively easy to work around, and it gets triggered rarely.
An earlier workaround I did added an empty inline assembly statement
before the call to fortify_panic(), which works surprisingly well,
but is really ugly and unintuitive.
This is a new approach to the same problem, this time addressing it by
not calling the 'extern __real_strnlen()' function for string constants
where __builtin_strlen() is a compile-time constant and therefore known
to be safe. We do this by checking if the last character in the string
is a compile-time constant '\0'. If it is, we can assume that
strlen() of the string is also constant. As a side-effect, this should
also improve the object code output for any other call of strlen()
on a string constant.
Cc: stable@vger.kernel.org
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82365
Link: https://patchwork.kernel.org/patch/9980413/
Link: https://patchwork.kernel.org/patch/9974047/
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
v3: don't use an asm barrier but use a constant string change.
Aside from two other patches for drivers/media that I sent last week,
this should fix all stack frames above 2KB, once all three are merged,
I'll send the patch to re-enable the warning.
---
include/linux/string.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/linux/string.h b/include/linux/string.h
index 410ecf17de3c..e5cc3f27f6e0 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -259,7 +259,8 @@ __FORTIFY_INLINE __kernel_size_t strlen(const char *p)
{
__kernel_size_t ret;
size_t p_size = __builtin_object_size(p, 0);
- if (p_size == (size_t)-1)
+ if (p_size == (size_t)-1 ||
+ (__builtin_constant_p(p[p_size - 1]) && p[p_size - 1] == '\0'))
return __builtin_strlen(p);
ret = strnlen(p, p_size);
if (p_size <= ret)
--
2.9.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] string.h: work around for increased stack usage
2017-12-05 21:51 [PATCH] string.h: work around for increased stack usage Arnd Bergmann
@ 2017-12-05 22:02 ` Andrew Morton
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Morton @ 2017-12-05 22:02 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Kees Cook, Mauro Carvalho Chehab, linux-media, kasan-dev,
Dmitry Vyukov, Alexander Potapenko, Andrey Ryabinin,
linux-kbuild, stable, Daniel Micay, Greg Kroah-Hartman,
Martin Wilck, Dan Williams, linux-kernel
On Tue, 5 Dec 2017 22:51:19 +0100 Arnd Bergmann <arnd@arndb.de> wrote:
> The hardened strlen() function causes rather large stack usage in at
> least one file in the kernel, in particular when CONFIG_KASAN is enabled:
>
> drivers/media/usb/em28xx/em28xx-dvb.c: In function 'em28xx_dvb_init':
> drivers/media/usb/em28xx/em28xx-dvb.c:2062:1: error: the frame size of 3256 bytes is larger than 204 bytes [-Werror=frame-larger-than=]
>
> Analyzing this problem led to the discovery that gcc fails to merge the
> stack slots for the i2c_board_info[] structures after we strlcpy() into
> them, due to the 'noreturn' attribute on the source string length check.
>
> I reported this as a gcc bug, but it is unlikely to get fixed for gcc-8,
> since it is relatively easy to work around, and it gets triggered rarely.
> An earlier workaround I did added an empty inline assembly statement
> before the call to fortify_panic(), which works surprisingly well,
> but is really ugly and unintuitive.
>
> This is a new approach to the same problem, this time addressing it by
> not calling the 'extern __real_strnlen()' function for string constants
> where __builtin_strlen() is a compile-time constant and therefore known
> to be safe. We do this by checking if the last character in the string
> is a compile-time constant '\0'. If it is, we can assume that
> strlen() of the string is also constant. As a side-effect, this should
> also improve the object code output for any other call of strlen()
> on a string constant.
>
> Cc: stable@vger.kernel.org
I'll add
Fixes: 6974f0c4555 ("include/linux/string.h: add the option of fortified string.h functions")
to simplify stable@'s life.
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82365
> Link: https://patchwork.kernel.org/patch/9980413/
> Link: https://patchwork.kernel.org/patch/9974047/
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> ...
>
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -259,7 +259,8 @@ __FORTIFY_INLINE __kernel_size_t strlen(const char *p)
> {
> __kernel_size_t ret;
> size_t p_size = __builtin_object_size(p, 0);
> - if (p_size == (size_t)-1)
> + if (p_size == (size_t)-1 ||
> + (__builtin_constant_p(p[p_size - 1]) && p[p_size - 1] == '\0'))
> return __builtin_strlen(p);
> ret = strnlen(p, p_size);
> if (p_size <= ret)
Let's have some sympathy for our poor readers?
--- a/include/linux/string.h~stringh-work-around-for-increased-stack-usage-fix
+++ a/include/linux/string.h
@@ -259,6 +259,8 @@ __FORTIFY_INLINE __kernel_size_t strlen(
{
__kernel_size_t ret;
size_t p_size = __builtin_object_size(p, 0);
+
+ /* Work around gcc excess stack consumption issue */
if (p_size == (size_t)-1 ||
(__builtin_constant_p(p[p_size - 1]) && p[p_size - 1] == '\0'))
return __builtin_strlen(p);
_
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 4/9] em28xx: fix em28xx_dvb_init for KASAN
@ 2017-10-02 8:33 Arnd Bergmann
2017-10-02 8:40 ` [PATCH] string.h: work around for increased stack usage Arnd Bergmann
0 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2017-10-02 8:33 UTC (permalink / raw)
To: Andrey Ryabinin
Cc: David Laight, Mauro Carvalho Chehab, Jiri Pirko,
Arend van Spriel, Kalle Valo, David S. Miller,
Alexander Potapenko, Dmitry Vyukov, Masahiro Yamada,
Michal Marek, Andrew Morton, Kees Cook, Geert Uytterhoeven,
Greg Kroah-Hartman, linux-media, linux-kernel, netdev,
linux-wireless, brcm80211-dev-list.pdl, brcm80211-dev-list,
kasan-dev, linux-kbuild, Jakub Jelinek, Martin Liška,
stable
On Thu, Sep 28, 2017 at 4:30 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thu, Sep 28, 2017 at 6:09 AM, Andrey Ryabinin
> <aryabinin@virtuozzo.com> wrote:
>> On 09/27/2017 04:26 PM, Arnd Bergmann wrote:
>>> On Tue, Sep 26, 2017 at 9:49 AM, Andrey Ryabinin
>>> <aryabinin@virtuozzo.com> wrote:
>
>>> --- a/include/linux/string.h
>>> +++ b/include/linux/string.h
>>> @@ -227,7 +227,7 @@ static inline const char *kbasename(const char *path)
>>> #define __FORTIFY_INLINE extern __always_inline __attribute__((gnu_inline))
>>> #define __RENAME(x) __asm__(#x)
>>>
>>> -void fortify_panic(const char *name) __noreturn __cold;
>>> +void fortify_panic(const char *name) __cold;
>>> void __read_overflow(void) __compiletime_error("detected read beyond
>>> size of object passed as 1st parameter");
>>> void __read_overflow2(void) __compiletime_error("detected read beyond
>>> size of object passed as 2nd parameter");
>>> void __read_overflow3(void) __compiletime_error("detected read beyond
>>> size of object passed as 3rd parameter");
>>>
>>> I don't immediately see why the __noreturn changes the behavior here, any idea?
>>>
>>
>>
>> At first I thought that this somehow might be related to __asan_handle_no_return(). GCC calls it
>> before noreturn function. So I made patch to remove generation of these calls (we don't need them in the kernel anyway)
>> but it didn't help. It must be something else than.
>
> I made a reduced test case yesterday (see http://paste.ubuntu.com/25628030/),
> and it shows the same behavior with and without the sanitizer, it uses 128
> bytes without the noreturn attribute and 480 bytes when its added, the sanitizer
> adds a factor of 1.5x on top. It's possible that I did something wrong while
> reducing, since the original driver file uses very little stack (a few hundred
> bytes) without -fsanitize=kernel-address, but finding out what happens in
> the reduced case may still help understand the other one.
This is now GCC PR82365, see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82365
I've come up with a workaround, but I'm not sure if that is any better than the
alternatives, will send the patch as a follow-up in a bit.
Arnd
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] string.h: work around for increased stack usage
2017-10-02 8:33 [PATCH v4 4/9] em28xx: fix em28xx_dvb_init for KASAN Arnd Bergmann
@ 2017-10-02 8:40 ` Arnd Bergmann
2017-10-02 9:02 ` Arnd Bergmann
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Arnd Bergmann @ 2017-10-02 8:40 UTC (permalink / raw)
To: Andrey Ryabinin
Cc: David Laight, Mauro Carvalho Chehab, David S . Miller,
Alexander Potapenko, Dmitry Vyukov, Masahiro Yamada,
Andrew Morton, Kees Cook, Geert Uytterhoeven, Greg Kroah-Hartman,
linux-media @ vger . kernel . org,
linux-kernel @ vger . kernel . org,
kasan-dev @ googlegroups . com,
linux-kbuild @ vger . kernel . org, Arnd Bergmann
The hardened strlen() function causes rather large stack usage
in at least one file in the kernel when CONFIG_KASAN is enabled:
drivers/media/usb/em28xx/em28xx-dvb.c: In function 'em28xx_dvb_init':
drivers/media/usb/em28xx/em28xx-dvb.c:2062:1: error: the frame size of 3256 bytes is larger than 204 bytes [-Werror=frame-larger-than=]
Analyzing this problem led to the discovery that gcc fails to
merge the stack slots for the i2c_board_info[] structures after
we strlcpy() into them, due to the 'noreturn' attribute on the
source string length check.
The compiler behavior should get fixed in gcc-8, but for users
of existing gcc versions, we can work around it using an empty
inline assembly statement before the call to fortify_panic().
The workaround is unfortunately very ugly, and I tried my best
to limit it being applied to affected versions of gcc when
KASAN is used. Alternative suggestions welcome.
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82365
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
include/linux/string.h | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/include/linux/string.h b/include/linux/string.h
index c7a1132cdc93..1bf5ecdf8e01 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -228,6 +228,16 @@ static inline const char *kbasename(const char *path)
#define __RENAME(x) __asm__(#x)
void fortify_panic(const char *name) __noreturn __cold;
+
+/* work around GCC PR82365 */
+#if defined(CONFIG_KASAN) && !defined(__clang__) && GCC_VERSION <= 80000
+#define fortify_panic(x) \
+ do { \
+ asm volatile(""); \
+ fortify_panic(x); \
+ } while (0)
+#endif
+
void __read_overflow(void) __compiletime_error("detected read beyond size of object passed as 1st parameter");
void __read_overflow2(void) __compiletime_error("detected read beyond size of object passed as 2nd parameter");
void __read_overflow3(void) __compiletime_error("detected read beyond size of object passed as 3rd parameter");
--
2.9.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] string.h: work around for increased stack usage
2017-10-02 8:40 ` [PATCH] string.h: work around for increased stack usage Arnd Bergmann
@ 2017-10-02 9:02 ` Arnd Bergmann
2017-10-02 14:07 ` Andrey Ryabinin
2017-10-03 18:10 ` kbuild test robot
2 siblings, 0 replies; 6+ messages in thread
From: Arnd Bergmann @ 2017-10-02 9:02 UTC (permalink / raw)
To: Andrey Ryabinin
Cc: David Laight, Mauro Carvalho Chehab, David S . Miller,
Alexander Potapenko, Dmitry Vyukov, Masahiro Yamada,
Andrew Morton, Kees Cook, Geert Uytterhoeven, Greg Kroah-Hartman,
linux-media @ vger . kernel . org,
linux-kernel @ vger . kernel . org,
kasan-dev @ googlegroups . com,
linux-kbuild @ vger . kernel . org, Arnd Bergmann
On Mon, Oct 2, 2017 at 10:40 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>
> void fortify_panic(const char *name) __noreturn __cold;
> +
> +/* work around GCC PR82365 */
> +#if defined(CONFIG_KASAN) && !defined(__clang__) && GCC_VERSION <= 80000
> +#define fortify_panic(x) \
> + do { \
> + asm volatile(""); \
> + fortify_panic(x); \
> + } while (0)
> +#endif
This broke the build for the fortify_panic() definition in lib/string.c which
clashes with the macro. I've fixed it locally by renaming it to __fortify_panic,
but won't post the fixed version until I get some feedback on the basic
approach.
Arnd
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] string.h: work around for increased stack usage
2017-10-02 8:40 ` [PATCH] string.h: work around for increased stack usage Arnd Bergmann
2017-10-02 9:02 ` Arnd Bergmann
@ 2017-10-02 14:07 ` Andrey Ryabinin
2017-10-03 18:10 ` kbuild test robot
2 siblings, 0 replies; 6+ messages in thread
From: Andrey Ryabinin @ 2017-10-02 14:07 UTC (permalink / raw)
To: Arnd Bergmann
Cc: David Laight, Mauro Carvalho Chehab, David S . Miller,
Alexander Potapenko, Dmitry Vyukov, Masahiro Yamada,
Andrew Morton, Kees Cook, Geert Uytterhoeven, Greg Kroah-Hartman,
linux-media @ vger . kernel . org,
linux-kernel @ vger . kernel . org,
kasan-dev @ googlegroups . com,
linux-kbuild @ vger . kernel . org
On 10/02/2017 11:40 AM, Arnd Bergmann wrote:
> The hardened strlen() function causes rather large stack usage
> in at least one file in the kernel when CONFIG_KASAN is enabled:
>
> drivers/media/usb/em28xx/em28xx-dvb.c: In function 'em28xx_dvb_init':
> drivers/media/usb/em28xx/em28xx-dvb.c:2062:1: error: the frame size of 3256 bytes is larger than 204 bytes [-Werror=frame-larger-than=]
>
> Analyzing this problem led to the discovery that gcc fails to
> merge the stack slots for the i2c_board_info[] structures after
> we strlcpy() into them, due to the 'noreturn' attribute on the
> source string length check.
>
> The compiler behavior should get fixed in gcc-8, but for users
> of existing gcc versions, we can work around it using an empty
> inline assembly statement before the call to fortify_panic().
>
> The workaround is unfortunately very ugly, and I tried my best
> to limit it being applied to affected versions of gcc when
> KASAN is used. Alternative suggestions welcome.
>
I don't have a really strong preference, so this approach is fine by me,
but s/strlcpy/[strncpy|memcpy] approach seems a little better to me, because it's not ugly.
This ugly workaround would make more sense if we a had lot of cases like in em28xx_dvb_init().
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82365
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> include/linux/string.h | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/include/linux/string.h b/include/linux/string.h
> index c7a1132cdc93..1bf5ecdf8e01 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -228,6 +228,16 @@ static inline const char *kbasename(const char *path)
> #define __RENAME(x) __asm__(#x)
>
> void fortify_panic(const char *name) __noreturn __cold;
> +
> +/* work around GCC PR82365 */
> +#if defined(CONFIG_KASAN) && !defined(__clang__) && GCC_VERSION <= 80000
> +#define fortify_panic(x) \
> + do { \
> + asm volatile(""); \
> + fortify_panic(x); \
> + } while (0)
> +#endif
> +
> void __read_overflow(void) __compiletime_error("detected read beyond size of object passed as 1st parameter");
> void __read_overflow2(void) __compiletime_error("detected read beyond size of object passed as 2nd parameter");
> void __read_overflow3(void) __compiletime_error("detected read beyond size of object passed as 3rd parameter");
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] string.h: work around for increased stack usage
2017-10-02 8:40 ` [PATCH] string.h: work around for increased stack usage Arnd Bergmann
2017-10-02 9:02 ` Arnd Bergmann
2017-10-02 14:07 ` Andrey Ryabinin
@ 2017-10-03 18:10 ` kbuild test robot
2 siblings, 0 replies; 6+ messages in thread
From: kbuild test robot @ 2017-10-03 18:10 UTC (permalink / raw)
To: Arnd Bergmann
Cc: kbuild-all, Andrey Ryabinin, David Laight, Mauro Carvalho Chehab,
David S . Miller, Alexander Potapenko, Dmitry Vyukov,
Masahiro Yamada, Andrew Morton, Kees Cook, Geert Uytterhoeven,
Greg Kroah-Hartman, linux-media @ vger . kernel . org,
linux-kernel @ vger . kernel . org,
kasan-dev @ googlegroups . com,
linux-kbuild @ vger . kernel . org
[-- Attachment #1: Type: text/plain, Size: 1529 bytes --]
Hi Arnd,
[auto build test ERROR on linus/master]
[also build test ERROR on v4.14-rc3 next-20170929]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Arnd-Bergmann/string-h-work-around-for-increased-stack-usage/20171003-210611
config: x86_64-randconfig-ws0-10040032 (attached as .config)
compiler: gcc-4.8 (Debian 4.8.4-1) 4.8.4
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All error/warnings (new ones prefixed by >>):
In file included from lib/string.c:23:0:
>> include/linux/string.h:235:2: error: expected identifier or '(' before 'do'
do { \
^
>> lib/string.c:1048:6: note: in expansion of macro 'fortify_panic'
void fortify_panic(const char *name)
^
>> include/linux/string.h:238:4: error: expected identifier or '(' before 'while'
} while (0)
^
>> lib/string.c:1048:6: note: in expansion of macro 'fortify_panic'
void fortify_panic(const char *name)
^
vim +235 include/linux/string.h
231
232 /* work around GCC PR82365 */
233 #if defined(CONFIG_KASAN) && !defined(__clang__) && GCC_VERSION <= 80000
234 #define fortify_panic(x) \
> 235 do { \
236 asm volatile(""); \
237 fortify_panic(x); \
> 238 } while (0)
239 #endif
240
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32772 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-12-05 22:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-05 21:51 [PATCH] string.h: work around for increased stack usage Arnd Bergmann
2017-12-05 22:02 ` Andrew Morton
-- strict thread matches above, loose matches on Subject: below --
2017-10-02 8:33 [PATCH v4 4/9] em28xx: fix em28xx_dvb_init for KASAN Arnd Bergmann
2017-10-02 8:40 ` [PATCH] string.h: work around for increased stack usage Arnd Bergmann
2017-10-02 9:02 ` Arnd Bergmann
2017-10-02 14:07 ` Andrey Ryabinin
2017-10-03 18:10 ` kbuild test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).