[PATCH 24/26] ocfs2: reduce stack size with KASAN
The internal logging infrastructure in ocfs2 causes special warning code to be used with KASAN, which produces rather large stack frames: fs/ocfs2/super.c: In function 'ocfs2_fill_super': fs/ocfs2/super.c:1219:1: error: the frame size of 3264 bytes is larger than 3072 bytes [-Werror=frame-larger-than=] By simply passing the mask by value instead of reference, we can avoid the problem completely. On 64-bit architectures, this is also more efficient, while on the less common (at least among ocfs2 users) 32-bit architectures, I'm guessing that the resulting code is comparable to what it was before. The current version was introduced by Joe Perches as an optimization, maybe he can see if my change regresses compared to his. Cc: Joe Perches Fixes: 7c2bd2f930ae ("ocfs2: reduce object size of mlog uses") Signed-off-by: Arnd Bergmann --- fs/ocfs2/cluster/masklog.c | 10 +- fs/ocfs2/cluster/masklog.h | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/fs/ocfs2/cluster/masklog.c b/fs/ocfs2/cluster/masklog.c index d331c2386b94..9720c5443e4d 100644 --- a/fs/ocfs2/cluster/masklog.c +++ b/fs/ocfs2/cluster/masklog.c @@ -64,7 +64,7 @@ static ssize_t mlog_mask_store(u64 mask, const char *buf, size_t count) return count; } -void __mlog_printk(const u64 *mask, const char *func, int line, +void __mlog_printk(const u64 mask, const char *func, int line, const char *fmt, ...) { struct va_format vaf; @@ -72,14 +72,14 @@ void __mlog_printk(const u64 *mask, const char *func, int line, const char *level; const char *prefix = ""; - if (!__mlog_test_u64(*mask, mlog_and_bits) || - __mlog_test_u64(*mask, mlog_not_bits)) + if (!__mlog_test_u64(mask, mlog_and_bits) || + __mlog_test_u64(mask, mlog_not_bits)) return; - if (*mask & ML_ERROR) { + if (mask & ML_ERROR) { level = KERN_ERR; prefix = "ERROR: "; - } else if (*mask & ML_NOTICE) { + } else if (mask & ML_NOTICE) { level = KERN_NOTICE; } else { level = KERN_INFO; diff --git a/fs/ocfs2/cluster/masklog.h b/fs/ocfs2/cluster/masklog.h index 308ea0eb35fd..0d0f4bf2c3d8 100644 --- a/fs/ocfs2/cluster/masklog.h +++ b/fs/ocfs2/cluster/masklog.h @@ -163,7 +163,7 @@ extern struct mlog_bits mlog_and_bits, mlog_not_bits; #endif __printf(4, 5) -void __mlog_printk(const u64 *m, const char *func, int line, +void __mlog_printk(const u64 m, const char *func, int line, const char *fmt, ...); /* @@ -174,7 +174,7 @@ void __mlog_printk(const u64 *m, const char *func, int line, do { \ u64 _m = MLOG_MASK_PREFIX | (mask); \ if (_m & ML_ALLOWED_BITS) \ - __mlog_printk(&_m, __func__, __LINE__, fmt, \ + __mlog_printk(_m, __func__, __LINE__, fmt, \ ##__VA_ARGS__); \ } while (0) -- 2.9.0
Re: [PATCH 24/26] ocfs2: reduce stack size with KASAN
On Thu, 2017-03-02 at 17:38 +0100, Arnd Bergmann wrote: > The internal logging infrastructure in ocfs2 causes special warning code to be > used with KASAN, which produces rather large stack frames: > fs/ocfs2/super.c: In function 'ocfs2_fill_super': > fs/ocfs2/super.c:1219:1: error: the frame size of 3264 bytes is larger than > 3072 bytes [-Werror=frame-larger-than=] At least by default it doesn't seem to. gcc 6.2 allyesconfig, CONFIG_KASAN=y with either CONFIG_KASAN_INLINE or CONFIG_KASAN_OUTLINE gcc doesn't emit a stack warning > By simply passing the mask by value instead of reference, we can avoid the > problem completely. Any idea why that's so? > On 64-bit architectures, this is also more efficient, Efficient true, but the same overall stack no? > while on the less common (at least among ocfs2 users) 32-bit architectures, > I'm guessing that the resulting code is comparable to what it was before. > > The current version was introduced by Joe Perches as an optimization, maybe > he can see if my change regresses compared to his. I don't see it. > Cc: Joe Perches > Fixes: 7c2bd2f930ae ("ocfs2: reduce object size of mlog uses") > Signed-off-by: Arnd Bergmann > --- > fs/ocfs2/cluster/masklog.c | 10 +- > fs/o cfs2/cluster/masklog.h | 4 ++-- > 2 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/fs/ocfs2/cluster/masklog.c b/fs/ocfs2/cluster/masklog.c > index d331c2386b94..9720c5443e4d 100644 > --- a/fs/ocfs2/cluster/masklog.c > +++ b/fs/ocfs2/cluster/masklog.c > @@ -64,7 +64,7 @@ static ssize_t mlog_mask_store(u64 mask, const char *buf, > size_t count) > return count; > } > > -void __mlog_printk(const u64 *mask, const char *func, int line, > +void __mlog_printk(const u64 mask, const char *func, int line, > const char *fmt, ...) > { > struct va_format vaf; > @@ -72,14 +72,14 @@ void __mlog_printk(const u64 *mask, const char *func, int > line, > const char *level; > const char *prefix = ""; > > - if (!__mlog_test_u64(*mask, mlog_and_bits) || > - __mlog_test_u64(*mask, mlog_not_bits)) > + if (!__mlog_test_u64(mask, mlog_and_bits) || > + __mlog_test_u64(mask, mlog_not_bits)) > return; > > - if (*mask & ML_ERROR) { > + if (mask & ML_ERROR) { > level = KERN_ERR; > prefix = "ERROR: "; > - } else if (*mask & ML_NOTICE) { > + } else if (mask & ML_NOTICE) { > level = KERN_NOTICE; > } else { > level = KERN_INFO; > diff --git a/fs/ocfs2/cluster/masklog.h b/fs/ocfs2/cluster/masklog.h > index 308ea0eb35fd..0d0f4bf2c3d8 100644 > --- a/fs/ocfs2/cluster/masklog.h > +++ b/fs/ocfs2/cluster/masklog.h > @@ -163,7 +163,7 @@ extern struct mlog_bits mlog_and_bits, mlog_not_bits; > #endif > > __printf(4, 5) > -void __mlog_printk(const u64 *m, const char *func, int line, > +void __mlog_printk(const u64 m, const char *func, int line, > const char *fmt, ...); > > /* > @@ -174,7 +174,7 @@ void __mlog_printk(const u64 *m, const char *func, int > line, > do { \ > u64 _m = MLOG_MASK_PREFIX | (mask); \ > if (_m & ML_ALLOWED_BITS) \ > - __mlog_printk(&_m, __func__, __LINE__, fmt, \ > + __mlog_printk(_m, __func__, __LINE__, fmt, \ > ##__VA_ARGS__); \ > } while (0) >
Re: [PATCH 24/26] ocfs2: reduce stack size with KASAN
On Thu, 2017-03-02 at 23:22 +0100, Arnd Bergmann wrote: > On Thu, Mar 2, 2017 at 6:46 PM, Joe Perches wrote: > > On Thu, 2017-03-02 at 17:38 +0100, Arnd Bergmann wrote: > > > The internal logging infrastructure in ocfs2 causes special warning code > > > to be > > > used with KASAN, which produces rather large stack frames: > > > fs/ocfs2/super.c: In function 'ocfs2_fill_super': > > > fs/ocfs2/super.c:1219:1: error: the frame size of 3264 bytes is larger > > > than 3072 bytes [-Werror=frame-larger-than=] > > > > At least by default it doesn't seem to. > > > > gcc 6.2 allyesconfig, CONFIG_KASAN=y > > with either CONFIG_KASAN_INLINE or CONFIG_KASAN_OUTLINE > > > > gcc doesn't emit a stack warning > > The warning is disabled until patch 26/26. which picks the 3072 default. > The 3264 number was with gcc-7, which is worse than gcc-6 since it enables > an extra check. > > > > By simply passing the mask by value instead of reference, we can avoid the > > > problem completely. > > > > Any idea why that's so? > > With KASAN, every time we inline the function, the compiler has to allocate > space for another copy of the variable plus a redzone to detect whether > passing it by reference into another function causes an overflow at runtime. These logging functions aren't inlined. You're referring to the stack frame? > > > On 64-bit architectures, this is also more efficient, > > > > Efficient true, but the same overall stack no? > > Here is what I see with CONFIG_FRAME_WARN=300 and x86_64-linux-gcc-6.3.1: > > before: [] > fs/ocfs2/super.c:1219:1: error: the frame size of 552 bytes is larger > than 300 bytes [-Werror=frame-larger-than=] > > after: > fs/ocfs2/super.c: In function 'ocfs2_fill_super': > fs/ocfs2/super.c:1219:1: error: the frame size of 472 bytes is larger > than 300 bytes [-Werror=frame-larger-than=] > > and with gcc-7.0.1 (including -fsanitize-address-use-after-scope), before: [] > fs/ocfs2/super.c:1219:1: error: the frame size of 3264 bytes is larger > than 300 bytes [-Werror=frame-larger-than=] > > after: > fs/ocfs2/super.c: In function 'ocfs2_fill_super': > fs/ocfs2/super.c:1219:1: error: the frame size of 704 bytes is larger > than 300 bytes [-Werror=frame-larger-than=] Still doesn't make sense to me. None of the logging functions are inlined as they are all EXPORT_SYMBOL. This just changes a pointer to a u64, which is the same size on x86-64 (and is of course larger on x86-32). Perhaps KASAN has the odd behavior and working around KASAN's behavior may not be the proper thing to do. Maybe if CONFIG_KASAN is set, the minimum stack should be increased via THREAD_SIZE_ORDER or some such.
Re: [PATCH 24/26] ocfs2: reduce stack size with KASAN
On Thu, Mar 2, 2017 at 6:46 PM, Joe Perches wrote: > On Thu, 2017-03-02 at 17:38 +0100, Arnd Bergmann wrote: >> The internal logging infrastructure in ocfs2 causes special warning code to >> be >> used with KASAN, which produces rather large stack frames: > >> fs/ocfs2/super.c: In function 'ocfs2_fill_super': >> fs/ocfs2/super.c:1219:1: error: the frame size of 3264 bytes is larger than >> 3072 bytes [-Werror=frame-larger-than=] > > At least by default it doesn't seem to. > > gcc 6.2 allyesconfig, CONFIG_KASAN=y > with either CONFIG_KASAN_INLINE or CONFIG_KASAN_OUTLINE > > gcc doesn't emit a stack warning The warning is disabled until patch 26/26. which picks the 3072 default. The 3264 number was with gcc-7, which is worse than gcc-6 since it enables an extra check. >> By simply passing the mask by value instead of reference, we can avoid the >> problem completely. > > Any idea why that's so? With KASAN, every time we inline the function, the compiler has to allocate space for another copy of the variable plus a redzone to detect whether passing it by reference into another function causes an overflow at runtime. >> On 64-bit architectures, this is also more efficient, > > Efficient true, but the same overall stack no? Here is what I see with CONFIG_FRAME_WARN=300 and x86_64-linux-gcc-6.3.1: before: fs/ocfs2/super.c: In function 'ocfs2_parse_options.isra.3': fs/ocfs2/super.c:1508:1: error: the frame size of 352 bytes is larger than 300 bytes [-Werror=frame-larger-than=] fs/ocfs2/super.c: In function 'ocfs2_enable_quotas': fs/ocfs2/super.c:974:1: error: the frame size of 344 bytes is larger than 300 bytes [-Werror=frame-larger-than=] fs/ocfs2/super.c: In function 'ocfs2_fill_super': fs/ocfs2/super.c:1219:1: error: the frame size of 552 bytes is larger than 300 bytes [-Werror=frame-larger-than=] after: fs/ocfs2/super.c: In function 'ocfs2_fill_super': fs/ocfs2/super.c:1219:1: error: the frame size of 472 bytes is larger than 300 bytes [-Werror=frame-larger-than=] and with gcc-7.0.1 (including -fsanitize-address-use-after-scope), before: fs/ocfs2/super.c: In function 'ocfs2_check_volume': fs/ocfs2/super.c:2512:1: error: the frame size of 768 bytes is larger than 300 bytes [-Werror=frame-larger-than=] fs/ocfs2/super.c: In function 'ocfs2_statfs': fs/ocfs2/super.c:1717:1: error: the frame size of 320 bytes is larger than 300 bytes [-Werror=frame-larger-than=] fs/ocfs2/super.c: In function 'ocfs2_parse_options.isra.3': fs/ocfs2/super.c:1508:1: error: the frame size of 464 bytes is larger than 300 bytes [-Werror=frame-larger-than=] fs/ocfs2/super.c: In function 'ocfs2_enable_quotas': fs/ocfs2/super.c:974:1: error: the frame size of 320 bytes is larger than 300 bytes [-Werror=frame-larger-than=] fs/ocfs2/super.c: In function 'ocfs2_remount': fs/ocfs2/super.c:752:1: error: the frame size of 568 bytes is larger than 300 bytes [-Werror=frame-larger-than=] fs/ocfs2/super.c: In function 'ocfs2_initialize_super.isra.8': fs/ocfs2/super.c:2339:1: error: the frame size of 1712 bytes is larger than 300 bytes [-Werror=frame-larger-than=] fs/ocfs2/super.c: In function 'ocfs2_fill_super': fs/ocfs2/super.c:1219:1: error: the frame size of 3264 bytes is larger than 300 bytes [-Werror=frame-larger-than=] after: fs/ocfs2/super.c: In function 'ocfs2_fill_super': fs/ocfs2/super.c:1219:1: error: the frame size of 704 bytes is larger than 300 bytes [-Werror=frame-larger-than=] Arnd
Re: [PATCH 24/26] ocfs2: reduce stack size with KASAN
On Thu, Mar 2, 2017 at 11:40 PM, Joe Perches wrote: > On Thu, 2017-03-02 at 23:22 +0100, Arnd Bergmann wrote: >> On Thu, Mar 2, 2017 at 6:46 PM, Joe Perches wrote: >> > On Thu, 2017-03-02 at 17:38 +0100, Arnd Bergmann wrote: >> > > The internal logging infrastructure in ocfs2 causes special warning code >> > > to be >> > > used with KASAN, which produces rather large stack frames: >> > > fs/ocfs2/super.c: In function 'ocfs2_fill_super': >> > > fs/ocfs2/super.c:1219:1: error: the frame size of 3264 bytes is larger >> > > than 3072 bytes [-Werror=frame-larger-than=] >> > >> > At least by default it doesn't seem to. >> > >> > gcc 6.2 allyesconfig, CONFIG_KASAN=y >> > with either CONFIG_KASAN_INLINE or CONFIG_KASAN_OUTLINE >> > >> > gcc doesn't emit a stack warning >> >> The warning is disabled until patch 26/26. which picks the 3072 default. >> The 3264 number was with gcc-7, which is worse than gcc-6 since it enables >> an extra check. >> >> > > By simply passing the mask by value instead of reference, we can avoid >> > > the >> > > problem completely. >> > >> > Any idea why that's so? >> >> With KASAN, every time we inline the function, the compiler has to allocate >> space for another copy of the variable plus a redzone to detect whether >> passing it by reference into another function causes an overflow at runtime. > > These logging functions aren't inlined. Sorry, my mistake. In this case mlog() is a macro, not an inline functions. The effect is the same though. > You're referring to the stack frame? The stack frame of the function that calls mlog(), yes. > > Still doesn't make sense to me. > > None of the logging functions are inlined as they are all > EXPORT_SYMBOL. mlog() is placed in the calling function. > This just changes a pointer to a u64, which is the same > size on x86-64 (and is of course larger on x86-32). KASAN decides that passing a pointer to _m into an extern function (_mlog_printk) is potentially dangerous, as that function might keep a reference to that pointer after it goes out of scope, or it might not know the correct length of the stack object pointed to. We can see from looking at the __mlog_printk() function definition that it's actually safe, but the compiler cannot see that when looking at another source file. > Perhaps KASAN has the odd behavior and working around > KASAN's behavior may not be the proper thing to do. Turning off KASAN fixes the problem, but the entire purpose of KASAN is to identify code that is potentially dangerous. > Maybe if CONFIG_KASAN is set, the minimum stack should > be increased via THREAD_SIZE_ORDER or some such. This is what happened in 3f181b4d8652 ("lib/Kconfig.debug: disable -Wframe-larger-than warnings with KASAN=y"). I'm trying to revert that patch so we actually get warnings again about functions that are still dangerous. I picked 3072 as an arbitrary limit, as there are only a handful of files that use larger stack frames in the worst case, but we can only use that limit after fixing up all the warnings it shows. Arnd
Re: [PATCH 24/26] ocfs2: reduce stack size with KASAN
On Thu, 2017-03-02 at 23:59 +0100, Arnd Bergmann wrote: > KASAN decides that passing a pointer to _m into an extern function > (_mlog_printk) is potentially dangerous, as that function might > keep a reference to that pointer after it goes out of scope, > or it might not know the correct length of the stack object pointed to. > > We can see from looking at the __mlog_printk() function definition > that it's actually safe, but the compiler cannot see that when looking > at another source file. OK, thanks. btw: changing __mlog_printk can save ~11% (90+KB) of object text size by removing __func__ and __LINE__ and using vsprintf pointer extension %pS, __builtin_return_address(0) as it is already used in dlmmaster. (defconfig x86-64, with ocfs2) $ size fs/ocfs2/built-in.o* textdata bss dec hex filename 759791 111373 105688 976852 ee7d4 fs/ocfs2/built-in.o.new 852959 111373 105688 1070020 1053c4 fs/ocfs2/built-in.o.old It's nearly the same output. --- fs/ocfs2/cluster/masklog.c | 8 fs/ocfs2/cluster/masklog.h | 8 +++- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/fs/ocfs2/cluster/masklog.c b/fs/ocfs2/cluster/masklog.c index d331c2386b94..a3f080f37108 100644 --- a/fs/ocfs2/cluster/masklog.c +++ b/fs/ocfs2/cluster/masklog.c @@ -64,8 +64,7 @@ static ssize_t mlog_mask_store(u64 mask, const char *buf, size_t count) return count; } -void __mlog_printk(const u64 *mask, const char *func, int line, - const char *fmt, ...) +void __mlog_printk(const u64 *mask, const char *fmt, ...) { struct va_format vaf; va_list args; @@ -90,9 +89,10 @@ void __mlog_printk(const u64 *mask, const char *func, int line, vaf.fmt = fmt; vaf.va = &args; - printk("%s(%s,%u,%u):%s:%d %s%pV", + printk("%s(%s,%u,%u):%pS %s%pV", level, current->comm, task_pid_nr(current), - raw_smp_processor_id(), func, line, prefix, &vaf); + raw_smp_processor_id(), __builtin_return_address(0), + prefix, &vaf); va_end(args); } diff --git a/fs/ocfs2/cluster/masklog.h b/fs/ocfs2/cluster/masklog.h index 3c16da69605d..56ba5baf625b 100644 --- a/fs/ocfs2/cluster/masklog.h +++ b/fs/ocfs2/cluster/masklog.h @@ -162,9 +162,8 @@ extern struct mlog_bits mlog_and_bits, mlog_not_bits; #endif -__printf(4, 5) __nocapture(2) -void __mlog_printk(const u64 *m, const char *func, int line, - const char *fmt, ...); +__printf(2, 3) __nocapture(2) +void __mlog_printk(const u64 *m, const char *fmt, ...); /* * Testing before the __mlog_printk call lets the compiler eliminate the @@ -174,8 +173,7 @@ void __mlog_printk(const u64 *m, const char *func, int line, do { \ u64 _m = MLOG_MASK_PREFIX | (mask); \ if (_m & ML_ALLOWED_BITS) \ - __mlog_printk(&_m, __func__, __LINE__, fmt, \ - ##__VA_ARGS__); \ + __mlog_printk(&_m, fmt, ##__VA_ARGS__); \ } while (0) #define mlog_errno(st) ({ \